From: Uwe Kleine-König <mailto:u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Tuesday, July 14, 2015 2:49 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611 > Subject: Re: [Patch v1] i2c: imx: implement bus recovery > > Hello Gao, > > On Tue, Jul 14, 2015 at 10:04:46AM +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> > > --- > > Documentation/devicetree/bindings/i2c/i2c-imx.txt | 4 + > > drivers/i2c/busses/i2c-imx.c | 102 > +++++++++++++++++++++- > > 2 files changed, 105 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > index ce4311d..0a0848f5 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > @@ -14,6 +14,8 @@ 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". > > +- recover-scl: specify the gpio related to SCL pin > > +- recover-sda: specify the gpio related to SDA pin > I don't like the naming here. That the gpios are used for recovery isn't > a hardware description. What about "scl-gpio" and "sda-gpio"? Thanks. I will change it in the next version. > > Examples: > > > > @@ -37,4 +39,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */ > > dmas = <&edma0 0 50>, > > <&edma0 0 51>; > > dma-names = "rx","tx"; > > + recover-scl = <&gpio5 26 0>; > > + recover-sda = <&gpio5 27 0>; > > }; > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index 785aa67..eb4f95c 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> > > #include <linux/of_dma.h> > > #include <linux/platform_data/i2c-imx.h> #include > > <linux/platform_device.h> @@ -175,6 +177,16 @@ enum imx_i2c_type { > > VF610_I2C, > > }; > > > > +enum imx_i2c_vol_level { > > + I2C_IMX_LOW_VOL_LEVEL, > > + I2C_IMX_HIGH_VOL_LEVEL, > > +}; > These are used as return value for the get_scl and get_sda callbacks > which should return an int. I'd use plain 0 and 1 here. Thanks. It can improve the readability to use plain 0 and 1 here. > > +struct imx_i2c_pinctrl { > > + unsigned int sda_pin; > > + unsigned int scl_pin; > > +}; > > + > > struct imx_i2c_hwdata { > > enum imx_i2c_type devtype; > > unsigned regshift; > > @@ -206,6 +218,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; > > @@ -436,7 +449,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); > > This looks wrong. What if the bus is still busy after the call to > i2c_recover_bus? Also it doesn't feel right to call i2c_recover_bus just > because the bus is busy. In there, maybe i2c device sda is pulled low and bus is dead, so I add the bus recovery in here. Because there wait 500ms, it is enough long. If the bus is still busy after recovery, it return the -EBUSY, and gpio also cannot recover the bus, so this transfer is failed. What do you suggest for the position to call i2c_recover_bus() ? Thanks. > > } > > schedule(); > > } > > @@ -450,6 +463,7 @@ static int i2c_imx_trx_complete(struct > > imx_i2c_struct *i2c_imx) > > > > if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) { > > dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__); > > + i2c_recover_bus(&i2c_imx->adapter); > > return -ETIMEDOUT; > ditto. The same as above. But it is better: return i2c_recover_bus(&i2c_imx->adapter); > > } > > dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__); @@ > > -956,6 +970,67 @@ fail0: > > 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.sda_pin && i2c_imx->pins.scl_pin) > > + return gpio_get_value(i2c_imx->pins.scl_pin); > > + > > + return I2C_IMX_HIGH_VOL_LEVEL; > > +} > > + > > +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_pin && i2c_imx->pins.scl_pin) { > > + gpio_direction_input(i2c_imx->pins.sda_pin); > Don't you want that call in the prepare callback? Thanks for your precise review. I will change it in the next version. > > + return gpio_get_value(i2c_imx->pins.sda_pin); > > + } > > + > > + return I2C_IMX_HIGH_VOL_LEVEL; > > +} > > + > > +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.sda_pin && i2c_imx->pins.scl_pin) > > + gpio_direction_output(i2c_imx->pins.scl_pin, val); > Please move the gpio_direction_output call to the prepare callback and > only use gpio_set_value here. Thank you, will change it in next version. > > +} > > + > > +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_pin && i2c_imx->pins.scl_pin) > > + pinctrl_pm_select_sleep_state(&adap->dev); > So the sleep state is expected to mux the pins into their gpio function? Yes, you are right. In here, we expect the pad is muxed to gpio function. > > + > spare empty line Thanks. > > > +} > > + > > +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_pin && i2c_imx->pins.scl_pin) > > + 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, > Mixed tab and spaces used for indention (if you ask me, use a single > space before the '=' ... Thanks for your carefulness, I will correct it in the next version. > > +}; > > + > > static u32 i2c_imx_func(struct i2c_adapter *adapter) { > > return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL @@ -1009,6 +1084,7 @@ > > static int i2c_imx_probe(struct platform_device *pdev) > > 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; > ... which prevents uglyness as introduce here.) Thanks. > > > > /* Get I2C clock */ > > @@ -1037,6 +1113,30 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > /* Set up adapter data */ > > i2c_set_adapdata(&i2c_imx->adapter, i2c_imx); > > > > + /* Init recover pins */ > > + i2c_imx->pins.sda_pin = > > + of_get_named_gpio(pdev->dev.of_node, "recover-sda", 0); > > + i2c_imx->pins.scl_pin = > > + of_get_named_gpio(pdev->dev.of_node, "recover-scl", 0); > I'd use devm_gpiod_get_optional here. Thanks for your good suggestion. I will change it in the next version. -- 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
