Hi Akinobu, On Mon, Apr 16, 2018 at 11:51:49AM +0900, Akinobu Mita wrote: > Depending on the v4l2 driver, calling s_power() could be nested. So the > actual transitions between power saving mode and normal operation mode > should only happen at the first power on and the last power off.
What do you mean with 'nested' ?
My understanding is that:
- if a sensor driver is mc compliant, it's s_power is called from
v4l2_mc.c pipeline_pm_power_one() only when the power state changes
- if a sensor driver is not mc compliant, the s_power routine is
called from the platform driver, and it should not happen that it is
called twice with the same power state
- if a sensor implements subdev's internal operations open and close
it may call it's own s_power routines from there, and it can be
opened more that once.
My understanding is that this driver s_power routines are only called
from platform drivers, and they -should- be safe.
Although, I'm not against this protection completely. Others might be,
though.
>
> This adds an s_power() nesting counter and updates the power state if the
> counter is modified from 0 to != 0 or from != 0 to 0.
>
> Cc: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Hans Verkuil <hans.verk...@cisco.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.m...@gmail.com>
> ---
> * v2
> - New patch
>
> drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 4245a46..2cd6e85 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -424,6 +424,9 @@ struct ov772x_priv {
> /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> unsigned short band_filter;
> unsigned int fps;
> + /* lock to protect power_count */
> + struct mutex power_lock;
> + int power_count;
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> @@ -871,9 +874,25 @@ static int ov772x_power_off(struct ov772x_priv *priv)
> static int ov772x_s_power(struct v4l2_subdev *sd, int on)
> {
> struct ov772x_priv *priv = to_ov772x(sd);
> + int ret = 0;
> +
> + mutex_lock(&priv->power_lock);
>
> - return on ? ov772x_power_on(priv) :
> - ov772x_power_off(priv);
> + /* If the power count is modified from 0 to != 0 or from != 0 to 0,
> + * update the power state.
> + */
> + if (priv->power_count == !on)
> + ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
Just release the mutex and return 0 if (power_count == on)
The code will be more readable imho.
> +
> + if (!ret) {
> + /* Update the power count. */
> + priv->power_count += on ? 1 : -1;
> + WARN_ON(priv->power_count < 0);
> + }
> +
> + mutex_unlock(&priv->power_lock);
> +
> + return ret;
> }
>
> static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
> @@ -1303,6 +1322,7 @@ static int ov772x_probe(struct i2c_client *client,
> return -ENOMEM;
>
> priv->info = client->dev.platform_data;
> + mutex_init(&priv->power_lock);
>
> v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1314,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
> v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
> priv->subdev.ctrl_handler = &priv->hdl;
> - if (priv->hdl.error)
> - return priv->hdl.error;
> + if (priv->hdl.error) {
> + ret = priv->hdl.error;
> + goto error_mutex_destroy;
> + }
>
> priv->clk = clk_get(&client->dev, "xclk");
> if (IS_ERR(priv->clk)) {
> @@ -1363,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
> clk_put(priv->clk);
> error_ctrl_free:
> v4l2_ctrl_handler_free(&priv->hdl);
> +error_mutex_destroy:
> + mutex_destroy(&priv->power_lock);
mutex_destroy() does nothing most of the time. It won't hurt though.
Thanks
j
>
> return ret;
> }
> @@ -1377,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
> gpiod_put(priv->pwdn_gpio);
> v4l2_async_unregister_subdev(&priv->subdev);
> v4l2_ctrl_handler_free(&priv->hdl);
> + mutex_destroy(&priv->power_lock);
>
> return 0;
> }
> --
> 2.7.4
>
signature.asc
Description: PGP signature
