media: staging: atomisp: rework reading the id and revision values
Subsystems (4)
PATCH RESEND 1/1 media: staging: atomisp: rework reading the id and revision values
From: Tom Rix <[email protected]>
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.
--- .../media/atomisp/i2c/atomisp-ov2722.c | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +8 -12
@@ -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); 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);
Signature · 1 line
2.26.3
Hi Tom,
Quoted 51 lines · On Tue, Mar 29, 2022 at 04:25:26PM -0700, [email protected] wrote:
From: Tom Rix <[email protected]>
Clang static analysis reports this representative issue atomisp-ov2722.c:920:3: warning: 3rd function call
argument is an uninitialized valuedev_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 <[email protected]>--- .../media/atomisp/i2c/atomisp-ov2722.c | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-)drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +1
-4@@ -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);
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c static int ov2722_get_fmt(struct v4l2_subdev *sd, · line 919
Quoted 10 lines
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +4
-6-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.
Quoted 5 lines
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +0
-0id = (high <<8 ) | low;if ((id !=OV2722_ID ) && (id !=OV2720_ID )) {@@ -929,8 +924,9 @@ static int ov2722_detect(struct i2c_client *client)return -ENODEV ;
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c static int ov2722_detect(struct i2c_client *client) · line 932
Quoted 10 lines
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +3
-2} - 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.
Signature · 3 lines
Kind regards, Sakari Ailus