Re: [PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Matthias,

On Thu, Feb 26, 2026 at 09:30:08PM +0100, Matthias Fend wrote:
> Hi Philipp,
> 
> thanks for your feedback.
> 
> Am 26.02.2026 um 11:13 schrieb Philipp Zabel:
> > On Do, 2026-02-26 at 09:56 +0100, Matthias Fend wrote:
> > > Add support for the required power supplies as well as the control of an
> > > optional sensor reset.
> > > 
> > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > ---
> > >   drivers/media/i2c/ov08d10.c | 104 +++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 97 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
> > > index cfe18dcde174ddc1f198cb2aaa6b4a3b34045508..4dba264488b3e1950016deb3fa34732871cc34fc 100644
> > > --- a/drivers/media/i2c/ov08d10.c
> > > +++ b/drivers/media/i2c/ov08d10.c
> > [...]
> > > @@ -1379,6 +1442,7 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   {
> > >   	struct ov08d10 *ov08d10;
> > >   	unsigned long freq;
> > > +	unsigned int i;
> > >   	int ret;
> > >   	ov08d10 = devm_kzalloc(&client->dev, sizeof(*ov08d10), GFP_KERNEL);
> > > @@ -1404,12 +1468,32 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		return ret;
> > >   	}
> > > +	ov08d10->reset = devm_reset_control_get_optional(ov08d10->dev, NULL);
> > 
> > Please use devm_reset_control_get_optional_exclusive() directly.
> 
> ACK
> 
> > 
> > > +	if (IS_ERR(ov08d10->reset))
> > > +		return dev_err_probe(ov08d10->dev, PTR_ERR(ov08d10->reset),
> > > +				     "failed to get reset\n");
> > > +	reset_control_assert(ov08d10->reset);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(ov08d10_supply_names); i++)
> > > +		ov08d10->supplies[i].supply = ov08d10_supply_names[i];
> > > +
> > > +	ret = devm_regulator_bulk_get(ov08d10->dev,
> > > +				      ARRAY_SIZE(ov08d10->supplies),
> > > +				      ov08d10->supplies);
> > > +	if (ret)
> > > +		return dev_err_probe(ov08d10->dev, ret,
> > > +				     "failed to get regulators\n");
> > > +
> > >   	v4l2_i2c_subdev_init(&ov08d10->sd, client, &ov08d10_subdev_ops);
> > > +	ret = ov08d10_power_on(ov08d10->dev);
> > > +	if (ret)
> > > +		return dev_err_probe(ov08d10->dev, ret, "failed to power on\n");
> > > +
> > >   	ret = ov08d10_identify_module(ov08d10);
> > >   	if (ret) {
> > >   		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
> > > -		return ret;
> > > +		goto probe_error_power_off;
> > >   	}
> > >   	mutex_init(&ov08d10->mutex);
> > > @@ -1430,6 +1514,9 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		goto probe_error_v4l2_ctrl_handler_free;
> > >   	}
> > > +	pm_runtime_set_active(ov08d10->dev);
> > > +	pm_runtime_enable(ov08d10->dev);
> > > +
> > >   	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
> > >   	if (ret < 0) {
> > >   		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
> > > @@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		goto probe_error_media_entity_cleanup;
> > >   	}
> > > -	/*
> > > -	 * Device is already turned on by i2c-core with ACPI domain PM.
> > > -	 * Enable runtime PM and turn off the device.
> > > -	 */
> > 
> > The commit message does not explain why this comment is dropped.
> 
> I didn't find the comment particularly helpful and since other sensors
> manage without it and there's now more than just ACPI, and "turn off"
> happens later, I thought it was fine to just drop the comment.
> 
> If you think it should still be included, I'd be happy to change it.

I'd drop it and mention this in the commit message, there's nothing
specific to this driver here (nor even camera sensors in general).

> 
> > 
> > > -	pm_runtime_set_active(ov08d10->dev);
> > > -	pm_runtime_enable(ov08d10->dev);
> > >   	pm_runtime_idle(ov08d10->dev);
> > >   	return 0;
> > >   probe_error_media_entity_cleanup:
> > > +	pm_runtime_disable(ov08d10->dev);
> > > +	pm_runtime_set_suspended(ov08d10->dev);
> > 
> > Does this do the correct thing if v4l2_async_register_subdev_sensor()
> > returns -EPROBE_DEFER (for example via privacy led) and then it probes
> > a second time? It looks like the assumption pm_runtime_set_active()
> > doesn't hold then.
> 
> At least it works as expected for me. But as mentioned, I don't have an ACPI
> hardware setup available. Does your point maybe refer to ACPI, or what
> exactly do you mean?
> To me, it now looks very similar to other Omnivision ACPI drivers – do you
> perhaps have a specific suggestion for what should be changed?

Moving pm_runtime_set_active() and pm_runtime_enable() calls above
v4l2_async_register_subdev_sensor() (and related teardown changes) are
actually a bugfix (the driver's external APIs are exposed before runtime PM
is enabled and thus e.g. streaming on will fail). It'd be nice to post that
separately from non-ACPI power management changes.

-- 
Kind regards,

Sakari Ailus