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
