From: Gao Pan <[email protected]>
A NACK flag in ISR means i2c bus error. In such codition,
there is no need to do read/write operation. It's better
to return ISR directly and then stop i2c transfer.
Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Gao Pan <[email protected]>
Signed-off-by: Carlos Song <[email protected]>
---
drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c3287c887c6f..158de0b7f030 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
temp = readl(lpi2c_imx->base + LPI2C_MSR);
temp &= enabled;
- if (temp & MSR_RDF)
+ if (temp & MSR_NDF) {
+ complete(&lpi2c_imx->complete);
+ return IRQ_HANDLED;
+ } else if (temp & MSR_RDF)
lpi2c_imx_read_rxfifo(lpi2c_imx);
-
- if (temp & MSR_TDF)
+ else if (temp & MSR_TDF)
lpi2c_imx_write_txfifo(lpi2c_imx);
- if (temp & MSR_NDF)
- complete(&lpi2c_imx->complete);
-
return IRQ_HANDLED;
}
--
2.34.1
From: Carlos Song <[email protected]>
Add bus recovery feature for LPI2C.
Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
Signed-off-by: Clark Wang <[email protected]>
Signed-off-by: Carlos Song <[email protected]>
---
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;
};
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;
--
2.34.1
From: Gao Pan <[email protected]>
Output error log when i2c peripheral clk rate is 0, then
directly return -EINVAL.
Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Gao Pan <[email protected]>
Signed-off-by: Carlos Song <[email protected]>
---
drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e93ff3b5373c..12b4f2a89343 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
lpi2c_imx_set_mode(lpi2c_imx);
clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
+ if (!clk_rate) {
+ dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");
+ return -EINVAL;
+ }
+
if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
filt = 0;
else
--
2.34.1
On Mon, Jul 24, 2023 at 7:52 AM <[email protected]> wrote:
>
> From: Gao Pan <[email protected]>
>
> Output error log when i2c peripheral clk rate is 0, then
> directly return -EINVAL.
>
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <[email protected]>
> Signed-off-by: Carlos Song <[email protected]>
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e93ff3b5373c..12b4f2a89343 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> lpi2c_imx_set_mode(lpi2c_imx);
>
> clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> + if (!clk_rate) {
> + dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");
The subject says 'debug message', but this is an error message.
Please adjust the Subject.
Thanks! I will adjust it and send V3 patch.
> -----Original Message-----
> From: Fabio Estevam <[email protected]>
> Sent: Monday, July 24, 2023 8:38 PM
> To: Carlos Song <[email protected]>
> Cc: [email protected]; Aisheng Dong <[email protected]>;
> [email protected]; [email protected]; [email protected];
> Clark Wang <[email protected]>; Bough Chen <[email protected]>;
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH v2 3/3] i2c: imx-lpi2c: add debug message when i2c
> peripheral clk doesn't work
>
> 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
>
>
> On Mon, Jul 24, 2023 at 7:52 AM <[email protected]> wrote:
> >
> > From: Gao Pan <[email protected]>
> >
> > Output error log when i2c peripheral clk rate is 0, then directly
> > return -EINVAL.
> >
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Gao Pan <[email protected]>
> > Signed-off-by: Carlos Song <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index e93ff3b5373c..12b4f2a89343 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct
> *lpi2c_imx)
> > lpi2c_imx_set_mode(lpi2c_imx);
> >
> > clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> > + if (!clk_rate) {
> > + dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is
> > + 0\n");
>
> The subject says 'debug message', but this is an error message.
>
> Please adjust the Subject.
Hi Carlos,
On Mon, Jul 24, 2023 at 06:55:44PM +0800, [email protected] wrote:
> From: Gao Pan <[email protected]>
>
> A NACK flag in ISR means i2c bus error. In such codition,
> there is no need to do read/write operation. It's better
> to return ISR directly and then stop i2c transfer.
>
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <[email protected]>
> Signed-off-by: Carlos Song <[email protected]>
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index c3287c887c6f..158de0b7f030 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> temp = readl(lpi2c_imx->base + LPI2C_MSR);
> temp &= enabled;
>
> - if (temp & MSR_RDF)
> + if (temp & MSR_NDF) {
> + complete(&lpi2c_imx->complete);
> + return IRQ_HANDLED;
you can actually remove the return here
if (temp & MSR_NDF)
complete();
else if (temp & MSR_RDF)
exfifo();
else if (temp & MSR_TDF)
txfifo();
return IRQ_HANDLED;
BTW, the logic here is changing, as well and it's not described
in the commit log. This patch is not only stopping when a nack is
received (MSR_NDF), but it's also making mutually exclusive
read/write (which I guess are MSR_RDF and MSR_TDF).
Is this what you want? If so, can you please describe it in the
commit log or add a comment describing that the three states are
all mutually exclusive.
Thanks,
Andi
> + } else if (temp & MSR_RDF)
> lpi2c_imx_read_rxfifo(lpi2c_imx);
> -
> - if (temp & MSR_TDF)
> + else if (temp & MSR_TDF)
> lpi2c_imx_write_txfifo(lpi2c_imx);
>
> - if (temp & MSR_NDF)
> - complete(&lpi2c_imx->complete);
> -
> return IRQ_HANDLED;
> }
>
> --
> 2.34.1
>
Hi, Andi
> -----Original Message-----
> From: Andi Shyti <[email protected]>
> Sent: Wednesday, July 26, 2023 7:55 AM
> To: Carlos Song <[email protected]>
> Cc: Aisheng Dong <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Clark
> Wang <[email protected]>; Bough Chen <[email protected]>;
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect
> a NACK
>
> 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,
>
> On Mon, Jul 24, 2023 at 06:55:44PM +0800, [email protected] wrote:
> > From: Gao Pan <[email protected]>
> >
> > A NACK flag in ISR means i2c bus error. In such codition, there is no
> > need to do read/write operation. It's better to return ISR directly
> > and then stop i2c transfer.
> >
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Gao Pan <[email protected]>
> > Signed-off-by: Carlos Song <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index c3287c887c6f..158de0b7f030 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void
> *dev_id)
> > temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > temp &= enabled;
> >
> > - if (temp & MSR_RDF)
> > + if (temp & MSR_NDF) {
> > + complete(&lpi2c_imx->complete);
> > + return IRQ_HANDLED;
>
> you can actually remove the return here
>
> if (temp & MSR_NDF)
> complete();
> else if (temp & MSR_RDF)
> exfifo();
> else if (temp & MSR_TDF)
> txfifo();
>
> return IRQ_HANDLED;
>
>
> BTW, the logic here is changing, as well and it's not described in the commit log.
> This patch is not only stopping when a nack is received (MSR_NDF), but it's also
> making mutually exclusive read/write (which I guess are MSR_RDF and
> MSR_TDF).
>
> Is this what you want? If so, can you please describe it in the commit log or add
> a comment describing that the three states are all mutually exclusive.
>
Yes. That is ok. I will fix the commit log and send V3 patch.
> Thanks,
> Andi
>
>
> > + } else if (temp & MSR_RDF)
> > lpi2c_imx_read_rxfifo(lpi2c_imx);
> > -
> > - if (temp & MSR_TDF)
> > + else if (temp & MSR_TDF)
> > lpi2c_imx_write_txfifo(lpi2c_imx);
> >
> > - if (temp & MSR_NDF)
> > - complete(&lpi2c_imx->complete);
> > -
> > return IRQ_HANDLED;
> > }
> >
> > --
> > 2.34.1
> >
Hi Carlos,
Quite a different patch this v2.
On Mon, Jul 24, 2023 at 06:55:45PM +0800, [email protected] wrote:
> From: Carlos Song <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Carlos Song <[email protected]>
> ---
> 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 <[email protected]>
> Sent: Wednesday, July 26, 2023 10:12 PM
> To: Carlos Song <[email protected]>
> Cc: Aisheng Dong <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Clark
> Wang <[email protected]>; Bough Chen <[email protected]>;
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]
> 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, [email protected] wrote:
> > From: Carlos Song <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Carlos Song <[email protected]>
> > ---
> > 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
> >