| Message ID | 20240902-imx290-avail-v3-7-b32a12799fed@skidata.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx290: check for availability in probe() | expand |
Hi Benjamin 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). Thanks Dave > 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; > > > -- > 2.46.0 > >
Hi Laurent! On Mon, 2 Sept 2024 at 22:04, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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. 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@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
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;