Hi Akinobu, thanks for v3 On Mon, Apr 23, 2018 at 12:56:13AM +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. > > 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 <[email protected]> > Cc: Laurent Pinchart <[email protected]> > Cc: Hans Verkuil <[email protected]> > Cc: Sakari Ailus <[email protected]> > Cc: Mauro Carvalho Chehab <[email protected]> > Signed-off-by: Akinobu Mita <[email protected]> > --- > * v3 > - Rename mutex name from power_lock to lock > - Add warning for duplicated s_power call > > drivers/media/i2c/ov772x.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 95c1c95d..8c0b850 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 lock; > + int power_count; > #ifdef CONFIG_MEDIA_CONTROLLER > struct media_pad pad; > #endif > @@ -871,9 +874,26 @@ 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->lock); > + > + /* 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); > + > + if (!ret) { > + /* Update the power count. */ > + priv->power_count += on ? 1 : -1; > + WARN(priv->power_count < 0, "Unbalanced power count\n"); > + WARN(priv->power_count > 1, "Duplicated s_power call\n");
The first time you receive a power on (on == 1, power_count == 0)
you'll have:
if (0 == !1)
ret = ov772x_power_on()
if (!0)
power_count += 1;
If power management is 'nested' you could receive a second,
unexpected, power on (on == 1, power_count == 1)
if (1 == !1)
if (!0)
power_count += 1;
Now power_count == 2, if you now receive a power off, you're going to
ignore it:
if (2 == !0)
ret = ov772x_power_off();
if (!0)
power_count += -1;
Again you now receive a new power off, and you're going to issue that
one.
if (1 == !0)
ret = ov772x_power_off();
if (!0)
power_count += -1;
So this seems correct to me!
If I were you, I would simplify this as:
power_count += on ? 1 : -1;
if (power_count == !!on)
return on ? ov772x_power_on(priv) :
ov772x_power_off(priv);
WARN_ON(power_count < 0 || power_count > 1);
return 0;
which, if I'm not wrong expands to:
power_count = 0 ; on = 1
power_count += 1;
if (1 == !!1)
return ov772x_power_on(priv);
power_count = 1 ; on = 1
power_count += 1;
if (2 == !!1)
WARN_ON(2 > 1)
power_count = 2 ; on = 0
power_count -= 1;
if (1 == !!0)
if (1 > 1 || 1 < 0)
power_count = 1; on = 0
power_count -= 1;
if (0 == !!0)
return ov772_power_off(priv);
power_count = 0 ; on = 0
power_count -= 1;
if (-1 == !!0)
WARN_ON(-1 < 0)
But if you have seen that pattern in other drivers, and maybe already
tested your one, I'm fine with either of them.
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> + }
> +
> + mutex_unlock(&priv->lock);
>
> - return on ? ov772x_power_on(priv) :
> - ov772x_power_off(priv);
> + return ret;
> }
>
> static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
> @@ -1303,6 +1323,7 @@ static int ov772x_probe(struct i2c_client *client,
> return -ENOMEM;
>
> priv->info = client->dev.platform_data;
> + mutex_init(&priv->lock);
>
> v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> v4l2_ctrl_handler_init(&priv->hdl, 3);
> @@ -1313,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)) {
> @@ -1362,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->lock);
>
> return ret;
> }
> @@ -1376,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->lock);
>
> return 0;
> }
> --
> 2.7.4
>
signature.asc
Description: PGP signature
