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

[Matthias Fend]

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@xxxxxxxxx>
> > ---
> >   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.

> > -	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?

Thanks a lot
 ~Matthias

> >   	media_entity_cleanup(&ov08d10->sd.entity);
> >   
> >   probe_error_v4l2_ctrl_handler_free:
> >   	v4l2_ctrl_handler_free(ov08d10->sd.ctrl_handler);
> >   	mutex_destroy(&ov08d10->mutex);
> >   
> > +probe_error_power_off:
> > +	ov08d10_power_off(ov08d10->dev);
> > +
> >   	return ret;
> >   }
>
> regards
> Philipp