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

Peng Fan
2016-03-21T09:10:45Z

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@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@gmail.com>
>>Cc: Albert Aribaud <albert.u.boot@aribaud.net>
>>Cc: Stefano Babic <sbabic@denx.de>
>>Cc: Heiko Schocher <hs@denx.de>
>>Cc: Simon Glass <sjg@chromium.org>
>>Cc: York Sun <york.sun@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@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
>>

[.....]