Hi Dave!
On Mon, 2 Sept 2024 at 20:10, Dave Stevenson
<dave.stevenson@xxxxxxxxxxxxxxx> 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.
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.
Kind regards
Benjamin
> Thanks
> Dave
> > 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;
> >
> >
> > --
> > 2.46.0
> >
> >
