Re: [RESEND PATCH] media: staging: atomisp: rework reading the id and revision values

[Sakari Ailus]

Hi Tom,

On Tue, Mar 29, 2022 at 04:25:26PM -0700, trix@xxxxxxxxxx wrote:
> From: Tom Rix <trix@xxxxxxxxxx>
> 
> Clang static analysis reports this representative issue
> atomisp-ov2722.c:920:3: warning: 3rd function call
>   argument is an uninitialized value
>   dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> high and low are only set when ov2722_read_reg() is successful.
> Reporting the high value when there is an error is not
> meaningful.  The later read for low is not checked.  high
> and low are or-ed together and checked against a non zero
> value.
> 
> Remove the unneeded error reporting for high.  Initialize
> high and low to 0 and use the id check to determine if
> the reads were successful
> 
> The later read for revision is not checked.  If it
> fails the old high value will be used and the revision
> will be misreported.
> 
> Since the revision is only reported and not checked or
> stored it is not necessary to return if the read with
> successful.  This makes the ret variable unnecessary
> so remove it.
> 
> Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
> ---
>  .../media/atomisp/i2c/atomisp-ov2722.c        | 20 ++++++++-----------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index da98094d7094..d5d099ac1b70 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -906,22 +906,17 @@ static int ov2722_get_fmt(struct v4l2_subdev *sd,
>  static int ov2722_detect(struct i2c_client *client)
>  {
>  	struct i2c_adapter *adapter = client->adapter;
> -	u16 high, low;
> -	int ret;
> +	u16 high = 0, low = 0;
>  	u16 id;
>  	u8 revision;
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>  		return -ENODEV;
>  
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_CHIP_ID_H, &high);
> -	if (ret) {
> -		dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
> -		return -ENODEV;
> -	}
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_CHIP_ID_L, &low);
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_CHIP_ID_H, &high);
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_CHIP_ID_L, &low);

You should check the return value as it's an entirely different problem not
being able to access the device than reading back wrong identification
information.

>  	id = (high << 8) | low;
>  
>  	if ((id != OV2722_ID) && (id != OV2720_ID)) {
> @@ -929,8 +924,9 @@ static int ov2722_detect(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_SUB_ID, &high);
> +	high = 0;
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_SUB_ID, &high);
>  	revision = (u8)high & 0x0f;
>  
>  	dev_dbg(&client->dev, "sensor_revision = 0x%x\n", revision);

This will also mean failure to read the revision is reported as revision 0.

These were the problems that pre-existed the patch, so if you're willing to
address these, please send v2 (would be nice). I can also merge this.

-- 
Kind regards,

Sakari Ailus