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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent!

On Mon, 2 Sept 2024 at 22:04, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxx> wrote:
> > > >
> > > > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> > > >
> > > > 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.

No prob, I didn't really expect this to be final and fully understand
it. I'll simply drop it for the next round. I also think that the DT
binding "has-privacy-led" is the much cleaner solution and also aligns
with the HW description constraint.

Thanks & regards
Benjamin

> > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux