[v2] media: ov5640: Use runtime PM

  Switch to using runtime PM for power management.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- replace manual tracking of power status with pm_runtime_get_if_in_use
- power on the sensor before reading the checking the chip id
- add dependency on PM to Kconfig
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
 2 files changed, 67 insertions(+), 46 deletions(-)
  

Hi Paul,

Thanks for the update.

On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> Switch to using runtime PM for power management.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - replace manual tracking of power status with pm_runtime_get_if_in_use
> - power on the sensor before reading the checking the chip id
> - add dependency on PM to Kconfig
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
>  2 files changed, 67 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e7194c1be4d2..97c3611d9304 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
>  	tristate "OmniVision OV5640 sensor support"
>  	depends on OF
>  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> +	depends on PM

I think this is not needed as the sensor is powered on explicitly in probe.

You should similarly power it off explicitly in remove, set the runtime PM
status suspended and disable runtime PM. See e.g. imx319 driver for an
example. It doesn't have resume callback but that doesn't really matter ---
it's just ACPI-only.

>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select V4L2_FWNODE
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 4de83d0ef85d..454ffd3c6d59 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -445,8 +446,6 @@ struct ov5640_dev {
>  	/* lock to protect all members below */
>  	struct mutex lock;
>  
> -	int power_count;
> -
>  	struct v4l2_mbus_framefmt fmt;
>  	bool pending_fmt_change;
>  
> @@ -2693,37 +2692,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  
>  /* --------------- Subdev Operations --------------- */
>  
> -static int ov5640_s_power(struct v4l2_subdev *sd, int on)
> -{
> -	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> -	int ret = 0;
> -
> -	mutex_lock(&sensor->lock);
> -
> -	/*
> -	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
> -	 * update the power state.
> -	 */
> -	if (sensor->power_count == !on) {
> -		ret = ov5640_set_power(sensor, !!on);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	/* Update the power count. */
> -	sensor->power_count += on ? 1 : -1;
> -	WARN_ON(sensor->power_count < 0);
> -out:
> -	mutex_unlock(&sensor->lock);
> -
> -	if (on && !ret && sensor->power_count == 1) {
> -		/* restore controls */
> -		ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> -	}
> -
> -	return ret;
> -}
> -
>  static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
>  				     struct v4l2_fract *fi,
>  				     u32 width, u32 height)
> @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	/* v4l2_ctrl_lock() locks our own mutex */
>  
> +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> +		return 0;
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_AUTOGAIN:
>  		val = ov5640_get_gain(sensor);
> @@ -3303,6 +3274,8 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> +
>  	return 0;
>  }
>  
> @@ -3334,7 +3307,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>  	 * not apply any controls to H/W at this time. Instead
>  	 * the controls will be restored right after power-up.
>  	 */
> -	if (sensor->power_count == 0)
> +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
>  		return 0;
>  
>  	switch (ctrl->id) {
> @@ -3376,6 +3349,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> +
>  	return ret;
>  }
>  
> @@ -3657,6 +3632,12 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct ov5640_dev *sensor = to_ov5640_dev(sd);
>  	int ret = 0;
>  
> +	if (enable) {
> +		ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	mutex_lock(&sensor->lock);
>  
>  	if (sensor->streaming == !enable) {
> @@ -3681,8 +3662,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!ret)
>  			sensor->streaming = enable;
>  	}
> +
>  out:
>  	mutex_unlock(&sensor->lock);
> +
> +	if (!enable || ret)
> +		pm_runtime_put(&sensor->i2c_client->dev);
> +
>  	return ret;
>  }
>  
> @@ -3704,7 +3690,6 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd,
>  }
>  
>  static const struct v4l2_subdev_core_ops ov5640_core_ops = {
> -	.s_power = ov5640_s_power,
>  	.log_status = v4l2_ctrl_subdev_log_status,
>  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> @@ -3750,15 +3735,11 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
>  	int ret = 0;
>  	u16 chip_id;
>  
> -	ret = ov5640_set_power_on(sensor);
> -	if (ret)
> -		return ret;
> -
>  	ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
>  	if (ret) {
>  		dev_err(&client->dev, "%s: failed to read chip identifier\n",
>  			__func__);
> -		goto power_off;
> +		return ret;
>  	}
>  
>  	if (chip_id != 0x5640) {
> @@ -3767,8 +3748,6 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
>  		ret = -ENXIO;
>  	}
>  
> -power_off:
> -	ov5640_set_power_off(sensor);
>  	return ret;
>  }
>  
> @@ -3863,20 +3842,35 @@ static int ov5640_probe(struct i2c_client *client)
>  
>  	mutex_init(&sensor->lock);
>  
> -	ret = ov5640_check_chip_id(sensor);
> +	ret = ov5640_init_controls(sensor);
>  	if (ret)
>  		goto entity_cleanup;
>  
> -	ret = ov5640_init_controls(sensor);
> +	ret = ov5640_set_power(sensor, true);
>  	if (ret)
> -		goto entity_cleanup;
> +		goto free_ctrls;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get(dev);
> +
> +	ret = ov5640_check_chip_id(sensor);
> +	if (ret)
> +		goto err_pm_runtime;
>  
>  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>  	if (ret)
> -		goto free_ctrls;
> +		goto err_pm_runtime;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
>  
>  	return 0;
>  
> +err_pm_runtime:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  free_ctrls:
>  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>  entity_cleanup:
> @@ -3898,6 +3892,31 @@ static int ov5640_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int __maybe_unused ov5640_sensor_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> +
> +	return ov5640_set_power(ov5640, false);
> +}
> +
> +static int __maybe_unused ov5640_sensor_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> +	int ret;
> +
> +	ret = ov5640_set_power(ov5640, true);
> +	if (ret)
> +		return ret;
> +
> +	return v4l2_ctrl_handler_setup(&ov5640->ctrls.handler);
> +}
> +
> +static const struct dev_pm_ops ov5640_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ov5640_sensor_suspend, ov5640_sensor_resume, NULL)
> +};
> +
>  static const struct i2c_device_id ov5640_id[] = {
>  	{"ov5640", 0},
>  	{},
> @@ -3914,6 +3933,7 @@ static struct i2c_driver ov5640_i2c_driver = {
>  	.driver = {
>  		.name  = "ov5640",
>  		.of_match_table	= ov5640_dt_ids,
> +		.pm = &ov5640_pm_ops,
>  	},
>  	.id_table = ov5640_id,
>  	.probe_new = ov5640_probe,
  

Hi Sakari,

On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > Switch to using runtime PM for power management.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > - power on the sensor before reading the checking the chip id
> > - add dependency on PM to Kconfig
> > ---
> >  drivers/media/i2c/Kconfig  |   1 +
> >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> >  2 files changed, 67 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index e7194c1be4d2..97c3611d9304 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> >  	tristate "OmniVision OV5640 sensor support"
> >  	depends on OF
> >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > +	depends on PM
> 
> I think this is not needed as the sensor is powered on explicitly in probe.
> 
> You should similarly power it off explicitly in remove, set the runtime PM
> status suspended and disable runtime PM. See e.g. imx319 driver for an
> example. It doesn't have resume callback but that doesn't really matter ---
> it's just ACPI-only.

Do we want to continue supporting !PM ? Does it have any real use case
when dealing with camera sensors ?

> >  	select MEDIA_CONTROLLER
> >  	select VIDEO_V4L2_SUBDEV_API
> >  	select V4L2_FWNODE
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 4de83d0ef85d..454ffd3c6d59 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > @@ -445,8 +446,6 @@ struct ov5640_dev {
> >  	/* lock to protect all members below */
> >  	struct mutex lock;
> >  
> > -	int power_count;
> > -
> >  	struct v4l2_mbus_framefmt fmt;
> >  	bool pending_fmt_change;
> >  
> > @@ -2693,37 +2692,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> >  
> >  /* --------------- Subdev Operations --------------- */
> >  
> > -static int ov5640_s_power(struct v4l2_subdev *sd, int on)
> > -{
> > -	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > -	int ret = 0;
> > -
> > -	mutex_lock(&sensor->lock);
> > -
> > -	/*
> > -	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
> > -	 * update the power state.
> > -	 */
> > -	if (sensor->power_count == !on) {
> > -		ret = ov5640_set_power(sensor, !!on);
> > -		if (ret)
> > -			goto out;
> > -	}
> > -
> > -	/* Update the power count. */
> > -	sensor->power_count += on ? 1 : -1;
> > -	WARN_ON(sensor->power_count < 0);
> > -out:
> > -	mutex_unlock(&sensor->lock);
> > -
> > -	if (on && !ret && sensor->power_count == 1) {
> > -		/* restore controls */
> > -		ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> >  static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
> >  				     struct v4l2_fract *fi,
> >  				     u32 width, u32 height)
> > @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> >  
> >  	/* v4l2_ctrl_lock() locks our own mutex */
> >  
> > +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> > +		return 0;
> > +
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_AUTOGAIN:
> >  		val = ov5640_get_gain(sensor);
> > @@ -3303,6 +3274,8 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >  
> > +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -3334,7 +3307,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >  	 * not apply any controls to H/W at this time. Instead
> >  	 * the controls will be restored right after power-up.
> >  	 */
> > -	if (sensor->power_count == 0)
> > +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> >  		return 0;
> >  
> >  	switch (ctrl->id) {
> > @@ -3376,6 +3349,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >  
> > +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -3657,6 +3632,12 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
> >  	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >  	int ret = 0;
> >  
> > +	if (enable) {
> > +		ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >  	mutex_lock(&sensor->lock);
> >  
> >  	if (sensor->streaming == !enable) {
> > @@ -3681,8 +3662,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
> >  		if (!ret)
> >  			sensor->streaming = enable;
> >  	}
> > +
> >  out:
> >  	mutex_unlock(&sensor->lock);
> > +
> > +	if (!enable || ret)
> > +		pm_runtime_put(&sensor->i2c_client->dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -3704,7 +3690,6 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd,
> >  }
> >  
> >  static const struct v4l2_subdev_core_ops ov5640_core_ops = {
> > -	.s_power = ov5640_s_power,
> >  	.log_status = v4l2_ctrl_subdev_log_status,
> >  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> >  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > @@ -3750,15 +3735,11 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
> >  	int ret = 0;
> >  	u16 chip_id;
> >  
> > -	ret = ov5640_set_power_on(sensor);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
> >  	if (ret) {
> >  		dev_err(&client->dev, "%s: failed to read chip identifier\n",
> >  			__func__);
> > -		goto power_off;
> > +		return ret;
> >  	}
> >  
> >  	if (chip_id != 0x5640) {
> > @@ -3767,8 +3748,6 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
> >  		ret = -ENXIO;
> >  	}
> >  
> > -power_off:
> > -	ov5640_set_power_off(sensor);
> >  	return ret;
> >  }
> >  
> > @@ -3863,20 +3842,35 @@ static int ov5640_probe(struct i2c_client *client)
> >  
> >  	mutex_init(&sensor->lock);
> >  
> > -	ret = ov5640_check_chip_id(sensor);
> > +	ret = ov5640_init_controls(sensor);
> >  	if (ret)
> >  		goto entity_cleanup;
> >  
> > -	ret = ov5640_init_controls(sensor);
> > +	ret = ov5640_set_power(sensor, true);
> >  	if (ret)
> > -		goto entity_cleanup;
> > +		goto free_ctrls;
> > +
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get(dev);
> > +
> > +	ret = ov5640_check_chip_id(sensor);
> > +	if (ret)
> > +		goto err_pm_runtime;
> >  
> >  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> >  	if (ret)
> > -		goto free_ctrls;
> > +		goto err_pm_runtime;
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_put_autosuspend(dev);
> >  
> >  	return 0;
> >  
> > +err_pm_runtime:
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> >  free_ctrls:
> >  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> >  entity_cleanup:
> > @@ -3898,6 +3892,31 @@ static int ov5640_remove(struct i2c_client *client)
> >  	return 0;
> >  }
> >  
> > +static int __maybe_unused ov5640_sensor_suspend(struct device *dev)
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> > +
> > +	return ov5640_set_power(ov5640, false);
> > +}
> > +
> > +static int __maybe_unused ov5640_sensor_resume(struct device *dev)
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> > +	int ret;
> > +
> > +	ret = ov5640_set_power(ov5640, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return v4l2_ctrl_handler_setup(&ov5640->ctrls.handler);
> > +}
> > +
> > +static const struct dev_pm_ops ov5640_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(ov5640_sensor_suspend, ov5640_sensor_resume, NULL)
> > +};
> > +
> >  static const struct i2c_device_id ov5640_id[] = {
> >  	{"ov5640", 0},
> >  	{},
> > @@ -3914,6 +3933,7 @@ static struct i2c_driver ov5640_i2c_driver = {
> >  	.driver = {
> >  		.name  = "ov5640",
> >  		.of_match_table	= ov5640_dt_ids,
> > +		.pm = &ov5640_pm_ops,
> >  	},
> >  	.id_table = ov5640_id,
> >  	.probe_new = ov5640_probe,
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
  

Hi Laurent,

On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > Switch to using runtime PM for power management.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v2:
> > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > - power on the sensor before reading the checking the chip id
> > > - add dependency on PM to Kconfig
> > > ---
> > >  drivers/media/i2c/Kconfig  |   1 +
> > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index e7194c1be4d2..97c3611d9304 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > >  	tristate "OmniVision OV5640 sensor support"
> > >  	depends on OF
> > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > +	depends on PM
> > 
> > I think this is not needed as the sensor is powered on explicitly in probe.
> > 
> > You should similarly power it off explicitly in remove, set the runtime PM
> > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > example. It doesn't have resume callback but that doesn't really matter ---
> > it's just ACPI-only.
> 
> Do we want to continue supporting !PM ? Does it have any real use case
> when dealing with camera sensors ?

Probably not much.

The changes I proposed are not eve related on runtime PM. Hence the
question here is whether there should be a dependency to CONFIG_PM or not,
and as there's no technical reason to have it, it should be omitted.
  

Hi Sakari,

On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > Switch to using runtime PM for power management.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > 
> > > > ---
> > > > Changes in v2:
> > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > - power on the sensor before reading the checking the chip id
> > > > - add dependency on PM to Kconfig
> > > > ---
> > > >  drivers/media/i2c/Kconfig  |   1 +
> > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index e7194c1be4d2..97c3611d9304 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > >  	tristate "OmniVision OV5640 sensor support"
> > > >  	depends on OF
> > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > +	depends on PM
> > > 
> > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > 
> > > You should similarly power it off explicitly in remove, set the runtime PM
> > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > example. It doesn't have resume callback but that doesn't really matter ---
> > > it's just ACPI-only.
> > 
> > Do we want to continue supporting !PM ? Does it have any real use case
> > when dealing with camera sensors ?
> 
> Probably not much.
> 
> The changes I proposed are not eve related on runtime PM. Hence the
> question here is whether there should be a dependency to CONFIG_PM or not,
> and as there's no technical reason to have it, it should be omitted.

But if there's no real use case for !PM, wouldn't we be better off
depending on PM and simplifying the probe functions instead ?
  

Hi Laurent,

On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > Switch to using runtime PM for power management.
> > > > > 
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > 
> > > > > ---
> > > > > Changes in v2:
> > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > - power on the sensor before reading the checking the chip id
> > > > > - add dependency on PM to Kconfig
> > > > > ---
> > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > >  	depends on OF
> > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > +	depends on PM
> > > > 
> > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > 
> > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > it's just ACPI-only.
> > > 
> > > Do we want to continue supporting !PM ? Does it have any real use case
> > > when dealing with camera sensors ?
> > 
> > Probably not much.
> > 
> > The changes I proposed are not eve related on runtime PM. Hence the
> > question here is whether there should be a dependency to CONFIG_PM or not,
> > and as there's no technical reason to have it, it should be omitted.
> 
> But if there's no real use case for !PM, wouldn't we be better off
> depending on PM and simplifying the probe functions instead ?

What would change in the probe function if runtime PM was required by the
driver?
  

Hi Sakari,

On Fri, Mar 11, 2022 at 03:32:26PM +0200, Sakari Ailus wrote:
> On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> > On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > > Switch to using runtime PM for power management.
> > > > > > 
> > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > 
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > > - power on the sensor before reading the checking the chip id
> > > > > > - add dependency on PM to Kconfig
> > > > > > ---
> > > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > > >  	depends on OF
> > > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > > +	depends on PM
> > > > > 
> > > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > > 
> > > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > > it's just ACPI-only.
> > > > 
> > > > Do we want to continue supporting !PM ? Does it have any real use case
> > > > when dealing with camera sensors ?
> > > 
> > > Probably not much.
> > > 
> > > The changes I proposed are not eve related on runtime PM. Hence the
> > > question here is whether there should be a dependency to CONFIG_PM or not,
> > > and as there's no technical reason to have it, it should be omitted.
> > 
> > But if there's no real use case for !PM, wouldn't we be better off
> > depending on PM and simplifying the probe functions instead ?
> 
> What would change in the probe function if runtime PM was required by the
> driver?

We wouldn't need the complicated dance of calling

	ret = ov5640_set_power(sensor, true);
	if (ret)
		goto free_ctrls;

	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	pm_runtime_get(dev);

but could write it as

	pm_runtime_enable(dev);
	pm_runtime_resume_and_get(dev);
  

Hi Laurent,

On Sun, Mar 13, 2022 at 03:01:52PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Mar 11, 2022 at 03:32:26PM +0200, Sakari Ailus wrote:
> > On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> > > On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > > > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > > > Switch to using runtime PM for power management.
> > > > > > > 
> > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > > > - power on the sensor before reading the checking the chip id
> > > > > > > - add dependency on PM to Kconfig
> > > > > > > ---
> > > > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > > > >  	depends on OF
> > > > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > > > +	depends on PM
> > > > > > 
> > > > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > > > 
> > > > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > > > it's just ACPI-only.
> > > > > 
> > > > > Do we want to continue supporting !PM ? Does it have any real use case
> > > > > when dealing with camera sensors ?
> > > > 
> > > > Probably not much.
> > > > 
> > > > The changes I proposed are not eve related on runtime PM. Hence the
> > > > question here is whether there should be a dependency to CONFIG_PM or not,
> > > > and as there's no technical reason to have it, it should be omitted.
> > > 
> > > But if there's no real use case for !PM, wouldn't we be better off
> > > depending on PM and simplifying the probe functions instead ?
> > 
> > What would change in the probe function if runtime PM was required by the
> > driver?
> 
> We wouldn't need the complicated dance of calling
> 
> 	ret = ov5640_set_power(sensor, true);
> 	if (ret)
> 		goto free_ctrls;
> 
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 	pm_runtime_get(dev);

pm_runtime_get() is redundant here.

> 
> but could write it as
> 
> 	pm_runtime_enable(dev);
> 	pm_runtime_resume_and_get(dev);

You'll need put here, too.

Also the latter only works on DT-based systems so it's not an option for
most of the drivers.
  

Hi Sakari,

On Sun, Mar 13, 2022 at 03:38:34PM +0200, Sakari Ailus wrote:
> On Sun, Mar 13, 2022 at 03:01:52PM +0200, Laurent Pinchart wrote:
> > On Fri, Mar 11, 2022 at 03:32:26PM +0200, Sakari Ailus wrote:
> > > On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > > > > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > > > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > > > > Switch to using runtime PM for power management.
> > > > > > > > 
> > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > > > > - power on the sensor before reading the checking the chip id
> > > > > > > > - add dependency on PM to Kconfig
> > > > > > > > ---
> > > > > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > > > > >  	depends on OF
> > > > > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > > > > +	depends on PM
> > > > > > > 
> > > > > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > > > > 
> > > > > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > > > > it's just ACPI-only.
> > > > > > 
> > > > > > Do we want to continue supporting !PM ? Does it have any real use case
> > > > > > when dealing with camera sensors ?
> > > > > 
> > > > > Probably not much.
> > > > > 
> > > > > The changes I proposed are not eve related on runtime PM. Hence the
> > > > > question here is whether there should be a dependency to CONFIG_PM or not,
> > > > > and as there's no technical reason to have it, it should be omitted.
> > > > 
> > > > But if there's no real use case for !PM, wouldn't we be better off
> > > > depending on PM and simplifying the probe functions instead ?
> > > 
> > > What would change in the probe function if runtime PM was required by the
> > > driver?
> > 
> > We wouldn't need the complicated dance of calling
> > 
> > 	ret = ov5640_set_power(sensor, true);
> > 	if (ret)
> > 		goto free_ctrls;
> > 
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 	pm_runtime_get(dev);
> 
> pm_runtime_get() is redundant here.
> 
> > 
> > but could write it as
> > 
> > 	pm_runtime_enable(dev);
> > 	pm_runtime_resume_and_get(dev);
> 
> You'll need put here, too.

Yes, after reading the version register (or doing any other harware
access). Actually the full code would be


 	pm_runtime_enable(dev);
 	pm_runtime_resume_and_get(dev);

	/* Hardware access */

	pm_runtime_set_autosuspend_delay(dev, 1000);
	pm_runtime_use_autosuspend(dev);
	pm_runtime_put_autosuspend(dev);

(plus error handling).

If the probe function doesn't need to access the hardware, then
the above becomes

	pm_runtime_enable(dev);
	pm_runtime_set_autosuspend_delay(dev, 1000);
	pm_runtime_use_autosuspend(dev);

instead of having to power up the device just in case !PM.

> Also the latter only works on DT-based systems so it's not an option for
> most of the drivers.

How so, what's wrong with the above for ACPI-based system ?
  

Hi Laurent,

On Sun, Mar 13, 2022 at 04:16:44PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sun, Mar 13, 2022 at 03:38:34PM +0200, Sakari Ailus wrote:
> > On Sun, Mar 13, 2022 at 03:01:52PM +0200, Laurent Pinchart wrote:
> > > On Fri, Mar 11, 2022 at 03:32:26PM +0200, Sakari Ailus wrote:
> > > > On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> > > > > On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > > > > > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > > > > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > > > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > > > > > Switch to using runtime PM for power management.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > > Changes in v2:
> > > > > > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > > > > > - power on the sensor before reading the checking the chip id
> > > > > > > > > - add dependency on PM to Kconfig
> > > > > > > > > ---
> > > > > > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > > > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > > > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > > > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > > > > > >  	depends on OF
> > > > > > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > > > > > +	depends on PM
> > > > > > > > 
> > > > > > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > > > > > 
> > > > > > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > > > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > > > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > > > > > it's just ACPI-only.
> > > > > > > 
> > > > > > > Do we want to continue supporting !PM ? Does it have any real use case
> > > > > > > when dealing with camera sensors ?
> > > > > > 
> > > > > > Probably not much.
> > > > > > 
> > > > > > The changes I proposed are not eve related on runtime PM. Hence the
> > > > > > question here is whether there should be a dependency to CONFIG_PM or not,
> > > > > > and as there's no technical reason to have it, it should be omitted.
> > > > > 
> > > > > But if there's no real use case for !PM, wouldn't we be better off
> > > > > depending on PM and simplifying the probe functions instead ?
> > > > 
> > > > What would change in the probe function if runtime PM was required by the
> > > > driver?
> > > 
> > > We wouldn't need the complicated dance of calling
> > > 
> > > 	ret = ov5640_set_power(sensor, true);
> > > 	if (ret)
> > > 		goto free_ctrls;
> > > 
> > > 	pm_runtime_set_active(dev);
> > > 	pm_runtime_enable(dev);
> > > 	pm_runtime_get(dev);
> > 
> > pm_runtime_get() is redundant here.
> > 
> > > 
> > > but could write it as
> > > 
> > > 	pm_runtime_enable(dev);
> > > 	pm_runtime_resume_and_get(dev);
> > 
> > You'll need put here, too.
> 
> Yes, after reading the version register (or doing any other harware
> access). Actually the full code would be
> 
> 
>  	pm_runtime_enable(dev);
>  	pm_runtime_resume_and_get(dev);
> 
> 	/* Hardware access */
> 
> 	pm_runtime_set_autosuspend_delay(dev, 1000);
> 	pm_runtime_use_autosuspend(dev);
> 	pm_runtime_put_autosuspend(dev);
> 
> (plus error handling).
> 
> If the probe function doesn't need to access the hardware, then
> the above becomes
> 
> 	pm_runtime_enable(dev);
> 	pm_runtime_set_autosuspend_delay(dev, 1000);
> 	pm_runtime_use_autosuspend(dev);
> 
> instead of having to power up the device just in case !PM.
> 
> > Also the latter only works on DT-based systems so it's not an option for
> > most of the drivers.
> 
> How so, what's wrong with the above for ACPI-based system ?

I²C devices are already powered on for probe on ACPI based systems.
  

Hi Sakari,

On Mon, Mar 14, 2022 at 10:01:00PM +0200, Sakari Ailus wrote:
> On Sun, Mar 13, 2022 at 04:16:44PM +0200, Laurent Pinchart wrote:
> > On Sun, Mar 13, 2022 at 03:38:34PM +0200, Sakari Ailus wrote:
> > > On Sun, Mar 13, 2022 at 03:01:52PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Mar 11, 2022 at 03:32:26PM +0200, Sakari Ailus wrote:
> > > > > On Fri, Mar 11, 2022 at 03:20:55PM +0200, Laurent Pinchart wrote:
> > > > > > On Fri, Mar 11, 2022 at 03:15:54PM +0200, Sakari Ailus wrote:
> > > > > > > On Fri, Mar 11, 2022 at 02:30:09PM +0200, Laurent Pinchart wrote:
> > > > > > > > On Fri, Mar 11, 2022 at 02:23:53PM +0200, Sakari Ailus wrote:
> > > > > > > > > On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> > > > > > > > > > Switch to using runtime PM for power management.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > > Changes in v2:
> > > > > > > > > > - replace manual tracking of power status with pm_runtime_get_if_in_use
> > > > > > > > > > - power on the sensor before reading the checking the chip id
> > > > > > > > > > - add dependency on PM to Kconfig
> > > > > > > > > > ---
> > > > > > > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > > > > > > >  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
> > > > > > > > > >  2 files changed, 67 insertions(+), 46 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > > > > index e7194c1be4d2..97c3611d9304 100644
> > > > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > > > @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
> > > > > > > > > >  	tristate "OmniVision OV5640 sensor support"
> > > > > > > > > >  	depends on OF
> > > > > > > > > >  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> > > > > > > > > > +	depends on PM
> > > > > > > > > 
> > > > > > > > > I think this is not needed as the sensor is powered on explicitly in probe.
> > > > > > > > > 
> > > > > > > > > You should similarly power it off explicitly in remove, set the runtime PM
> > > > > > > > > status suspended and disable runtime PM. See e.g. imx319 driver for an
> > > > > > > > > example. It doesn't have resume callback but that doesn't really matter ---
> > > > > > > > > it's just ACPI-only.
> > > > > > > > 
> > > > > > > > Do we want to continue supporting !PM ? Does it have any real use case
> > > > > > > > when dealing with camera sensors ?
> > > > > > > 
> > > > > > > Probably not much.
> > > > > > > 
> > > > > > > The changes I proposed are not eve related on runtime PM. Hence the
> > > > > > > question here is whether there should be a dependency to CONFIG_PM or not,
> > > > > > > and as there's no technical reason to have it, it should be omitted.
> > > > > > 
> > > > > > But if there's no real use case for !PM, wouldn't we be better off
> > > > > > depending on PM and simplifying the probe functions instead ?
> > > > > 
> > > > > What would change in the probe function if runtime PM was required by the
> > > > > driver?
> > > > 
> > > > We wouldn't need the complicated dance of calling
> > > > 
> > > > 	ret = ov5640_set_power(sensor, true);
> > > > 	if (ret)
> > > > 		goto free_ctrls;
> > > > 
> > > > 	pm_runtime_set_active(dev);
> > > > 	pm_runtime_enable(dev);
> > > > 	pm_runtime_get(dev);
> > > 
> > > pm_runtime_get() is redundant here.
> > > 
> > > > but could write it as
> > > > 
> > > > 	pm_runtime_enable(dev);
> > > > 	pm_runtime_resume_and_get(dev);
> > > 
> > > You'll need put here, too.
> > 
> > Yes, after reading the version register (or doing any other harware
> > access). Actually the full code would be
> > 
> > 
> >  	pm_runtime_enable(dev);
> >  	pm_runtime_resume_and_get(dev);
> > 
> > 	/* Hardware access */
> > 
> > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > 	pm_runtime_use_autosuspend(dev);
> > 	pm_runtime_put_autosuspend(dev);
> > 
> > (plus error handling).
> > 
> > If the probe function doesn't need to access the hardware, then
> > the above becomes
> > 
> > 	pm_runtime_enable(dev);
> > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > 	pm_runtime_use_autosuspend(dev);
> > 
> > instead of having to power up the device just in case !PM.
> > 
> > > Also the latter only works on DT-based systems so it's not an option for
> > > most of the drivers.
> > 
> > How so, what's wrong with the above for ACPI-based system ?
> 
> I²C devices are already powered on for probe on ACPI based systems.

Not through RPM I suppose ?
  

Hi Laurent,

On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
...
> > > Yes, after reading the version register (or doing any other harware
> > > access). Actually the full code would be
> > > 
> > > 
> > >  	pm_runtime_enable(dev);
> > >  	pm_runtime_resume_and_get(dev);
> > > 
> > > 	/* Hardware access */
> > > 
> > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > 	pm_runtime_use_autosuspend(dev);
> > > 	pm_runtime_put_autosuspend(dev);
> > > 
> > > (plus error handling).
> > > 
> > > If the probe function doesn't need to access the hardware, then
> > > the above becomes
> > > 
> > > 	pm_runtime_enable(dev);
> > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > 	pm_runtime_use_autosuspend(dev);
> > > 
> > > instead of having to power up the device just in case !PM.
> > > 
> > > > Also the latter only works on DT-based systems so it's not an option for
> > > > most of the drivers.
> > > 
> > > How so, what's wrong with the above for ACPI-based system ?
> > 
> > I²C devices are already powered on for probe on ACPI based systems.
> 
> Not through RPM I suppose ?

Runtime PM isn't involved, this takes place in the ACPI framework (via
dev_pm_domain_attach() called in i2c_device_probe()).
  

Hi Paul,

Thank you for the patch.

On Fri, Mar 11, 2022 at 08:12:59PM +0900, Paul Elder wrote:
> Switch to using runtime PM for power management.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - replace manual tracking of power status with pm_runtime_get_if_in_use
> - power on the sensor before reading the checking the chip id
> - add dependency on PM to Kconfig
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov5640.c | 112 ++++++++++++++++++++++---------------
>  2 files changed, 67 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e7194c1be4d2..97c3611d9304 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1025,6 +1025,7 @@ config VIDEO_OV5640
>  	tristate "OmniVision OV5640 sensor support"
>  	depends on OF
>  	depends on GPIOLIB && VIDEO_V4L2 && I2C
> +	depends on PM
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select V4L2_FWNODE
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 4de83d0ef85d..454ffd3c6d59 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -445,8 +446,6 @@ struct ov5640_dev {
>  	/* lock to protect all members below */
>  	struct mutex lock;
>  
> -	int power_count;
> -
>  	struct v4l2_mbus_framefmt fmt;
>  	bool pending_fmt_change;
>  
> @@ -2693,37 +2692,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  
>  /* --------------- Subdev Operations --------------- */
>  
> -static int ov5640_s_power(struct v4l2_subdev *sd, int on)
> -{
> -	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> -	int ret = 0;
> -
> -	mutex_lock(&sensor->lock);
> -
> -	/*
> -	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
> -	 * update the power state.
> -	 */
> -	if (sensor->power_count == !on) {
> -		ret = ov5640_set_power(sensor, !!on);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	/* Update the power count. */
> -	sensor->power_count += on ? 1 : -1;
> -	WARN_ON(sensor->power_count < 0);
> -out:
> -	mutex_unlock(&sensor->lock);
> -
> -	if (on && !ret && sensor->power_count == 1) {
> -		/* restore controls */
> -		ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> -	}
> -
> -	return ret;
> -}
> -
>  static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
>  				     struct v4l2_fract *fi,
>  				     u32 width, u32 height)
> @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	/* v4l2_ctrl_lock() locks our own mutex */
>  
> +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> +		return 0;

I'm afraid this won't work as intended :-S This function is called by
v4l2_ctrl_handler_setup(), itself called from ov5640_sensor_resume(). At
that point, runtime PM isn't flagged as in use yet, we're still in the
process of resuming.

There are a few ways around this. The simplest one may be to move the
v4l2_ctrl_handler_setup() call from ov5640_sensor_resume() to
ov5640_s_stream(). Sakari, any opinion ?

> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_AUTOGAIN:
>  		val = ov5640_get_gain(sensor);
> @@ -3303,6 +3274,8 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> +
>  	return 0;
>  }
>  
> @@ -3334,7 +3307,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>  	 * not apply any controls to H/W at this time. Instead
>  	 * the controls will be restored right after power-up.
>  	 */
> -	if (sensor->power_count == 0)
> +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
>  		return 0;
>  
>  	switch (ctrl->id) {
> @@ -3376,6 +3349,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put_autosuspend(&sensor->i2c_client->dev);
> +
>  	return ret;
>  }
>  
> @@ -3657,6 +3632,12 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct ov5640_dev *sensor = to_ov5640_dev(sd);
>  	int ret = 0;
>  
> +	if (enable) {
> +		ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	mutex_lock(&sensor->lock);
>  
>  	if (sensor->streaming == !enable) {
> @@ -3681,8 +3662,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!ret)
>  			sensor->streaming = enable;
>  	}
> +
>  out:
>  	mutex_unlock(&sensor->lock);
> +
> +	if (!enable || ret)
> +		pm_runtime_put(&sensor->i2c_client->dev);
> +
>  	return ret;
>  }
>  
> @@ -3704,7 +3690,6 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd,
>  }
>  
>  static const struct v4l2_subdev_core_ops ov5640_core_ops = {
> -	.s_power = ov5640_s_power,
>  	.log_status = v4l2_ctrl_subdev_log_status,
>  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> @@ -3750,15 +3735,11 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
>  	int ret = 0;
>  	u16 chip_id;
>  
> -	ret = ov5640_set_power_on(sensor);
> -	if (ret)
> -		return ret;
> -
>  	ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
>  	if (ret) {
>  		dev_err(&client->dev, "%s: failed to read chip identifier\n",
>  			__func__);
> -		goto power_off;
> +		return ret;
>  	}
>  
>  	if (chip_id != 0x5640) {
> @@ -3767,8 +3748,6 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor)
>  		ret = -ENXIO;
>  	}
>  
> -power_off:
> -	ov5640_set_power_off(sensor);
>  	return ret;
>  }
>  
> @@ -3863,20 +3842,35 @@ static int ov5640_probe(struct i2c_client *client)
>  
>  	mutex_init(&sensor->lock);
>  
> -	ret = ov5640_check_chip_id(sensor);
> +	ret = ov5640_init_controls(sensor);
>  	if (ret)
>  		goto entity_cleanup;
>  
> -	ret = ov5640_init_controls(sensor);
> +	ret = ov5640_set_power(sensor, true);
>  	if (ret)
> -		goto entity_cleanup;
> +		goto free_ctrls;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get(dev);
> +
> +	ret = ov5640_check_chip_id(sensor);
> +	if (ret)
> +		goto err_pm_runtime;
>  
>  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>  	if (ret)
> -		goto free_ctrls;
> +		goto err_pm_runtime;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
>  
>  	return 0;
>  
> +err_pm_runtime:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  free_ctrls:
>  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>  entity_cleanup:
> @@ -3898,6 +3892,31 @@ static int ov5640_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int __maybe_unused ov5640_sensor_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> +
> +	return ov5640_set_power(ov5640, false);
> +}
> +
> +static int __maybe_unused ov5640_sensor_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5640_dev *ov5640 = to_ov5640_dev(sd);
> +	int ret;
> +
> +	ret = ov5640_set_power(ov5640, true);
> +	if (ret)
> +		return ret;
> +
> +	return v4l2_ctrl_handler_setup(&ov5640->ctrls.handler);
> +}
> +
> +static const struct dev_pm_ops ov5640_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ov5640_sensor_suspend, ov5640_sensor_resume, NULL)
> +};
> +
>  static const struct i2c_device_id ov5640_id[] = {
>  	{"ov5640", 0},
>  	{},
> @@ -3914,6 +3933,7 @@ static struct i2c_driver ov5640_i2c_driver = {
>  	.driver = {
>  		.name  = "ov5640",
>  		.of_match_table	= ov5640_dt_ids,
> +		.pm = &ov5640_pm_ops,
>  	},
>  	.id_table = ov5640_id,
>  	.probe_new = ov5640_probe,
  

Hi Laurent,

On Sat, Mar 19, 2022 at 12:28:22AM +0200, Laurent Pinchart wrote:
> > @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> >  
> >  	/* v4l2_ctrl_lock() locks our own mutex */
> >  
> > +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> > +		return 0;
> 
> I'm afraid this won't work as intended :-S This function is called by
> v4l2_ctrl_handler_setup(), itself called from ov5640_sensor_resume(). At
> that point, runtime PM isn't flagged as in use yet, we're still in the
> process of resuming.
> 
> There are a few ways around this. The simplest one may be to move the
> v4l2_ctrl_handler_setup() call from ov5640_sensor_resume() to
> ov5640_s_stream(). Sakari, any opinion ?

That's one way to do it, yes.

The problem is that when the s_ctrl callback is run, there's no information
on whether it's being called from the runtime PM resume handler (which
powers on the device) or via another path.

Changing that would require changing the callback arguments, or adding a
new callback with that information included.
  

On Mon, Mar 21, 2022 at 12:58:25PM +0200, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Sat, Mar 19, 2022 at 12:28:22AM +0200, Laurent Pinchart wrote:
> > > @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > >  
> > >  	/* v4l2_ctrl_lock() locks our own mutex */
> > >  
> > > +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> > > +		return 0;
> > 
> > I'm afraid this won't work as intended :-S This function is called by
> > v4l2_ctrl_handler_setup(), itself called from ov5640_sensor_resume(). At
> > that point, runtime PM isn't flagged as in use yet, we're still in the
> > process of resuming.
> > 
> > There are a few ways around this. The simplest one may be to move the
> > v4l2_ctrl_handler_setup() call from ov5640_sensor_resume() to
> > ov5640_s_stream(). Sakari, any opinion ?
> 
> That's one way to do it, yes.
> 
> The problem is that when the s_ctrl callback is run, there's no information
> on whether it's being called from the runtime PM resume handler (which
> powers on the device) or via another path.
> 
> Changing that would require changing the callback arguments, or adding a
> new callback with that information included.

