Re: [PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature
From: Carlos Song
Date: Fri Jul 28 2023 - 05:48:50 EST
> -----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
> >