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

[Peng Fan]

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.

Thanks,
Peng.

>
>Thanks a lot!
>
>bye,
>Heiko
>>

[.....]