We can also add a flag in the driver-specific structure to tell if the
device is powered or not. Delaying v4l2_ctrl_handler_setup() until
.s_stream() seems easier though, but then maybe we should return early
from .s_ctrl() until streaming is started, even if the device is powered
on ?
  

On Mon, Mar 21, 2022 at 01:24:19PM +0200, Laurent Pinchart wrote:
> On Mon, Mar 21, 2022 at 12:58:25PM +0200, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Sat, Mar 19, 2022 at 12:28:22AM +0200, Laurent Pinchart wrote:
> > > > @@ -3288,6 +3256,9 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > >  
> > > >  	/* v4l2_ctrl_lock() locks our own mutex */
> > > >  
> > > > +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> > > > +		return 0;
> > > 
> > > I'm afraid this won't work as intended :-S This function is called by
> > > v4l2_ctrl_handler_setup(), itself called from ov5640_sensor_resume(). At
> > > that point, runtime PM isn't flagged as in use yet, we're still in the
> > > process of resuming.
> > > 
> > > There are a few ways around this. The simplest one may be to move the
> > > v4l2_ctrl_handler_setup() call from ov5640_sensor_resume() to
> > > ov5640_s_stream(). Sakari, any opinion ?
> > 
> > That's one way to do it, yes.
> > 
> > The problem is that when the s_ctrl callback is run, there's no information
> > on whether it's being called from the runtime PM resume handler (which
> > powers on the device) or via another path.
> > 
> > Changing that would require changing the callback arguments, or adding a
> > new callback with that information included.
> 
> We can also add a flag in the driver-specific structure to tell if the
> device is powered or not. Delaying v4l2_ctrl_handler_setup() until
> .s_stream() seems easier though, but then maybe we should return early
> from .s_ctrl() until streaming is started, even if the device is powered
> on ?

