Re: [PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxx>
> Sent: Wednesday, July 26, 2023 10:12 PM
> To: Carlos Song <carlos.song@xxxxxxx>
> Cc: Aisheng Dong <aisheng.dong@xxxxxxx>; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; Clark
> Wang <xiaoning.wang@xxxxxxx>; Bough Chen <haibo.chen@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Carlos,
> 
> Quite a different patch this v2.
> 

Hi, Andi

hhh... yes, as you see, your advice for V1 guided me.
In i2c_init_recovery, I find the patch: “i2c: core: add generic I2C GPIO recovery“.
Because of it I found i2c driver hasn't needed so many explicit recovery information statements. 
It can help i2c driver to fill incomplete recovery information in i2c_init_recovery().
Based on this patch, any I2C bus drivers will be able to support GPIO recovery just by
providing a pointer to platform's pinctrl and calling i2c_recover_bus() when SDA is
stuck low.

So there are lots of redundant initialization lines in the V1 patch. I have to remove
them and remake the patch V2. 

> On Mon, Jul 24, 2023 at 06:55:45PM +0800, carlos.song@xxxxxxx wrote:
> > From: Carlos Song <carlos.song@xxxxxxx>
> >
> > Add bus recovery feature for LPI2C.
> > Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
> 
> mmhhh... I already asked you in the previous version to update the commit log...
> where is the DTS?
> 

Yes, I actually have got you advice in V1. The reason I keep it is that we hope i2c recovery function just be optical. 
In fact the commit log means:
We don’t use i2c recovery function as default. If you want use i2c recovery function, you should
add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
If you don't add it, it is ok. There is no any error log, of course i2c will not recovery.
(I have added a comment at lpi2c_imx_init_recovery_info())
So I keep itat V2. If there is no need to add it. I also support to remove it or fix it at V3.

> > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
> > Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> > ---
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 51
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index 158de0b7f030..e93ff3b5373c 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -107,6 +107,7 @@ struct lpi2c_imx_struct {
> >       unsigned int            txfifosize;
> >       unsigned int            rxfifosize;
> >       enum lpi2c_imx_mode     mode;
> > +     struct i2c_bus_recovery_info rinfo;
> 
> if this is in the i2c_adapter, why do you also need it here? You keep this place
> allocated even if it is optional.
> 

There is a i2c_bus_recovery_info pointer in i2c adapter, so I think I need to allocate
memory space for i2c_bus_recovery_info. How to choose this place allocated also
bother me. I'd also like to know your suggestion about it.

I tried two ways to that:
1. Define a global structure and assign values ​​to its members
+ static struct i2c_bus_recovery_info lpi2c_i2c_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+}
And in probe(){
+	lpi2c_imx->adapter.bus_recovery_info = &lpi2c_i2c_recovery_info;
}

I2c recovery function will be mandatory. If there is not a gpio-pinctrl configure in dts.
I2c will not output error log "Not using recovery: no {get|set}_scl() found".

That is not what we hope. We hope i2c recovery function is optional. If we do not configure
gpio-pinctrl in dts, it means that we don't use i2c recovery function and should not report an
error. It should be a conditional initialization. We hope check if there is a gpio-pinctrl 
configure in dts. If there is, i2c recovery info begin initialization(This means that i2c recovery
function is needed).

So I choose define i2c_bus_recovery_info in lpi2c_imx_struct(it looks concise and easy to use).
And define a function lpi2c_imx_init_recovery_info to check if i2c recovery info need to be initialized.

> >  };
> >
> >  static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx, @@
> > -134,6 +135,8 @@ static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct
> > *lpi2c_imx)
> >
> >               if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> >                       dev_dbg(&lpi2c_imx->adapter.dev, "bus not
> > work\n");
> > +                     if (lpi2c_imx->adapter.bus_recovery_info)
> > +                             i2c_recover_bus(&lpi2c_imx->adapter);
> 
> what is the recover_bus() function that will get called?

It is i2c_generic_scl_recovery. In i2c_init_recovery()-> i2c_gpio_init_generic_recovery(),
if generic GPIO recovery is available, will complete the incomplete recovery information.

> >                       return -ETIMEDOUT;
> >               }
> >               schedule();
> > @@ -191,6 +194,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct
> > *lpi2c_imx)
> >
> >               if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> >                       dev_dbg(&lpi2c_imx->adapter.dev, "stop
> > timeout\n");
> > +                     if (lpi2c_imx->adapter.bus_recovery_info)
> > +                             i2c_recover_bus(&lpi2c_imx->adapter);
> >                       break;
> >               }
> >               schedule();
> > @@ -323,6 +328,8 @@ static int lpi2c_imx_txfifo_empty(struct
> > lpi2c_imx_struct *lpi2c_imx)
> >
> >               if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> >                       dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty
> > timeout\n");
> > +                     if (lpi2c_imx->adapter.bus_recovery_info)
> > +                             i2c_recover_bus(&lpi2c_imx->adapter);
> >                       return -ETIMEDOUT;
> >               }
> >               schedule();
> > @@ -525,6 +532,44 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> >       return IRQ_HANDLED;
> >  }
> >
> > +/*
> > + * We switch SCL and SDA to their GPIO function and do some
> > +bitbanging
> > + * for bus recovery. These alternative pinmux settings can be
> > + * described in the device tree by a separate pinctrl state "gpio".
> > +If
> 
> What is the description in the device tree?
> 

The configure in dts when we need i2c recovery function:

@@ -410,9 +410,12 @@ &lpi2c1 {
- 		pinctrl-names = "default", "sleep";
+       pinctrl-names = "default", "sleep", "gpio";
+       pinctrl-2 = <&pinctrl_lpi2c1_gpio>;
+       scl-gpios = <&gpio1 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+       sda-gpios = <&gpio1 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
       status = "okay";
@@ -837,6 +840,13 @@ MX93_PAD_I2C1_SDA__LPI2C1_SDA                      0x40000b9e
                >;
        };

+       pinctrl_lpi2c1_gpio: lpi2c1gpiogrp {
+               fsl,pins = <
+                       MX93_PAD_I2C1_SCL__GPIO1_IO00                   0xb9e
+                       MX93_PAD_I2C1_SDA__GPIO1_IO01                   0xb9e
+               >;
+       };

> > + * this is missing this is not a big problem, the only implication is
> > + * that we can't do bus recovery.
> > + */
> > +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
> > +                               struct platform_device *pdev) {
> > +     struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo;
> > +
> > +     /*
> > +      * When there is no pinctrl state "gpio" in device tree, it means i2c
> > +      * recovery function is not needed, so it is not a problem even if
> > +      * pinctrl state "gpio" is missing. Just do not initialize the I2C bus
> > +      * recovery information.
> > +      */
> > +     rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +     if (IS_ERR(rinfo->pinctrl)) {
> > +             if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +             dev_info(&pdev->dev, "can't get pinctrl, bus recovery
> > + not supported\n");
> 
> nit: "can't get pinctrl..." sounds like error and this is not an error. Just "bus
> recovery not supported" is more a friendly message.
> 
ok, I will fix it at V3.
> > +             return PTR_ERR(rinfo->pinctrl);
> > +     } else if (!rinfo->pinctrl) {
> > +             return -ENODEV;
> 
> this is the unsupported case and here I would return '0' as it's not an error.
> 
I will fix it at V3.
> > +     }
> > +
> > +     if (IS_ERR(pinctrl_lookup_state(rinfo->pinctrl, "gpio"))) {
> > +             dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > +             return 0;
> > +     }
> > +
> > +     lpi2c_imx->adapter.bus_recovery_info = rinfo;
> > +
> > +     return 0;
> > +}
> > +
> >  static u32 lpi2c_imx_func(struct i2c_adapter *adapter)  {
> >       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | @@ -603,6
> +648,12 @@
> > static int lpi2c_imx_probe(struct platform_device *pdev)
> >       lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
> >       lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
> >
> > +     /* Init optional bus recovery function */
> > +     ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
> > +     /* Give it another chance if pinctrl used is not ready yet */
> > +     if (ret == -EPROBE_DEFER)
> > +             goto rpm_disable;
> 
> what about other errors like -ENOMEM?
> 

This judgment cannot cover all error conditions, I will fix it at V3:
+     /* Init optional bus recovery function */
+     ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
+     /* Give it another chance if pinctrl used is not ready yet */
+     if (ret)
+             goto rpm_disable;
Is this the judgment expected to be valid?
> Andi
> 
> >       ret = i2c_add_adapter(&lpi2c_imx->adapter);
> >       if (ret)
> >               goto rpm_disable;
> > 
Hope my excessive explanation didn't confuse you. Thanks!
--
> > 2.34.1
> >




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux