On Fri, Sep 15, 2023 at 03:50:00PM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 03:16:59PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 11:48:43AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 15, 2023 at 02:30:12PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> > > > > On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > > > > > Enable runtime PM before registering the async sub-device as the ipu
> > > > > > > bridge may try to resume the device immediately after the async sub-device
> > > > > >
> > > > > > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > > > > > particular CSI-2 receiver.
> > > > > >
> > > > > > > has been registered. If runtime PM is still disabled, this will fail.
> > > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > drivers/media/i2c/ov2740.c | 14 ++++++++------
> > > > > > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > > > > > index 41d4f85470fd..319dc00e47b4 100644
> > > > > > > --- a/drivers/media/i2c/ov2740.c
> > > > > > > +++ b/drivers/media/i2c/ov2740.c
> > > > > > > @@ -1172,6 +1172,12 @@ static int ov2740_probe(struct i2c_client *client)
> > > > > > > goto probe_error_v4l2_ctrl_handler_free;
> > > > > > > }
> > > > > > >
> > > > > > > + /* Set the device's state to active if it's in D0 state. */
> > > > > > > + if (full_power)
> > > > > > > + pm_runtime_set_active(&client->dev);
> > > > > >
> > > > > > I wonder why we need this in drivers. If ACPI has powered the device on
> > > > > > prior to calling probe(), couldn't it also set the PM runtime state
> > > > > > accordingly ?
> > > > >
> > > > > What happens here is that the ipu bridge creates a VCM device and it
> > > > > resumes the related sensor before instantiating that device (see
> > > > > ipu_bridge_instantiate_vcm_work()). However this may take place already
> > > > > right after registering the async sub-device. Resuming the sensor will fail
> > > > > if runtime PM isn't enabled.
> > > > >
> > > > > I'll add something along these lines to the commit message.
> > > >
> > > > I understand this, but that doesn't answer my question :-) Why is there
> > > > a need to call pm_runtime_set_active(), couldn't it be done somewhere in
> > > > common code ?
> > >
> > > Ah, I think I misunderstood your earlier question.
> > >
> > > The sensor may be left powered off for probe if the device's _DSC object
> > > evaluates to 4. See Documentation/firmware-guide/acpi/non-d0-probe.rst for
> > > more information.
> >
> > That information should be available to core code, given that it's
> > retrieved in the driver with a call to acpi_dev_state_d0(). Couldn't it
> > thus be handled in core code ?
>
> The I²C framework is responsible powering on the device before probe, not
> the ACPI framework. The default runtime PM state is suspended, so I guess
> this could be changed for ACPI devices in the I²C framework. But that
> certainly does not belong to this patchset.
Of course. Sorry if I didn't make it clear that this comment was
unrelated to this patch series.
> I was actually hoping the ACPI introduced I²C devices could have power
> management working the same was as for DT: driver would resume the device
> instead of the framework. This would simplify drivers.
You're the ACPI specialist (I'm sorry about that :-)), whatever you
think is best is fine with me, as long as we simplify drivers. Power
management in camera sensor drivers is way too complicated to implement,
with too much boilerplate code being copied (often incorrectly) between
drivers.
> > > > > > > + pm_runtime_enable(&client->dev);
> > > > > > > + pm_runtime_idle(&client->dev);
> > > > > > > +
> > > > > >
> > > > > > With the commit message fixed,
> > > > > >
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > >
> > > > > Thank you.
> > > > >
> > > > > > > ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
> > > > > > > if (ret < 0) {
> > > > > > > dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
> > > > > > > @@ -1182,16 +1188,12 @@ static int ov2740_probe(struct i2c_client *client)
> > > > > > > if (ret)
> > > > > > > dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);
> > > > > > >
> > > > > > > - /* Set the device's state to active if it's in D0 state. */
> > > > > > > - if (full_power)
> > > > > > > - pm_runtime_set_active(&client->dev);
> > > > > > > - pm_runtime_enable(&client->dev);
> > > > > > > - pm_runtime_idle(&client->dev);
> > > > > > > -
> > > > > > > return 0;
> > > > > > >
> > > > > > > probe_error_media_entity_cleanup:
> > > > > > > media_entity_cleanup(&ov2740->sd.entity);
> > > > > > > + pm_runtime_disable(&client->dev);
> > > > > > > + pm_runtime_set_suspended(&client->dev);
> > > > > > >
> > > > > > > probe_error_v4l2_ctrl_handler_free:
> > > > > > > v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
--
Regards,
Laurent Pinchart