(Finally fixed Jacopo's e-mail.)

The device is likely powered off if streaming is disabled. But it might not
be if the controls are being accessed right after stopping streaming.

You could use a driver-local information to keep the knowledge of this, but
it'd be better if drivers wouldn't have to manage such information.
  

Hi Sakari,

On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> ...
> > > > Yes, after reading the version register (or doing any other harware
> > > > access). Actually the full code would be
> > > > 
> > > > 
> > > >  	pm_runtime_enable(dev);
> > > >  	pm_runtime_resume_and_get(dev);
> > > > 
> > > > 	/* Hardware access */
> > > > 
> > > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > 	pm_runtime_use_autosuspend(dev);
> > > > 	pm_runtime_put_autosuspend(dev);
> > > > 
> > > > (plus error handling).
> > > > 
> > > > If the probe function doesn't need to access the hardware, then
> > > > the above becomes
> > > > 
> > > > 	pm_runtime_enable(dev);
> > > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > 	pm_runtime_use_autosuspend(dev);
> > > > 
> > > > instead of having to power up the device just in case !PM.
> > > > 
> > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > most of the drivers.

Does the former work on ACPI systems ?

> > > > How so, what's wrong with the above for ACPI-based system ?
> > > 
> > > I²C devices are already powered on for probe on ACPI based systems.
> > 
> > Not through RPM I suppose ?
> 
> Runtime PM isn't involved, this takes place in the ACPI framework (via
> dev_pm_domain_attach() called in i2c_device_probe()).

