ov5645: Move an error code assignment in ov5645_probe()

PATCH In review SF Markus Elfring 3 participants 4 messages Mar 14, 2018 – Mar 15, 2018

Subsystems (3)

Media (V4L/DVB) Kernel janitors V4L2 camera sensor drivers

lore

Share & tools

lore

SF Markus Elfring

author

2018-03-14 21:15 UTC #0 lore raw

PATCH 1/1 ov5645: Move an error code assignment in ov5645_probe()

From: Markus Elfring <[email protected]>
Date: Wed, 14 Mar 2018 22:02:52 +0100

Move an assignment for a specific error code so that it is stored only once in this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/media/i2c/ov5645.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/media/i2c/ov5645.c +1 -5

index d28845f7356f..374576380fd4 100644
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
 		dev_err(dev, "could not read ID high\n");
-		ret = -ENODEV;
 		goto power_down;
 	}
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
 	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
 		dev_err(dev, "could not read ID low\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
 		dev_err(dev, "could not read AEC/AGC mode\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg20);
 	if (ret < 0) {
 		dev_err(dev, "could not read vflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg21);
 	if (ret < 0) {
 		dev_err(dev, "could not read hflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
 
 power_down:
 	ov5645_s_power(&ov5645->sd, false);
+	ret = -ENODEV;
 free_entity:
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:

Signature · 1 line

2.16.2

Sakari Ailus

maintainer

2018-03-14 22:17 UTC #1 lore raw

Quoted 7 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 14 Mar 2018 22:02:52 +0100
Move an assignment for a specific error code so that it is stored only once in this function implementation.
This issue was detected by using the Coccinelle software.

How?

Quoted 49 lines

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/media/i2c/ov5645.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/media/i2c/ov5645.c +0 -5

index d28845f7356f..374576380fd4 100644
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
 		dev_err(dev, "could not read ID high\n");
-		ret = -ENODEV;
 		goto power_down;
 	}
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
 	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
 		dev_err(dev, "could not read ID low\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
 		dev_err(dev, "could not read AEC/AGC mode\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg20);
 	if (ret < 0) {
 		dev_err(dev, "could not read vflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg21);
 	if (ret < 0) {
 		dev_err(dev, "could not read hflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

drivers/media/i2c/ov5645.c static int ov5645_probe(struct i2c_client *client, · line 1332

Quoted 4 lines

drivers/media/i2c/ov5645.c +1 -0

@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
 
 power_down:
 	ov5645_s_power(&ov5645->sd, false);
+	ret = -ENODEV;

I don't think this is where people would expect you to set the error code in general. It should rather take place before goto, not after it. That'd mean another variable, and I'm not convinced the result would improve the driver.

Quoted 3 lines

drivers/media/i2c/ov5645.c +0 -0

 free_entity:
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:

Signature · 4 lines

Regards,

Sakari Ailus
[email protected]

Todor Tomov

2018-03-15 08:57 UTC #2 lore raw

Hi,

Quoted 61 lines · Sakari Ailus

Quoted 7 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 14 Mar 2018 22:02:52 +0100
Move an assignment for a specific error code so that it is stored only once in this function implementation.
This issue was detected by using the Coccinelle software.

How?

Quoted 50 lines

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/media/i2c/ov5645.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/media/i2c/ov5645.c +0 -5

index d28845f7356f..374576380fd4 100644
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
 		dev_err(dev, "could not read ID high\n");
-		ret = -ENODEV;
 		goto power_down;
 	}
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
 	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
 		dev_err(dev, "could not read ID low\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
 		dev_err(dev, "could not read AEC/AGC mode\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg20);
 	if (ret < 0) {
 		dev_err(dev, "could not read vflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
 			      &ov5645->timing_tc_reg21);
 	if (ret < 0) {
 		dev_err(dev, "could not read hflip value\n");
-		ret = -ENODEV;
 		goto power_down;
 	}

@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,

Quoted 8 lines

drivers/media/i2c/ov5645.c static int ov5645_probe(struct i2c_client *client, · line 1332
Quoted 3 lines
drivers/media/i2c/ov5645.c +1 -0
 power_down:
 	ov5645_s_power(&ov5645->sd, false);
+	ret = -ENODEV;
I don't think this is where people would expect you to set the error code in general. It should rather take place before goto, not after it. That'd mean another variable, and I'm not convinced the result would improve the driver.

I agree with Sakari.

Quoted 5 lines

Quoted 4 lines
drivers/media/i2c/ov5645.c +0 -0
 free_entity:
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:

Signature · 2 lines

Best regards,
Todor Tomov

SF Markus Elfring

author

2018-03-15 09:34 UTC #3 lore raw

In reply to Sakari Ailus

Quoted 6 lines

Quoted 4 lines
Move an assignment for a specific error code so that it is stored only once in this function implementation.
This issue was detected by using the Coccinelle software.
How?

Would you like to experiment a bit more with the following approach for the semantic patch language?

show_same_statements3.cocci:

@duplicated_code@
identifier work;
statement s1, s2;
type T;
@@
T work(...)
{
... when any
*if ((...) < 0)

*{
    ...

* s1
* s2
*}
... when any
*if ((...) < 0)

*{
    ...

* s1
* s2
*}
... when any
}

Quoted 8 lines

drivers/media/i2c/ov5645.c static int ov5645_probe(struct i2c_client *client, · line 1332
Quoted 5 lines
drivers/media/i2c/ov5645.c +1 -0
@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
 
 power_down:
 	ov5645_s_power(&ov5645->sd, false);
+	ret = -ENODEV;
I don't think this is where people would expect you to set the error code in general.

This can be. - The view depends on some factors.

Quoted 1 line

It should rather take place before goto, not after it.

I proposed another software design direction.

Quoted 1 line

That'd mean another variable,

To which detail do you refer here?

Quoted 1 line

and I'm not convinced the result would improve the driver.

Can you see the relevance of a small code reduction in this function?

Regards,
Markus