diff mbox series

[v4] i2c: imx-lpi2c: return -EINVAL when i2c peripheral clk doesn't work

Message ID 20230725083117.2745327-1-carlos.song@nxp.com
State Superseded
Headers show
Series [v4] i2c: imx-lpi2c: return -EINVAL when i2c peripheral clk doesn't work | expand

Commit Message

Carlos Song July 25, 2023, 8:31 a.m. UTC 
From: Gao Pan <pandy.gao@nxp.com>

When i2c peripheral clk rate is 0 directly return -EINVAL.

Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- adjust the subject from "debug message" to "error message"
Changes for V3:
- remove output error log when i2c peripheral clk doesn't work
- adjust commit log and subject
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Andi Shyti July 25, 2023, 11:41 p.m. UTC | #1 
Hi Carlos,

> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -209,6 +209,9 @@ 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)
> +		return -EINVAL;
> +

this is a very unlikely to happen and generally not really
appreciated.

If you got so far it's basically impossible that clk_rate is '0'.
Uwe asked you in v2 if you actually had such case.

I don't have a strong opinion, thoug... I would drop this patch
unless Dong is OK with it and I can accept it with his ack.

Andi

>  	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
>  		filt = 0;
>  	else
> -- 
> 2.34.1
>
Carlos Song July 26, 2023, 7:32 a.m. UTC | #2 
Hi, Andi

According to Aisheng, I find that if the i2c clock rate is not assigned on MX8,
due to chip architecture, the default speed will be 0, it cause our i2c not work.
The judgment will be triggered easily. This is a special case for imx.

The log like this:
[    2.401402] imx-lpi2c 5a800000.i2c: use pio mode
[    2.419788] i2c i2c-4: clk_per rate is 0
[    2.423724] i2c i2c-4: clk_per rate is 0
[    2.444071] i2c i2c-4: clk_per rate is 0
[    2.448011] fxos8700_i2c 4-001e: Error reading chip id
[    2.453172] fxos8700_i2c: probe of 4-001e failed with error -22
[    2.459271] i2c i2c-4: supply vdd not found, using dummy regulator
[    2.465522] i2c i2c-4: supply vddio not found, using dummy regulator
[    2.471913] i2c i2c-4: clk_per rate is 0
[    2.475867] fxas21002c_i2c: probe of 4-0020 failed with error -22
[    2.482066] i2c i2c-4: clk_per rate is 0
[    2.495716] i2c i2c-4: clk_per rate is 0
[    2.505464] i2c i2c-4: clk_per rate is 0
[    2.514786] i2c i2c-4: LPI2C adapter registered

So the patch can not be dropped.
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Wednesday, July 26, 2023 7:41 AM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: u.kleine-koenig@pengutronix.de; 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 v4] i2c: imx-lpi2c: return -EINVAL 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
> 
> 
> Hi Carlos,
> 
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -209,6 +209,9 @@ 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)
> > +             return -EINVAL;
> > +
> 
> this is a very unlikely to happen and generally not really appreciated.
> 
> If you got so far it's basically impossible that clk_rate is '0'.
> Uwe asked you in v2 if you actually had such case.
> 
> I don't have a strong opinion, thoug... I would drop this patch unless Dong is OK
> with it and I can accept it with his ack.
> 
> Andi
> 
> >       if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
> >               filt = 0;
> >       else
> > --
> > 2.34.1
> >
Aisheng Dong July 26, 2023, 7:59 a.m. UTC | #3 
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: 2023年7月26日 7:41
> Hi Carlos,
> 
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -209,6 +209,9 @@ 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)
> > +		return -EINVAL;
> > +
> 
> this is a very unlikely to happen and generally not really appreciated.
> 
> If you got so far it's basically impossible that clk_rate is '0'.
> Uwe asked you in v2 if you actually had such case.
> 
> I don't have a strong opinion, thoug... I would drop this patch unless Dong is
> OK with it and I can accept it with his ack.

On MX8X platforms, the default clock rate is 0 if without explicit clock setting in
dts nodes. So I wonder it may be worth adding a check here.
If you're also ok, feel free to add my tag.
Acked-by: Dong Aisheng <Aisheng.dong@nxp.com>

BTW, please see another reply from Carlos with the test log.

Regards
Aisheng

> 
> Andi
> 
> >  	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
> >  		filt = 0;
> >  	else
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c3287c887c6f..150d923ca7f1 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -209,6 +209,9 @@  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)
+		return -EINVAL;
+
 	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
 		filt = 0;
 	else