[PATCH v2] drm/amdgpu: add return result for amdgpu_i2c_{get/put}_byte

[Christian König]

Am 24.04.24 um 09:52 schrieb Bob Zhou:
> After amdgpu_i2c_get_byte fail, amdgpu_i2c_put_byte shouldn't be
> conducted to put wrong value.
> So return and check the i2c transfer result.
>
> Signed-off-by: Bob Zhou <bob.zhou at amd.com>

Looks good in general, just some coding style comments below.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c | 42 +++++++++++++++----------
>   1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
> index 82608df43396..c588704d56a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
> @@ -280,11 +280,12 @@ amdgpu_i2c_lookup(struct amdgpu_device *adev,
>   	return NULL;
>   }
>   
> -static void amdgpu_i2c_get_byte(struct amdgpu_i2c_chan *i2c_bus,
> +static int amdgpu_i2c_get_byte(struct amdgpu_i2c_chan *i2c_bus,
>   				 u8 slave_addr,
>   				 u8 addr,
>   				 u8 *val)
>   {
> +	int r = 0;

BTW: Short variables like i and r should be declared last. I don't care 
much about that personally, but some upstream maintainers insist on that.

And never initialize a variable if you don't need it. This will be 
complained about by automated checkers as well.

>   	u8 out_buf[2];
>   	u8 in_buf[2];
>   	struct i2c_msg msgs[] = {
> @@ -309,16 +310,18 @@ static void amdgpu_i2c_get_byte(struct amdgpu_i2c_chan *i2c_bus,
>   		*val = in_buf[0];
>   		DRM_DEBUG("val = 0x%02x\n", *val);
>   	} else {
> -		DRM_DEBUG("i2c 0x%02x 0x%02x read failed\n",
> -			  addr, *val);
> +		r = -EIO;
> +		DRM_DEBUG("i2c 0x%02x 0x%02x read failed\n", addr, *val);
>   	}
> +	return r;

Better to write it like this:

if (error_condition) {
     DRM_DEBUG("i2c 0x%02x read failed\n", addr);
     return -EIO)
}

*val = in_buf[0];
DRM_DEBUG("val = 0x%02x\n", *val);

Printing *val in the error condition will result in use of uninitialized 
value as well.

>   }
>   
> -static void amdgpu_i2c_put_byte(struct amdgpu_i2c_chan *i2c_bus,
> +static int amdgpu_i2c_put_byte(struct amdgpu_i2c_chan *i2c_bus,
>   				 u8 slave_addr,
>   				 u8 addr,
>   				 u8 val)
>   {
> +	int r = 0;
>   	uint8_t out_buf[2];
>   	struct i2c_msg msg = {
>   		.addr = slave_addr,
> @@ -330,9 +333,12 @@ static void amdgpu_i2c_put_byte(struct amdgpu_i2c_chan *i2c_bus,
>   	out_buf[0] = addr;
>   	out_buf[1] = val;
>   
> -	if (i2c_transfer(&i2c_bus->adapter, &msg, 1) != 1)
> -		DRM_DEBUG("i2c 0x%02x 0x%02x write failed\n",
> -			  addr, val);
> +	if (i2c_transfer(&i2c_bus->adapter, &msg, 1) != 1) {
> +		r = -EIO;
> +		DRM_DEBUG("i2c 0x%02x 0x%02x write failed\n", addr, val);
> +	}
> +
> +	return r;

Same here. As long as you don't have anything to cleanup just use 
"return -EIO" and "return 0;" directly.

Regards,
Christian.

>   }
>   
>   /* ddc router switching */
> @@ -347,16 +353,18 @@ amdgpu_i2c_router_select_ddc_port(const struct amdgpu_connector *amdgpu_connecto
>   	if (!amdgpu_connector->router_bus)
>   		return;
>   
> -	amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
> +	if (amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
> -			    0x3, &val);
> +			    0x3, &val))
> +		return;
>   	val &= ~amdgpu_connector->router.ddc_mux_control_pin;
>   	amdgpu_i2c_put_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
>   			    0x3, val);
> -	amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
> +	if (amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
> -			    0x1, &val);
> +			    0x1, &val))
> +		return;
>   	val &= ~amdgpu_connector->router.ddc_mux_control_pin;
>   	val |= amdgpu_connector->router.ddc_mux_state;
>   	amdgpu_i2c_put_byte(amdgpu_connector->router_bus,
> @@ -368,7 +376,7 @@ amdgpu_i2c_router_select_ddc_port(const struct amdgpu_connector *amdgpu_connecto
>   void
>   amdgpu_i2c_router_select_cd_port(const struct amdgpu_connector *amdgpu_connector)
>   {
> -	u8 val;
> +	u8 val = 0;
>   
>   	if (!amdgpu_connector->router.cd_valid)
>   		return;
> @@ -376,16 +384,18 @@ amdgpu_i2c_router_select_cd_port(const struct amdgpu_connector *amdgpu_connector
>   	if (!amdgpu_connector->router_bus)
>   		return;
>   
> -	amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
> +	if (amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
> -			    0x3, &val);
> +			    0x3, &val))
> +		return;
>   	val &= ~amdgpu_connector->router.cd_mux_control_pin;
>   	amdgpu_i2c_put_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
>   			    0x3, val);
> -	amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
> +	if (amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
>   			    amdgpu_connector->router.i2c_addr,
> -			    0x1, &val);
> +			    0x1, &val))
> +		return;
>   	val &= ~amdgpu_connector->router.cd_mux_control_pin;
>   	val |= amdgpu_connector->router.cd_mux_state;
>   	amdgpu_i2c_put_byte(amdgpu_connector->router_bus,