How can we fix this ? It may have made sense a long time ago, but it's
making RPM handling way too difficult in I2C drivers now. We need
something better instead of continuing to rely on cargo-cult for probe
functions. Most drivers are broken.
  

Hi Laurent,

On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > ...
> > > > > Yes, after reading the version register (or doing any other harware
> > > > > access). Actually the full code would be
> > > > > 
> > > > > 
> > > > >  	pm_runtime_enable(dev);
> > > > >  	pm_runtime_resume_and_get(dev);
> > > > > 
> > > > > 	/* Hardware access */
> > > > > 
> > > > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > 	pm_runtime_use_autosuspend(dev);
> > > > > 	pm_runtime_put_autosuspend(dev);
> > > > > 
> > > > > (plus error handling).
> > > > > 
> > > > > If the probe function doesn't need to access the hardware, then
> > > > > the above becomes
> > > > > 
> > > > > 	pm_runtime_enable(dev);
> > > > > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > 	pm_runtime_use_autosuspend(dev);
> > > > > 
> > > > > instead of having to power up the device just in case !PM.
> > > > > 
> > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > most of the drivers.
> 
> Does the former work on ACPI systems ?

Yes (i.e. the one that was above the quoted text).

> 
> > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > 
> > > > I²C devices are already powered on for probe on ACPI based systems.
> > > 
> > > Not through RPM I suppose ?
> > 
> > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > dev_pm_domain_attach() called in i2c_device_probe()).
> 
> How can we fix this ? It may have made sense a long time ago, but it's
> making RPM handling way too difficult in I2C drivers now. We need
> something better instead of continuing to rely on cargo-cult for probe
> functions. Most drivers are broken.

