| Message ID | 20230724105546.1964059-2-carlos.song@nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/3] i2c: imx-lpi2c: directly return ISR when detect a NACK | expand |
Hi Carlos, Quite a different patch this v2. On Mon, Jul 24, 2023 at 06:55:45PM +0800, carlos.song@nxp.com wrote: > From: Carlos Song <carlos.song@nxp.com> > > 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? > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Carlos Song <carlos.song@nxp.com> > --- > 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. > }; > > 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? > 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? > + * 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. > + 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. > + } > + > + 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? Andi > ret = i2c_add_adapter(&lpi2c_imx->adapter); > if (ret) > goto rpm_disable; > -- > 2.34.1 >
> -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Wednesday, July 26, 2023 10:12 PM > To: Carlos Song <carlos.song@nxp.com> > Cc: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; Clark > Wang <xiaoning.wang@nxp.com>; Bough Chen <haibo.chen@nxp.com>; > dl-linux-imx <linux-imx@nxp.com>; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > 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@nxp.com wrote: > > From: Carlos Song <carlos.song@nxp.com> > > > > 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@nxp.com> > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > > --- > > 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 > >
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; }; 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); 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 + * 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"); + return PTR_ERR(rinfo->pinctrl); + } else if (!rinfo->pinctrl) { + return -ENODEV; + } + + 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; + ret = i2c_add_adapter(&lpi2c_imx->adapter); if (ret) goto rpm_disable;