staging: media: atomisp: remove redundant ov2722_startup() helper

RFC PATCH In review Mohit Mishra 3 participants 3 messages Aug 9

Subsystems (4)

Media (V4L/DVB) Staging Intel MID (Mobile Internet Device) platform Staging - atomisp driver

lore

Share & tools

lore

Mohit Mishra

author

2026-08-09 18:01 UTC #0 lore raw

RFC PATCH 1/1 staging: media: atomisp: remove redundant ov2722_startup() helper

In atomisp-ov2722.c, ov2722_startup() was invoked inside ov2722_set_fmt() to program sensor hardware registers during format negotiation, accompanied by a "TODO: remove it" comment.

Standard V4L2 subdevice drivers do not program hardware registers during .set_fmt(), but defer hardware register configuration to stream start (.s_stream(1)).

This patch:
1. Removes the ov2722_startup() helper function and its TODO comment.
2. Refactors register initialization into a static ov2722_startup_registers() helper.
3. Moves sensor software reset and resolution register array configuration into ov2722_s_stream() when enable == 1.
4. Preserves the PMIC power-cycle retry loop on startup failure inside ov2722_s_stream(1) to maintain hardware fault-tolerance for AtomISP tablets.
5. Updates ov2722_set_fmt() to purely update internal software state.

BEHAVIORAL RISK & UNTESTED NOTICE:
This is a functional timing change. Deferring hardware register writes to .s_stream(1) has not been tested on physical Intel AtomISP hardware. Submitted as RFC for review by AtomISP maintainers (Hans de Goede, Mauro Carvalho Chehab).

Signed-off-by Mohit Mishra <[email protected]>

---
 .../media/atomisp/i2c/atomisp-ov2722.c        | 109 +++++++++---------
 1 file changed, 53 insertions(+), 56 deletions(-)

drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +53 ~~-56~~

index 2c41c496daa6..5cbfbe5217a4 100644
@@ -599,39 +599,14 @@ static int ov2722_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
-/* TODO: remove it. */
-static int ov2722_startup(struct v4l2_subdev *sd)
-{
-	struct ov2722_device *dev = to_ov2722_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret = 0;
-
-	ret = ov2722_write_reg(client, OV2722_8BIT,
-			       OV2722_SW_RESET, 0x01);
-	if (ret) {
-		dev_err(&client->dev, "ov2722 reset err.\n");
-		return ret;
-	}
-
-	ret = ov2722_write_reg_array(client, dev->res->regs);
-	if (ret) {
-		dev_err(&client->dev, "ov2722 write register err.\n");
-		return ret;
-	}
-
-	return ret;
-}
-
 static int ov2722_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
 	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct ov2722_device *dev = to_ov2722_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov2722_resolution *res;
 	struct camera_mipi_info *ov2722_info = NULL;
-	int ret = 0;
 
 	if (format->pad)
 		return -EINVAL;

@@ -662,38 +637,8 @@ static int ov2722_set_fmt(struct v4l2_subdev *sd,
 	dev->pixels_per_line = dev->res->pixels_per_line;
 	dev->lines_per_frame = dev->res->lines_per_frame;
 
-	ret = ov2722_startup(sd);
-	if (ret) {
-		int i = 0;
-
-		dev_err(&client->dev, "ov2722 startup err, retry to power up\n");
-		for (i = 0; i < OV2722_POWER_UP_RETRY_NUM; i++) {
-			dev_err(&client->dev,
-				"ov2722 retry to power up %d/%d times, result: ",
-				i + 1, OV2722_POWER_UP_RETRY_NUM);
-			power_down(sd);
-			ret = power_up(sd);
-			if (ret) {
-				dev_err(&client->dev, "power up failed, continue\n");
-				continue;
-			}
-			ret = ov2722_startup(sd);
-			if (ret) {
-				dev_err(&client->dev, " startup FAILED!\n");
-			} else {
-				dev_err(&client->dev, " startup SUCCESS!\n");
-				break;
-			}
-		}
-		if (ret) {
-			dev_err(&client->dev, "ov2722 startup err\n");
-			goto err;
-		}
-	}
-
-err:
 	mutex_unlock(&dev->input_lock);
-	return ret;
+	return 0;
 }
 
 static int ov2722_get_fmt(struct v4l2_subdev *sd,

@@ -746,6 +691,26 @@ static int ov2722_detect(struct i2c_client *client)
 	return 0;
 }
 