Some could be broken, there's no question of that. A lot of drivers support
either ACPI or DT, too, so not _that_ many need to work with both. Albeit
that number is probably increasing constantly for the same devices are used
on both.

Then there are drivers that prefer not powering on the device in probe (see
<URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
it gets complicated to support all the combinatios of DT/ACPI (with or
without the flag / property for waiving powering device on for probe) and
CONFIG_PM enabled/disabled.

What I think could be done to add a flag for drivers that handle power on
their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
works. Right now it expects a property on the device but that check could
be moved to existing drivers using the flag. Not many drivers are currently
using the flag. I think this would simplify driver implementation as both
firmware interfaces would work the same way in this respect.

You'd have to change one driver at a time, and people should be encouraged
to write new drivers with that flag. Or add the flag to all existing
drivers and not accept new ones with it.

These devices I think are all I²C but my understanding is that such
differences exist elsewhere in the kernel, too. If they are to be
addressed, it would probably be best to have a unified approach towards it.

Added a few more people and lists to cc.
  

On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Laurent,
>
> On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > ...
> > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > access). Actually the full code would be
> > > > > >
> > > > > >
> > > > > >       pm_runtime_enable(dev);
> > > > > >       pm_runtime_resume_and_get(dev);
> > > > > >
> > > > > >       /* Hardware access */
> > > > > >
> > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > >
> > > > > > (plus error handling).
> > > > > >
> > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > the above becomes
> > > > > >
> > > > > >       pm_runtime_enable(dev);
> > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > >
> > > > > > instead of having to power up the device just in case !PM.
> > > > > >
> > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > most of the drivers.
> >
> > Does the former work on ACPI systems ?
>
> Yes (i.e. the one that was above the quoted text).
>
> >
> > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > >
> > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > >
> > > > Not through RPM I suppose ?
> > >
> > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > dev_pm_domain_attach() called in i2c_device_probe()).
> >
> > How can we fix this ? It may have made sense a long time ago, but it's
> > making RPM handling way too difficult in I2C drivers now. We need
> > something better instead of continuing to rely on cargo-cult for probe
> > functions. Most drivers are broken.
>
> Some could be broken, there's no question of that. A lot of drivers support
> either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> that number is probably increasing constantly for the same devices are used
> on both.
>
> Then there are drivers that prefer not powering on the device in probe (see
> <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
> it gets complicated to support all the combinatios of DT/ACPI (with or
> without the flag / property for waiving powering device on for probe) and
> CONFIG_PM enabled/disabled.
>
> What I think could be done to add a flag for drivers that handle power on
> their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> works. Right now it expects a property on the device but that check could
> be moved to existing drivers using the flag. Not many drivers are currently
> using the flag. I think this would simplify driver implementation as both
> firmware interfaces would work the same way in this respect.
>
> You'd have to change one driver at a time, and people should be encouraged
> to write new drivers with that flag. Or add the flag to all existing
> drivers and not accept new ones with it.
>
> These devices I think are all I涎 but my understanding is that such
> differences exist elsewhere in the kernel, too. If they are to be
> addressed, it would probably be best to have a unified approach towards it.
>
> Added a few more people and lists to cc.

+ Hidenori from my team for visibility.

I think we may want to take a step back and first define the problem
itself. To do that, let's take a look separately at DT and ACPI cases
(is platform data still relevant? are there any other firmware
interfaces that deal with I2C devices?).
For simplicity, let's forget about the ACPI waived power on in probe.

DT:
 1) hardware state unknown when probe is called
 2) claim any independently managed resources (e.g. GPIOs)
 3) enable runtime PM
 4) if driver wants to access the hardware:
    a) runtime PM get
    b) enable any independently controlled resources (e.g. reset GPIO)
    c) [do access]
    d) disable any independently controlled resources
    e) runtime PM put
 5) after probe returns, regulators, clocks (and other similarly
managed resources) would be force disabled if their enable count is 0
 6) hardware state is off (after the runtime PM state settles)

