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
