From: [email protected] <mailto:[email protected]>
Sent: Wednesday, August 19, 2015 3:14 PM
> To: Gao Pan-B54642
> Cc: [email protected]; [email protected]; Li Frank-B20596; Duan
> Fugang-B38611; [email protected]
> 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 <[email protected]>
> > Signed-off-by: Gao Pan <[email protected]>
> > ---
> > 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
