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

[Uwe Kleine-König]

On Tue, Jul 25, 2023 at 10:11:08AM +0800, carlos.song@xxxxxxx wrote:
> From: Gao Pan <pandy.gao@xxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> ---
> 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/ |

Attachment: signature.asc
Description: PGP signature