[U-Boot] [PATCH] dm: i2c: mxc_i2c: implement i2c_idle_bus

[Peng Fan]

Hi Heiko,

On Mon, Mar 21, 2016 at 05:10:45PM +0800, Peng Fan wrote:
>Hi Heiko,
>
>On Mon, Mar 21, 2016 at 07:50:32AM +0100, Heiko Schocher wrote:
>>Hello Peng Fan,
>>
>>Sorry for the late reply ...
>>
>>Am 11.03.2016 um 09:47 schrieb Peng Fan:
>>>Implement i2c_idle_bus in driver, then setup_i2c can
>>>be dropped for boards which enable DM_I2C/DM_GPIO/PINCTRL.
>>>The i2c_idle_bus force bus idle flow follows setup_i2c in
>>>arch/arm/imx-common/i2c-mxv7.c
>>>
>>>This patch is an implementation following linux kernel patch:
>>>"
>>>commit 1c4b6c3bcf30d0804db0d0647d8ebeb862c6f7e5
>>>Author: Gao Pan <b54642 at freescale.com>
>>>Date:   Fri Oct 23 20:28:54 2015 +0800
>>>
>>>     i2c: imx: implement bus recovery
>>>
>>>     Implement bus recovery methods for i2c-imx so we can recover from
>>>     situations where SCL/SDA are stuck low.
>>>
>>>     Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
>>>     pinctrl to gpio mode by calling pinctrl sleep set function, and then
>>>     use GPIO to emulate the i2c protocol to send nine dummy clock to recover
>>>     i2c device. After recovery, set i2c pinctrl to default group setting.
>>>"
>>>
>>>See Documentation/devicetree/bindings/i2c/i2c-imx.txt for detailed
>>>description.
>>>1. Introuduce scl_gpio/sda_gpio/bus in mxc_i2c_bus.
>>>2. Discard the __weak attribute for i2c_idle_bus and implement it,
>>>    since we have pinctrl driver/driver model gpio driver. We can
>>>    use device tree, but not let board code to do this.
>>>3. gpio state for mxc_i2c is not a must, but it is recommended. If
>>>    there is no gpio state, driver will give tips, but not fail.
>>>4. The i2c controller was first probed, default pinctrl state will
>>>    be used, so when need to use gpio function, need to do
>>>    "pinctrl_select_state(dev, "gpio")" and after force bus idle,
>>>    need to switch back "pinctrl_select_state(dev, "default")".
>>>
>>>This is example about how to use the gpio force bus
>>>idle function:
>>>"
>>>  &i2c1 {
>>>  	clock-frequency = <100000>;
>>>	pinctrl-names = "default", "gpio";
>>>  	pinctrl-0 = <&pinctrl_i2c1>;
>>>	pinctrl-1 = <&pinctrl_i2c1_gpio>;
>>>	scl-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>>	sda-gpios = <&gpio1 29 GPIO_ACTIVE_HIGH>;
>>>	status = "okay";
>>>	[....]
>>>  };
>>>
>>>[.....]
>>>
>>>	pinctrl_i2c1_gpio: i2c1grp_gpio {
>>>		fsl,pins = <
>>>			MX6UL_PAD_UART4_TX_DATA__GPIO1_IO28 0x1b8b0
>>>			MX6UL_PAD_UART4_RX_DATA__GPIO1_IO29 0x1b8b0
>>>		>;
>>>	};
>>>"
>>>
>>>Signed-off-by: Peng Fan <van.freenix at gmail.com>
>>>Cc: Albert Aribaud <albert.u.boot at aribaud.net>
>>>Cc: Stefano Babic <sbabic at denx.de>
>>>Cc: Heiko Schocher <hs at denx.de>
>>>Cc: Simon Glass <sjg at chromium.org>
>>>Cc: York Sun <york.sun at nxp.com>
>>>---
>>>  arch/arm/include/asm/imx-common/mxc_i2c.h |  10 +++
>>>  drivers/i2c/mxc_i2c.c                     | 101 +++++++++++++++++++++++++++---
>>>  2 files changed, 102 insertions(+), 9 deletions(-)
>>
>>Thanks for this patch. In principle it looks very good ... I
>>have only a "nitpick" ... Couldn;t we split this patch into a
>>common piece, which does the deblocking of the bus, and a
>>"board/driver" specific part, where we do the pinmux changes?
>>
>>There is nothing "imx" special in the deblocking of the i2c
>>bus, beside switching the pinmux ... and as you use DM and DT
>>there is not even in your patch a imx special part ...
>>
>>So I tend to say, we can move this piece of code into a more
>>common place (drivers/i2c/i2c_core.c or into a new file i2c_deblock.c)
>>instead having it in the imx i2c driver ...
>>
>>Can you rework this?
>
>The idea of this patch is from Linux I2C patch for IMX:
>"
>commit 1c4b6c3bcf30d0804db0d0647d8ebeb862c6f7e5
>Author: Gao Pan <b54642 at freescale.com>
>Date:   Fri Oct 23 20:28:54 2015 +0800
>
>     i2c: imx: implement bus recovery
>
>     Implement bus recovery methods for i2c-imx so we can recover from
>     situations where SCL/SDA are stuck low.
>"
>
>so I would like to keep it in mxc i2c driver for now. When other i2c drivers
>have such requirement to force bus idle, then we can think of a new common
>way to support them.

Will you reject this patch or pick up this patch?

Thanks,
Peng.

>
>Thanks,
>Peng.
>
>>
>>Thanks a lot!
>>
>>bye,
>>Heiko
>>>
>
>[.....]