From: linux-i2c-owner@xxxxxxxxxxxxxxx <mailto:linux-i2c-owner@xxxxxxxxxxxxxxx> Sent: Wednesday, August 19, 2015 3:14 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611; kernel@xxxxxxxxxxxxxx > Subject: Re: [Patch v3] i2c: imx: implement bus recovery > > Hello, > > On Wed, Aug 19, 2015 at 03:04:01PM +0800, Gao Pan wrote: > > 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. > > > > Signed-off-by: Fugang Duan <B38611@xxxxxxxxxxxxx> > > Signed-off-by: Gao Pan <b54642@xxxxxxxxxxxxx> > > --- > > V2: > > As Uwe Kleine-König's suggestion, the version do below changes: > > -replace of_get_named_gpio() with devm_gpiod_get_optional() -move > > gpio_direction_input() and gpio_direction_output() call to the > > prepare callback > > -use 0 and 1 as return value for the get_scl and get_sda callbacks > > > > V3: > > -replace "...-gpio" with "...-gpios" in i2c binding doc -document > > the requirement of using sleep state to configure the pins > > as gpio in i2c binding doc > > -remove i2c_recover_bus() in i2c_imx_trx_complete() -use > > GPIOD_OUT_HIGH as the parameter of devm_gpiod_get_optional to config > > the gpios as output high > > -add error disposal as devm_gpiod_get_optional meets error. > > > > Documentation/devicetree/bindings/i2c/i2c-imx.txt | 8 ++ > > drivers/i2c/busses/i2c-imx.c | 100 > ++++++++++++++++++++-- > > 2 files changed, 101 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > index ce4311d..f778db0 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > @@ -14,6 +14,12 @@ Optional properties: > > The absence of the propoerty indicates the default frequency 100 kHz. > > - dmas: A list of two dma specifiers, one for each entry in dma-names. > > - dma-names: should contain "tx" and "rx". > > +- scl-gpios: specify the gpio related to SCL pin > > +- sda-gpios: specify the gpio related to SDA pin > > + > > +I2C bus recovery implementation note: > > +- If "scl-gpios" and "sda-gpios" property are present, pinctrl sleep > > +state > > + is used to configure the pins as gpio. > > > > Examples: > > > > @@ -37,4 +43,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */ > > dmas = <&edma0 0 50>, > > <&edma0 0 51>; > > dma-names = "rx","tx"; > > + scl-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH>; > > + sda-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>; > > }; > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index 4960fc1..59a6ffd 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -40,6 +40,7 @@ > > #include <linux/dmapool.h> > > #include <linux/err.h> > > #include <linux/errno.h> > > +#include <linux/gpio.h> > > #include <linux/i2c.h> > > #include <linux/init.h> > > #include <linux/interrupt.h> > > @@ -48,6 +49,7 @@ > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > +#include <linux/of_gpio.h> > This is not necessary any more, now, is it? Yes, you are right. Thanks. > > #include <linux/of_dma.h> > > #include <linux/platform_data/i2c-imx.h> #include > > <linux/platform_device.h> @@ -178,6 +180,11 @@ enum imx_i2c_type { > > VF610_I2C, > > }; > > > > +struct imx_i2c_pinctrl { > > + struct gpio_desc *sda; > > + struct gpio_desc *scl; > > +}; > > + > > struct imx_i2c_hwdata { > > enum imx_i2c_type devtype; > > unsigned regshift; > > @@ -209,6 +216,7 @@ struct imx_i2c_struct { > > unsigned int ifdr; /* IMX_I2C_IFDR */ > > unsigned int cur_clk; > > unsigned int bitrate; > > + struct imx_i2c_pinctrl pins; > > const struct imx_i2c_hwdata *hwdata; > > > > struct imx_i2c_dma *dma; > > @@ -439,7 +447,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct > *i2c_imx, int for_busy) > > if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) > { > > dev_dbg(&i2c_imx->adapter.dev, > > "<%s> I2C bus is busy\n", __func__); > > - return -ETIMEDOUT; > > + return i2c_recover_bus(&i2c_imx->adapter); > You dropped the other call to i2c_recover_bus, did you keep this one on > purpose? Yes, if the bus is still unavailable after specific time. It can be concluded that i2c bus is stuck. The i2c_recover_bus is carried out under the condition of "i2c_imx->pins.sda && i2c_imx->pins.scl". > > } > > schedule(); > > } > > @@ -963,6 +971,67 @@ out: > > return (result < 0) ? result : num; > > } > > > > +static int i2c_imx_get_scl(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + if (i2c_imx->pins.scl) > > + return gpiod_get_value(i2c_imx->pins.scl); > > If you implement my suggested change below you can drop the check and > call return gpiod_get_value(i2c_imx->pins.scl) unconditionally here. Thanks, you are right, will change it in next version. > > + return 1; > > +} > > + > > +static int i2c_imx_get_sda(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + if (i2c_imx->pins.sda) > > + return gpiod_get_value(i2c_imx->pins.sda); > > ditto Thanks. > > + > > + return 1; > > +} > > + > > +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + if (i2c_imx->pins.scl) > > + gpiod_set_value(i2c_imx->pins.scl, val); > > and also here Thanks. > > +} > > + > > +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + if (i2c_imx->pins.sda && i2c_imx->pins.scl) { > > + pinctrl_pm_select_sleep_state(&adap->dev); > > You wanted to document this. But maybe Linus Walleij want to say > something here. Thanks. > > > + gpiod_direction_input(i2c_imx->pins.sda); > > + gpiod_direction_output(i2c_imx->pins.scl, 1); > > + } > > +} > > + > > +static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + if (i2c_imx->pins.sda && i2c_imx->pins.scl) > > + pinctrl_pm_select_default_state(&adap->dev); > > +} > > + > > +static struct i2c_bus_recovery_info i2c_imx_bus_recovery_info = { > > + .get_scl = i2c_imx_get_scl, > > + .get_sda = i2c_imx_get_sda, > > + .set_scl = i2c_imx_set_scl, > > + .prepare_recovery = i2c_imx_prepare_recovery, > > + .unprepare_recovery = i2c_imx_unprepare_recovery, > > + .recover_bus = i2c_generic_scl_recovery, }; > > + > > static u32 i2c_imx_func(struct i2c_adapter *adapter) { > > return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL @@ -1011,12 +1080,13 @@ > > static int i2c_imx_probe(struct platform_device *pdev) > > > > /* Setup i2c_imx driver structure */ > > strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx- > >adapter.name)); > > - i2c_imx->adapter.owner = THIS_MODULE; > > - i2c_imx->adapter.algo = &i2c_imx_algo; > > - i2c_imx->adapter.dev.parent = &pdev->dev; > > - i2c_imx->adapter.nr = pdev->id; > > - i2c_imx->adapter.dev.of_node = pdev->dev.of_node; > > - i2c_imx->base = base; > > + i2c_imx->adapter.owner = THIS_MODULE; > > + i2c_imx->adapter.algo = &i2c_imx_algo; > > + i2c_imx->adapter.dev.parent = &pdev->dev; > > + i2c_imx->adapter.nr = pdev->id; > > + i2c_imx->adapter.dev.of_node = pdev->dev.of_node; > > + i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info; > > + i2c_imx->base = base; > > Maybe: > > - i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info; > + if (i2c_imx->pins.sda && i2c_imx->pins.scl) > + i2c_imx->adapter.bus_recovery_info = > &i2c_imx_bus_recovery_info; > > (This check obviously needs to be done further down when sda and scl are > setup.) Thanks. If i2c_recover_bus() is called under the condition of "i2c_imx->pins.sda && i2c_imx->pins.scl", the check here can be removed. How do you think? > > /* Get I2C clock */ > > i2c_imx->clk = devm_clk_get(&pdev->dev, NULL); @@ -1052,6 +1122,22 > > @@ static int i2c_imx_probe(struct platform_device *pdev) > > pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > > pm_runtime_use_autosuspend(&pdev->dev); > > > > + /* Init recover pins */ > > + i2c_imx->pins.sda = > > + devm_gpiod_get_optional(&pdev->dev, "sda-gpio", > GPIOD_OUT_HIGH); > > + i2c_imx->pins.scl = > > + devm_gpiod_get_optional(&pdev->dev, "scl-gpio", > GPIOD_OUT_HIGH); > As already noticed in the v2 thread: I think GPIOD_OUT_HIGH is wrong. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
