[PATCH] [RFC PATCH] staging: media: atomisp: remove redundant ov2722_startup() helper

[PATCH] [RFC PATCH] staging: media: atomisp: remove redundant ov2722_startup() helper

[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.

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 <mishraloopmohit@gmail.com>
---
 .../media/atomisp/i2c/atomisp-ov2722.c        | 109 +++++++++---------
 1 file changed, 53 insertions(+), 56 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index 2c41c496daa6..5cbfbe5217a4 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -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;
 }
-- 
2.43.0

Re: [PATCH] [RFC PATCH] staging: media: atomisp: remove redundant ov2722_startup() helper

[Greg Kroah-Hartman]

On Sun, Aug 09, 2026 at 11:31:05PM +0530, Mohit Mishra wrote:
> 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).

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

Re: [PATCH] [RFC PATCH] staging: media: atomisp: remove redundant ov2722_startup() helper

[Mauro Carvalho Chehab]

On Sun, 9 Aug 2026 20:17:15 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Sun, Aug 09, 2026 at 11:31:05PM +0530, Mohit Mishra wrote:
> > 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).  
> 
> 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.

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



Thanks,
Mauro