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

Peng Fan
2016-03-25T09:17:59Z

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@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.

Will you reject this patch or pick up this patch?

Thanks,
Peng.

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