Re: [PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Sep 02, 2024 at 09:49:33PM +0200, Benjamin Bara wrote:
> On Mon, 2 Sept 2024 at 20:10, Dave Stevenson wrote:
> > On Mon, 2 Sept 2024 at 16:59, <bbara93@gmail.com> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > Currently, we have a trade-off between potentially enabling the privacy
> > > LED and reading out the connection state of the sensor during probe().
> > >
> > > To have a somewhat defined policy for now, make a decision based on the
> > > power supplies of the sensor. If they are enabled anyways, communicate
> > > with the powered sensor for an availability check. Otherwise, create the
> > > subdevice without knowing whether the sensor is connected or not.
> >
> > Almost all the camera modules used on Raspberry Pi have regulators
> > controlled via a GPIO, but no privacy LED. The preference from us is
> > very definitely to query the sensor during probe where possible to
> > flag up any connectivity issues, and indeed I've had a number of
> > support threads with imx290 where it's just not been connected but it
> > probed fully and showed up in libcamera.
> >
> > How can I opt in to patch 6 checking basic I2C to the sensor during
> > probe when I have a controllable regulator? (This is where the
> > discussions over a dtbinding for privacy LEDs and not powering up
> > sensors during probe comes in).
> 
> When you want to probe only during boot time, you can use the
> "regulator-boot-on" DT binding on your controllable regulator. This
> enables the regulator while it is probed and disables it later if not
> used (in comparison to "always-on"). Should also work for modules.

This seems like a hack, I'm sorry :-( I think we should instead have a
DT property standardized for camera sensors that tell whether or not
there is a privacy LED, and skip the detection in that case.

> Unfortunately, I don't have a clean solution (which also autosuspends)
> for "any probe time". I think it is not possible to enable a regulator
> from user space without having a consuming DT node. A somewhat clean
> workaround might be CONFIG_REGULATOR_USERSPACE_CONSUMER, which gives you
> the possibility to change the state of a regulator via sysfs (after
> creating a DT node). This gives you the possibility to enable it any
> time. However, the userspace-consumer driver gets the regulators
> exclusive, which means you cannot add the sensor driver as consumer and
> therefore cannot use the autosuspend feature of the imx290. Not really
> "nice", but probably "feasible" if you have special constraints when you
> are allowed to probe (e.g. the temperature as mentioned by Laurent). A
> DT binding would be easier for this case.
> 
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > >  drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 57 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 6b292bbb0856..338b2c5ea547 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> > >   * Power management
> > >   */
> > >
> > > +static bool is_imx290_power_on(struct imx290 *imx)
> > > +{
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> > > +               if (!regulator_is_enabled(imx->supplies[i].consumer))
> > > +                       return false;
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  static int imx290_power_on(struct imx290 *imx290)
> > >  {
> > >         int ret;
> > > @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
> > >  {
> > >         struct device *dev = &client->dev;
> > >         struct imx290 *imx290;
> > > +       bool power_on;
> > >         u64 val;
> > >         int ret;
> > >
> > > @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
> > >                 return ret;
> > >
> > >         /*
> > > -        * Enable power management. The driver supports runtime PM, but needs to
> > > -        * work when runtime PM is disabled in the kernel. To that end, power
> > > -        * the sensor on manually here.
> > > +        * Privacy mode: if the regulators are not enabled, avoid enabling them.
> > > +        * In case the regulators are enabled, we still want to make sure that
> > > +        * the regulators know that they have another consumer, therefore run
> > > +        * the powering sequence.
> > >          */
> > > -       ret = imx290_power_on(imx290);
> > > -       if (ret < 0) {
> > > -               dev_err(dev, "Could not power on the device\n");
> > > -               return ret;
> > > +       power_on = is_imx290_power_on(imx290);
> > > +       dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> > > +       if (power_on) {
> > > +               /*
> > > +                * Enable power management. The driver supports runtime PM, but
> > > +                * needs to work when runtime PM is disabled in the kernel. To
> > > +                * that end, power the sensor on manually here.
> > > +                */
> > > +               ret = imx290_power_on(imx290);
> > > +               if (ret < 0) {
> > > +                       dev_err(dev, "Could not power on the device\n");
> > > +                       return ret;
> > > +               }
> > > +
> > > +               /*
> > > +                * Enable runtime PM with autosuspend. As the device has been
> > > +                * powered manually, mark it as active, and increase the usage
> > > +                * count without resuming the device.
> > > +                */
> > > +               pm_runtime_set_active(dev);
> > > +               pm_runtime_get_noresume(dev);
> > >         }
> > >
> > > -       /*
> > > -        * Enable runtime PM with autosuspend. As the device has been powered
> > > -        * manually, mark it as active, and increase the usage count without
> > > -        * resuming the device.
> > > -        */
> > > -       pm_runtime_set_active(dev);
> > > -       pm_runtime_get_noresume(dev);
> > >         pm_runtime_enable(dev);
> > >         pm_runtime_set_autosuspend_delay(dev, 1000);
> > >         pm_runtime_use_autosuspend(dev);
> > >
> > > -       /* Make sure the sensor is available before V4L2 subdev init. */
> > > -       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > -       if (ret) {
> > > -               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > > -               goto err_pm;
> > > -       }
> > > -       if (val != IMX290_STANDBY_STANDBY) {
> > > -               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > > -               goto err_pm;
> > > +       /*
> > > +        * Make sure the sensor is available before V4L2 subdev init.
> > > +        * This only works when the sensor is powered.
> > > +        */
> > > +       if (power_on) {
> > > +               ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > +               if (ret) {
> > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > +                                           "Failed to detect sensor\n");
> > > +                       goto err_pm;
> > > +               }
> > > +               if (val != IMX290_STANDBY_STANDBY) {
> > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > +                                           "Sensor is not in standby\n");
> > > +                       goto err_pm;
> > > +               }
> > >         }
> > >
> > >         /* Initialize the V4L2 subdev. */
> > > @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
> > >          * Decrease the PM usage count. The device will get suspended after the
> > >          * autosuspend delay, turning the power off.
> > >          */
> > > -       pm_runtime_mark_last_busy(dev);
> > > -       pm_runtime_put_autosuspend(dev);
> > > +       if (power_on) {
> > > +               pm_runtime_mark_last_busy(dev);
> > > +               pm_runtime_put_autosuspend(dev);
> > > +       }
> > >
> > >         return 0;
> > >

-- 
Regards,

Laurent Pinchart