ACPI:
 1) hardware state is active when probe is called
 2) [n/a]
 3) tell runtime PM framework that the state is active and then enable
runtime PM
 4) if driver wants to access the hardware:
    a) runtime PM get
    b) [n/a]
    c) [do access]
    d) [n/a]
    e) runtime PM put
 5) [n/a]
 6) hardware state is off (after the runtime PM state settles)

It seems like the relevant difference here is that for ACPI, the
driver needs to know that the initial state is active and also relay
this knowledge to the runtime PM subsystem. If we could make the ACPI
PM domain work the same way as regulators and clocks and eventually
power off some time later when the enable count is 0, then perhaps we
could avoid the problem in the first place?

Best regards,
Tomasz
  

[Fixed Jacopo's email address.]

On Mon, Aug 1, 2022 at 4:17 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Laurent,
> >
> > On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > > ...
> > > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > > access). Actually the full code would be
> > > > > > >
> > > > > > >
> > > > > > >       pm_runtime_enable(dev);
> > > > > > >       pm_runtime_resume_and_get(dev);
> > > > > > >
> > > > > > >       /* Hardware access */
> > > > > > >
> > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > > >
> > > > > > > (plus error handling).
> > > > > > >
> > > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > > the above becomes
> > > > > > >
> > > > > > >       pm_runtime_enable(dev);
> > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > >
> > > > > > > instead of having to power up the device just in case !PM.
> > > > > > >
> > > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > > most of the drivers.
> > >
> > > Does the former work on ACPI systems ?
> >
> > Yes (i.e. the one that was above the quoted text).
> >
> > >
> > > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > > >
> > > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > > >
> > > > > Not through RPM I suppose ?
> > > >
> > > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > > dev_pm_domain_attach() called in i2c_device_probe()).
> > >
> > > How can we fix this ? It may have made sense a long time ago, but it's
> > > making RPM handling way too difficult in I2C drivers now. We need
> > > something better instead of continuing to rely on cargo-cult for probe
> > > functions. Most drivers are broken.
> >
> > Some could be broken, there's no question of that. A lot of drivers support
> > either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> > that number is probably increasing constantly for the same devices are used
> > on both.
> >
> > Then there are drivers that prefer not powering on the device in probe (see
> > <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
> > it gets complicated to support all the combinatios of DT/ACPI (with or
> > without the flag / property for waiving powering device on for probe) and
> > CONFIG_PM enabled/disabled.
> >
> > What I think could be done to add a flag for drivers that handle power on
> > their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> > works. Right now it expects a property on the device but that check could
> > be moved to existing drivers using the flag. Not many drivers are currently
> > using the flag. I think this would simplify driver implementation as both
> > firmware interfaces would work the same way in this respect.
> >
> > You'd have to change one driver at a time, and people should be encouraged
> > to write new drivers with that flag. Or add the flag to all existing
> > drivers and not accept new ones with it.
> >
> > These devices I think are all I涎 but my understanding is that such
> > differences exist elsewhere in the kernel, too. If they are to be
> > addressed, it would probably be best to have a unified approach towards it.
> >
> > Added a few more people and lists to cc.
>
> + Hidenori from my team for visibility.
>
> I think we may want to take a step back and first define the problem
> itself. To do that, let's take a look separately at DT and ACPI cases
> (is platform data still relevant? are there any other firmware
> interfaces that deal with I2C devices?).
> For simplicity, let's forget about the ACPI waived power on in probe.
>
> DT:
>  1) hardware state unknown when probe is called
>  2) claim any independently managed resources (e.g. GPIOs)
>  3) enable runtime PM
>  4) if driver wants to access the hardware:
>     a) runtime PM get
>     b) enable any independently controlled resources (e.g. reset GPIO)
>     c) [do access]
>     d) disable any independently controlled resources
>     e) runtime PM put
>  5) after probe returns, regulators, clocks (and other similarly
> managed resources) would be force disabled if their enable count is 0
>  6) hardware state is off (after the runtime PM state settles)
>
> ACPI:
>  1) hardware state is active when probe is called
>  2) [n/a]
>  3) tell runtime PM framework that the state is active and then enable
> runtime PM
>  4) if driver wants to access the hardware:
>     a) runtime PM get
>     b) [n/a]
>     c) [do access]
>     d) [n/a]
>     e) runtime PM put
>  5) [n/a]
>  6) hardware state is off (after the runtime PM state settles)
>
> It seems like the relevant difference here is that for ACPI, the
> driver needs to know that the initial state is active and also relay
> this knowledge to the runtime PM subsystem. If we could make the ACPI
> PM domain work the same way as regulators and clocks and eventually
> power off some time later when the enable count is 0, then perhaps we
> could avoid the problem in the first place?
>
> Best regards,
> Tomasz
  

