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]>
---
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
--- 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 Tue, Jul 25, 2023 at 10:11:08AM +0800, [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]>
> ---
> 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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi
> -----Original Message-----
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Tuesday, July 25, 2023 2:45 PM
> To: Carlos Song <[email protected]>
> Cc: [email protected]; [email protected]; Aisheng Dong
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Bough Chen <[email protected]>;
> [email protected]; Clark Wang <[email protected]>;
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> 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, [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]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
Date: Tue Jul 25 14:08:59 2023 +1000
Add linux-next specific files for 20230725
Signed-off-by: Stephen Rothwell <[email protected]>
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/ |