Re: [PATCH] i2c-hix5hd2: Add more debug info when transfer fail.

[Andi Shyti]

Hi Taolan,

On Fri, Sep 30, 2022 at 01:44:05AM +0000, Tao Lan wrote:
> From: taolan <taolan@xxxxxxxxxx>
> 
> The transfer result is checked, classified, and printed, which
> facilitates debugging.

you are missing your Signed-off-by here.

> ---
>  drivers/i2c/busses/i2c-hix5hd2.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-hix5hd2.c b/drivers/i2c/busses/i2c-hix5hd2.c
> index 61ae58f57047..e6ab8b20cd01 100644
> --- a/drivers/i2c/busses/i2c-hix5hd2.c
> +++ b/drivers/i2c/busses/i2c-hix5hd2.c
> @@ -366,8 +366,17 @@ static int hix5hd2_i2c_xfer(struct i2c_adapter *adap,
>  			goto out;
>  	}
>  
> -	ret = num;
> +	if (i == num) {
> +		ret = num;
> +	} else {

it can't be, because if you are exiting normally from the for
loop, then i is equal to num.

You might want to put this inside the out label, or, even better:

                    stop = (i == num - 1);
                    ret = hix5hd2_i2c_xfer_msg(priv, msgs, stop);
                    if (ret < 0)
    -                       goto out;
    +                       break;
            }
     
    -       ret = num;
    +       if (i == num) {
    +               ret = num;
    +       } else {
    +               /* Only one message, cannot access the device */
                    ...
    +       }
     
    -out:

What do you think?

> +		/* Only one message, cannot access the device */
> +		if (i == 1)
> +			ret = -EREMOTEIO;
> +		else
> +			ret = i;
>  
> +		pr_warning("xfer message failed\n");

please use dev_warn here.

BTW, is it warning or error?

Andi

> +	}
>  out:
>  	pm_runtime_mark_last_busy(priv->dev);
>  	pm_runtime_put_autosuspend(priv->dev);
> -- 
> 2.22.0
>