On Mon, Aug 01, 2022 at 04:23:54PM +0900, Tomasz Figa wrote:
> [Fixed Jacopo's email address.]
> 
> On Mon, Aug 1, 2022 at 4:17 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Laurent,
> > >
> > > On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > > > ...
> > > > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > > > access). Actually the full code would be
> > > > > > > >
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_resume_and_get(dev);
> > > > > > > >
> > > > > > > >       /* Hardware access */
> > > > > > > >
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > > > >
> > > > > > > > (plus error handling).
> > > > > > > >
> > > > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > > > the above becomes
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >
> > > > > > > > instead of having to power up the device just in case !PM.
> > > > > > > >
> > > > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > > > most of the drivers.
> > > >
> > > > Does the former work on ACPI systems ?
> > >
> > > Yes (i.e. the one that was above the quoted text).
> > >
> > > >
> > > > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > > > >
> > > > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > > > >
> > > > > > Not through RPM I suppose ?
> > > > >
> > > > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > > > dev_pm_domain_attach() called in i2c_device_probe()).
> > > >
> > > > How can we fix this ? It may have made sense a long time ago, but it's
> > > > making RPM handling way too difficult in I2C drivers now. We need
> > > > something better instead of continuing to rely on cargo-cult for probe
> > > > functions. Most drivers are broken.
> > >
> > > Some could be broken, there's no question of that. A lot of drivers support
> > > either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> > > that number is probably increasing constantly for the same devices are used
> > > on both.
> > >
> > > Then there are drivers that prefer not powering on the device in probe (see
> > > <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
> > > it gets complicated to support all the combinatios of DT/ACPI (with or
> > > without the flag / property for waiving powering device on for probe) and
> > > CONFIG_PM enabled/disabled.
> > >
> > > What I think could be done to add a flag for drivers that handle power on
> > > their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> > > works. Right now it expects a property on the device but that check could
> > > be moved to existing drivers using the flag. Not many drivers are currently
> > > using the flag. I think this would simplify driver implementation as both
> > > firmware interfaces would work the same way in this respect.
> > >
> > > You'd have to change one driver at a time, and people should be encouraged
> > > to write new drivers with that flag. Or add the flag to all existing
> > > drivers and not accept new ones with it.
> > >
> > > These devices I think are all I涎 but my understanding is that such
> > > differences exist elsewhere in the kernel, too. If they are to be
> > > addressed, it would probably be best to have a unified approach towards it.
> > >
> > > Added a few more people and lists to cc.
> >
> > + Hidenori from my team for visibility.
> >
> > I think we may want to take a step back and first define the problem
> > itself. To do that, let's take a look separately at DT and ACPI cases
> > (is platform data still relevant? are there any other firmware
> > interfaces that deal with I2C devices?).
> > For simplicity, let's forget about the ACPI waived power on in probe.
> >
> > DT:
> >  1) hardware state unknown when probe is called
> >  2) claim any independently managed resources (e.g. GPIOs)
> >  3) enable runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) enable any independently controlled resources (e.g. reset GPIO)