+static int ov2722_startup_registers(struct v4l2_subdev *sd)
+{
+	struct ov2722_device *dev = to_ov2722_sensor(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret;
+
+	ret = ov2722_write_reg(client, OV2722_8BIT,
+			       OV2722_SW_RESET, 0x01);
+	if (ret) {
+		dev_err(&client->dev, "ov2722 reset err.\n");
+		return ret;
+	}
+
+	ret = ov2722_write_reg_array(client, dev->res->regs);
+	if (ret)
+		dev_err(&client->dev, "ov2722 write register err.\n");
+
+	return ret;
+}
+
 static int ov2722_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct ov2722_device *dev = to_ov2722_sensor(sd);

@@ -754,10 +719,42 @@ static int ov2722_s_stream(struct v4l2_subdev *sd, int enable)
 
 	mutex_lock(&dev->input_lock);
 
+	if (enable) {
+		ret = ov2722_startup_registers(sd);
+		if (ret) {
+			int i;
+
+			dev_err(&client->dev, "ov2722 startup err, retry to power up\n");
+			for (i = 0; i < OV2722_POWER_UP_RETRY_NUM; i++) {
+				dev_err(&client->dev,
+					"ov2722 retry to power up %d/%d times, result: ",
+					i + 1, OV2722_POWER_UP_RETRY_NUM);
+				power_down(sd);
+				ret = power_up(sd);
+				if (ret) {
+					dev_err(&client->dev, "power up failed, continue\n");
+					continue;
+				}
+				ret = ov2722_startup_registers(sd);
+				if (ret) {
+					dev_err(&client->dev, " startup FAILED!\n");
+				} else {
+					dev_err(&client->dev, " startup SUCCESS!\n");
+					break;
+				}
+			}
+			if (ret) {
+				dev_err(&client->dev, "ov2722 startup err\n");
+				goto unlock;
+			}
+		}
+	}
+
 	ret = ov2722_write_reg(client, OV2722_8BIT, OV2722_SW_STREAM,
 			       enable ? OV2722_START_STREAMING :
 			       OV2722_STOP_STREAMING);
 
+unlock:
 	mutex_unlock(&dev->input_lock);
 	return ret;
 }

Signature · 1 line

2.43.0

Greg Kroah-Hartman

maintainer

2026-08-09 18:18 UTC #1 lore raw

Quoted 19 lines · Mohit Mishra

In atomisp-ov2722.c, ov2722_startup() was invoked inside ov2722_set_fmt() to program sensor hardware registers during format negotiation, accompanied by a "TODO: remove it" comment.
Standard V4L2 subdevice drivers do not program hardware registers during .set_fmt(), but defer hardware register configuration to stream start (.s_stream(1)).
This patch:
1. Removes the ov2722_startup() helper function and its TODO comment.
2. Refactors register initialization into a static ov2722_startup_registers() helper.
3. Moves sensor software reset and resolution register array configuration into ov2722_s_stream() when enable == 1.
4. Preserves the PMIC power-cycle retry loop on startup failure inside ov2722_s_stream(1) to maintain hardware fault-tolerance for AtomISP tablets.
5. Updates ov2722_set_fmt() to purely update internal software state.

Quoted 5 lines

BEHAVIORAL RISK & UNTESTED NOTICE:
This is a functional timing change. Deferring hardware register writes to .s_stream(1) has not been tested on physical Intel AtomISP hardware. Submitted as RFC for review by AtomISP maintainers (Hans de Goede, Mauro Carvalho Chehab).

I would recommend doing changes like this ONLY if you have the hardware to test it with.

And did you forget an Assisted-by: tag?

thanks,

greg k-h

Mauro Carvalho Chehab

2026-08-09 20:47 UTC #2 lore raw

Quoted 20 lines · On Sun, 9 Aug 2026 20:17:15 +0200 Greg Kroah-Hartman…

Quoted 19 lines · Mohit Mishra
In atomisp-ov2722.c, ov2722_startup() was invoked inside ov2722_set_fmt() to program sensor hardware registers during format negotiation, accompanied by a "TODO: remove it" comment.
Standard V4L2 subdevice drivers do not program hardware registers during .set_fmt(), but defer hardware register configuration to stream start (.s_stream(1)).
This patch:
1. Removes the ov2722_startup() helper function and its TODO comment.
2. Refactors register initialization into a static ov2722_startup_registers() helper.
3. Moves sensor software reset and resolution register array configuration into ov2722_s_stream() when enable == 1.
4. Preserves the PMIC power-cycle retry loop on startup failure inside ov2722_s_stream(1) to maintain hardware fault-tolerance for AtomISP tablets.
5. Updates ov2722_set_fmt() to purely update internal software state.

Quoted 8 lines

Quoted 5 lines
BEHAVIORAL RISK & UNTESTED NOTICE:
This is a functional timing change. Deferring hardware register writes to .s_stream(1) has not been tested on physical Intel AtomISP hardware. Submitted as RFC for review by AtomISP maintainers (Hans de Goede, Mauro Carvalho Chehab).
I would recommend doing changes like this ONLY if you have the hardware to test it with.

Agreed. You should also run v4l2-compliance tools to check if the V4L2 API behavior didn't cause regressions.

Quoted 6 lines

And did you forget an Assisted-by: tag?
thanks,
greg k-h

Thanks,
Mauro