diff mbox series

[v3] i2c: imx-lpi2c: add error message when i2c peripheral clk doesn't work

Message ID 20230725021108.1977494-1-carlos.song@nxp.com
State Changes Requested
Headers show
Series [v3] i2c: imx-lpi2c: add error message when i2c peripheral clk doesn't work | expand

Commit Message

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

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 <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"
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Uwe Kleine-König July 25, 2023, 6:44 a.m. UTC | #1 
On Tue, Jul 25, 2023 at 10:11:08AM +0800, carlos.song@nxp.com wrote:
> From: Gao Pan <pandy.gao@nxp.com>
> 
> 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 <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"
> ---
>  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

I wonder which tree you based this on. The object e93ff3b5373c is
neither in v6.5-rc1 nor next nor in Wolfram's tree. (Probably not
critical for this patch. But if you base your patch on a public tree,
you're making things easier for the build bots. Related to that I
recommend git format-patch --base=...)

> --- 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;
> +	}

I'm not sure this is a good idea. In my book error messages in functions
that might be called often (and triggered by user space) do more harm
than good as they flood the kernel log buffer and so might make it hard
to find the original cause of the problem. I'd say returning an error
code should be enough here.

Having said that I wonder if you're fixing a real issue? (i.e. did it
happen to you that clk_rate was 0? Did this result in a div-by-zero? Or
a hardware hang? Or a too fast (or too slow) bus speed?) Or is this just
addressing a warning by some static checker?

Best regards
Uwe
Carlos Song July 25, 2023, 8:24 a.m. UTC | #2 
Hi

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Tuesday, July 25, 2023 2:45 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: festevam@gmail.com; andi.shyti@kernel.org; Aisheng Dong
> <aisheng.dong@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; Bough Chen <haibo.chen@nxp.com>;
> linux-kernel@vger.kernel.org; Clark Wang <xiaoning.wang@nxp.com>;
> linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v3] i2c: imx-lpi2c: add error message when i2c
> peripheral clk doesn't work
> 
> On Tue, Jul 25, 2023 at 10:11:08AM +0800, carlos.song@nxp.com wrote:
> > From: Gao Pan <pandy.gao@nxp.com>
> >
> > 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 <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"
> > ---
> >  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
> 
> I wonder which tree you based this on. The object e93ff3b5373c is neither in
> v6.5-rc1 nor next nor in Wolfram's tree. (Probably not critical for this patch. But
> if you base your patch on a public tree, you're making things easier for the build
> bots. Related to that I recommend git format-patch --base=...)
> 

I make the patch on linux-next master branch at:
commit 4d2c646ac07cf4a35ef1c4a935a1a4fd6c6b1a36 (tag: next-20230724, origin/master, origin/HEAD)
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon Jul 24 14:07:27 2023 +1000

Just now I git fetch and rebase on the linux-next, now I am on master branch at:
commit 1e25dd7772483f477f79986d956028e9f47f990a (tag: next-20230725, origin/master, origin/HEAD)
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Tue Jul 25 14:08:59 2023 +1000

    Add linux-next specific files for 20230725

    Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
I can still git am the patch.

> > --- 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;
> > +	}
> 
> I'm not sure this is a good idea. In my book error messages in functions that
> might be called often (and triggered by user space) do more harm than good as
> they flood the kernel log buffer and so might make it hard to find the original
> cause of the problem. I'd say returning an error code should be enough here.
> 
> Having said that I wonder if you're fixing a real issue? (i.e. did it happen to you
> that clk_rate was 0? Did this result in a div-by-zero? Or a hardware hang? Or a
> too fast (or too slow) bus speed?) Or is this just addressing a warning by some
> static checker?
> 

Thanks for you suggestion. I will rm the line " dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");"
And resend the patch.

The per_clk is a critical i2c function clk. If it is 0, it will cause lpi2c can't work. This is a double check for lpi2c clk before configing clk register. It can improve the stability of the driver, even though it rarely triggers.
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
diff mbox series

Patch

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