media: staging: atomisp:

These 3 patches fixes (at least I hope) some issues found in or around 'lm3554_probe()'.

Please review them carefully. I've only compile tested the changes and I propose them because they sound logical to me.

The first one, return an error code instead of 0 if the call to an initialisation function fails.
The 2nd one reorders own some label are reached in order to have a logical flow (first error goes to last label, last error goes to first label) The 3rd one fix the use 'media_entity_cleanup()'. If this one is correct, some other drivers will need to be fixed the same way.

Christophe JAILLET (3):
media: staging: atomisp: Return an error code in case

    of error in 'lm3554_probe()'
  media: staging: atomisp: Fix an error handling path in
    'lm3554_probe()'

media: staging: atomisp: Fix usage of 'media_entity_cleanup()'

 .../media/atomisp/i2c/atomisp-lm3554.c        | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)

2.17.0

If 'v4l2_ctrl_handler_init()' fails, we go to the error handling path, do some clean-up and return err, which is known to be 0 (i.e. success).

Axe the 'ret' variable and use 'err' directly in order to return the error code instead.
Also remove the initialization of 'err' which was hiding this issue.

---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 7098bf317f16..723fa74ff815 100644
@@ -852,10 +852,9 @@ static void *lm3554_platform_data_func(struct i2c_client *client)
 
 static int lm3554_probe(struct i2c_client *client)
 {
-	int err = 0;
+	int err;
 	struct lm3554 *flash;
 	unsigned int i;
-	int ret;
 
 	flash = kzalloc(sizeof(*flash), GFP_KERNEL);
 	if (!flash)

@@ -868,10 +867,9 @@ static int lm3554_probe(struct i2c_client *client)
 	flash->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	flash->mode = ATOMISP_FLASH_MODE_OFF;
 	flash->timeout = LM3554_MAX_TIMEOUT / LM3554_TIMEOUT_STEPSIZE - 1;
-	ret -	    v4l2_ctrl_handler_init(&flash->ctrl_handler,
-				   ARRAY_SIZE(lm3554_controls));
-	if (ret) {
+	err = v4l2_ctrl_handler_init(&flash->ctrl_handler,
+				     ARRAY_SIZE(lm3554_controls));
+	if (err) {
 		dev_err(&client->dev, "error initialize a ctrl_handler.\n");
 		goto fail2;
 	}

2.17.0

The use of 'fail1' and 'fail2' is not correct. Reorder these calls to branch at the right place of the error handling path.

---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 723fa74ff815..1e5f516f6e50 100644
@@ -871,7 +871,7 @@ static int lm3554_probe(struct i2c_client *client)
 				     ARRAY_SIZE(lm3554_controls));
 	if (err) {
 		dev_err(&client->dev, "error initialize a ctrl_handler.\n");
-		goto fail2;
+		goto fail1;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)

@@ -879,7 +879,6 @@ static int lm3554_probe(struct i2c_client *client)
 				     NULL);
 
 	if (flash->ctrl_handler.error) {
-
 		dev_err(&client->dev, "ctrl_handler error.\n");
 		goto fail2;
 	}

@@ -888,7 +887,7 @@ static int lm3554_probe(struct i2c_client *client)
 	err = media_entity_pads_init(&flash->sd.entity, 0, NULL);
 	if (err) {
 		dev_err(&client->dev, "error initialize a media entity.\n");
-		goto fail1;
+		goto fail2;
 	}
 
 	flash->sd.entity.function = MEDIA_ENT_F_FLASH;

2.17.0

Maybe it would be good to improve the names at the same time?

julia

Its scheduled for deletion - please don't bother.

Alan

According to the doc, 'media_entity_cleanup()' must be called after unregistering the entity.
All places I've check do it that way.
So, move the call after 'v4l2_device_unregister_subdev()' as done elsewhere.

Actually, this is not an issue, because 'media_entity_cleanup()' does nothing, but it is more future proof.

---
The change from '&flash->sd.entity' to '&sd->entity' in the last hunk is done because most of the drivers I've checked do it that way. Not sure if it is correct. It looks logical to me and it can be compiled. That's all I know.

If this patch is reviewed and confirmed, I'll propose similar fixes for some other drivers.

---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 1e5f516f6e50..b369b101abe7 100644
@@ -902,11 +902,12 @@ static int lm3554_probe(struct i2c_client *client)
 		goto fail2;
 	}
 	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
+
 fail2:
-	media_entity_cleanup(&flash->sd.entity);
 	v4l2_ctrl_handler_free(&flash->ctrl_handler);
 fail1:
 	v4l2_device_unregister_subdev(&flash->sd);
+	media_entity_cleanup(&flash->sd.entity);
 	kfree(flash);
 
 	return err;

@@ -918,9 +919,9 @@ static int lm3554_remove(struct i2c_client *client)
 	struct lm3554 *flash = to_lm3554(sd);
 	int ret;
 
-	media_entity_cleanup(&flash->sd.entity);
 	v4l2_ctrl_handler_free(&flash->ctrl_handler);
 	v4l2_device_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
 
 	atomisp_gmin_remove_subdev(sd);
 

2.17.0