A small precision here, the resource handling is usually done in the
runtime PM resume/suspend handlers.

> >     c) [do access]
> >     d) disable any independently controlled resources
> >     e) runtime PM put
> >  5) after probe returns, regulators, clocks (and other similarly
> >     managed resources) would be force disabled if their enable count is 0
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > ACPI:
> >  1) hardware state is active when probe is called
> >  2) [n/a]
> >  3) tell runtime PM framework that the state is active and then enable
> >     runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) [n/a]
> >     c) [do access]
> >     d) [n/a]
> >     e) runtime PM put
> >  5) [n/a]
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > It seems like the relevant difference here is that for ACPI, the
> > driver needs to know that the initial state is active and also relay
> > this knowledge to the runtime PM subsystem. If we could make the ACPI
> > PM domain work the same way as regulators and clocks and eventually
> > power off some time later when the enable count is 0, then perhaps we
> > could avoid the problem in the first place?

Two additional questions if we're brainstorming this:

- Why is the I2C device hardware state active when probe is called, and
  would there be a way to change that (that is, beside the obvious issue
  that the transition could be painful, are there any other reasons to
  keep the status quo) ?

- If we have to keep this difference between the ACPI and DT models, how
  can we handle them in core code instead of drivers ? In particular,
  how could code core inform the RPM framework about the initial device
  state instead of leaving it to the driver ?

There's large set of RPM-related calls that have to be performed at
probe time in a very specific order, interleaved with manual power
handling. That is way over the threshold of what can be reasonably
expected from driver developers. I don't care much how it's done, but
this has to be dumbed down to make it dead simple in drivers.
  

Hi Tomasz,

On Mon, Aug 01, 2022 at 04:23:54PM +0900, Tomasz Figa wrote:
> [Fixed Jacopo's email address.]
> 
> On Mon, Aug 1, 2022 at 4:17 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Thu, Apr 14, 2022 at 6:30 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Laurent,
> > >
> > > On Tue, Mar 29, 2022 at 04:02:54PM +0300, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, Mar 14, 2022 at 11:11:18PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 14, 2022 at 10:05:37PM +0200, Laurent Pinchart wrote:
> > > > > ...
> > > > > > > > Yes, after reading the version register (or doing any other harware
> > > > > > > > access). Actually the full code would be
> > > > > > > >
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_resume_and_get(dev);
> > > > > > > >
> > > > > > > >       /* Hardware access */
> > > > > > > >
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >       pm_runtime_put_autosuspend(dev);
> > > > > > > >
> > > > > > > > (plus error handling).
> > > > > > > >
> > > > > > > > If the probe function doesn't need to access the hardware, then
> > > > > > > > the above becomes
> > > > > > > >
> > > > > > > >       pm_runtime_enable(dev);
> > > > > > > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > > > > >       pm_runtime_use_autosuspend(dev);
> > > > > > > >
> > > > > > > > instead of having to power up the device just in case !PM.
> > > > > > > >
> > > > > > > > > Also the latter only works on DT-based systems so it's not an option for
> > > > > > > > > most of the drivers.
> > > >
> > > > Does the former work on ACPI systems ?
> > >
> > > Yes (i.e. the one that was above the quoted text).
> > >
> > > >
> > > > > > > > How so, what's wrong with the above for ACPI-based system ?
> > > > > > >
> > > > > > > I涎 devices are already powered on for probe on ACPI based systems.
> > > > > >
> > > > > > Not through RPM I suppose ?
> > > > >
> > > > > Runtime PM isn't involved, this takes place in the ACPI framework (via
> > > > > dev_pm_domain_attach() called in i2c_device_probe()).
> > > >
> > > > How can we fix this ? It may have made sense a long time ago, but it's
> > > > making RPM handling way too difficult in I2C drivers now. We need
> > > > something better instead of continuing to rely on cargo-cult for probe
> > > > functions. Most drivers are broken.
> > >
> > > Some could be broken, there's no question of that. A lot of drivers support
> > > either ACPI or DT, too, so not _that_ many need to work with both. Albeit
> > > that number is probably increasing constantly for the same devices are used
> > > on both.
> > >
> > > Then there are drivers that prefer not powering on the device in probe (see
> > > <URL:https://lore.kernel.org/linux-acpi/20210210230800.30291-2-sakari.ailus@linux.intel.com/T/>),
> > > it gets complicated to support all the combinatios of DT/ACPI (with or
> > > without the flag / property for waiving powering device on for probe) and
> > > CONFIG_PM enabled/disabled.
> > >
> > > What I think could be done to add a flag for drivers that handle power on
> > > their own, or perhaps rather change how I2C_DRV_ACPI_WAIVE_D0_PROBE flag
> > > works. Right now it expects a property on the device but that check could
> > > be moved to existing drivers using the flag. Not many drivers are currently
> > > using the flag. I think this would simplify driver implementation as both
> > > firmware interfaces would work the same way in this respect.
> > >
> > > You'd have to change one driver at a time, and people should be encouraged
> > > to write new drivers with that flag. Or add the flag to all existing
> > > drivers and not accept new ones with it.
> > >
> > > These devices I think are all I涎 but my understanding is that such
> > > differences exist elsewhere in the kernel, too. If they are to be
> > > addressed, it would probably be best to have a unified approach towards it.
> > >
> > > Added a few more people and lists to cc.
> >
> > + Hidenori from my team for visibility.
> >
> > I think we may want to take a step back and first define the problem
> > itself. To do that, let's take a look separately at DT and ACPI cases
> > (is platform data still relevant? are there any other firmware
> > interfaces that deal with I2C devices?).
> > For simplicity, let's forget about the ACPI waived power on in probe.
> >
> > DT:
> >  1) hardware state unknown when probe is called
> >  2) claim any independently managed resources (e.g. GPIOs)
> >  3) enable runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) enable any independently controlled resources (e.g. reset GPIO)
> >     c) [do access]
> >     d) disable any independently controlled resources
> >     e) runtime PM put
> >  5) after probe returns, regulators, clocks (and other similarly
> > managed resources) would be force disabled if their enable count is 0
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > ACPI:
> >  1) hardware state is active when probe is called

This isn't a property of ACPI as such, but rather what I²C core does
to ACPI devices before calling probe (or after remove).

> >  2) [n/a]
> >  3) tell runtime PM framework that the state is active and then enable
> > runtime PM
> >  4) if driver wants to access the hardware:
> >     a) runtime PM get
> >     b) [n/a]
> >     c) [do access]
> >     d) [n/a]
> >     e) runtime PM put
> >  5) [n/a]
> >  6) hardware state is off (after the runtime PM state settles)
> >
> > It seems like the relevant difference here is that for ACPI, the
> > driver needs to know that the initial state is active and also relay
> > this knowledge to the runtime PM subsystem. If we could make the ACPI
> > PM domain work the same way as regulators and clocks and eventually
> > power off some time later when the enable count is 0, then perhaps we
> > could avoid the problem in the first place?