[v3,13/29] media: ov2680: Drop is_enabled flag
Commit Message
With runtime-pm it is guaranteed that ov2680_power_on() and
ov2680_power_off() will always be called in a balanced way;
and the is_enabled check in ov2680_s_ctrl() can be replaced
by checking the runtime-suspend state.
So there is no more need for the is_enabled flag, remove it.
While at it also make sure that flip control changes while
suspended still lead to the bayer-order getting updated so
that get_fmt returns the correct bayer-order.
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/i2c/ov2680.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
Comments
Hi Hans
On 27/06/2023 15:18, Hans de Goede wrote:
> With runtime-pm it is guaranteed that ov2680_power_on() and
> ov2680_power_off() will always be called in a balanced way;
> and the is_enabled check in ov2680_s_ctrl() can be replaced
> by checking the runtime-suspend state.
>
> So there is no more need for the is_enabled flag, remove it.
>
> While at it also make sure that flip control changes while
> suspended still lead to the bayer-order getting updated so
> that get_fmt returns the correct bayer-order.
I think that this could benefit from a comment in the code - it's a departure from the usual and
looks a bit strange. With that added:
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/i2c/ov2680.c | 36 ++++++++++++++++++------------------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 56aaf67c1d82..b7c23286700e 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -100,7 +100,6 @@ struct ov2680_dev {
> struct gpio_desc *pwdn_gpio;
> struct mutex lock; /* protect members */
>
> - bool is_enabled;
> bool is_streaming;
>
> struct ov2680_ctrls ctrls;
> @@ -312,14 +311,9 @@ static int ov2680_stream_disable(struct ov2680_dev *sensor)
>
> static int ov2680_power_off(struct ov2680_dev *sensor)
> {
> - if (!sensor->is_enabled)
> - return 0;
> -
> clk_disable_unprepare(sensor->xvclk);
> ov2680_power_down(sensor);
> regulator_bulk_disable(OV2680_NUM_SUPPLIES, sensor->supplies);
> - sensor->is_enabled = false;
> -
> return 0;
> }
>
> @@ -327,9 +321,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
> {
> int ret;
>
> - if (sensor->is_enabled)
> - return 0;
> -
> ret = regulator_bulk_enable(OV2680_NUM_SUPPLIES, sensor->supplies);
> if (ret < 0) {
> dev_err(sensor->dev, "failed to enable regulators: %d\n", ret);
> @@ -353,8 +344,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
> if (ret < 0)
> goto err_disable_regulators;
>
> - sensor->is_enabled = true;
> -
> return 0;
>
> err_disable_regulators:
> @@ -541,26 +530,37 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> struct ov2680_dev *sensor = to_ov2680_dev(sd);
> + int ret;
>
> - if (!sensor->is_enabled)
> + /* Only apply changes to the controls if the device is powered up */
> + if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
> + ov2680_set_bayer_order(sensor, &sensor->fmt);
> return 0;
> + }
>
> switch (ctrl->id) {
> case V4L2_CID_GAIN:
> - return ov2680_gain_set(sensor, ctrl->val);
> + ret = ov2680_gain_set(sensor, ctrl->val);
> + break;
> case V4L2_CID_EXPOSURE:
> - return ov2680_exposure_set(sensor, ctrl->val);
> + ret = ov2680_exposure_set(sensor, ctrl->val);
> + break;
> case V4L2_CID_VFLIP:
> - return ov2680_set_vflip(sensor, ctrl->val);
> + ret = ov2680_set_vflip(sensor, ctrl->val);
> + break;
> case V4L2_CID_HFLIP:
> - return ov2680_set_hflip(sensor, ctrl->val);
> + ret = ov2680_set_hflip(sensor, ctrl->val);
> + break;
> case V4L2_CID_TEST_PATTERN:
> - return ov2680_test_pattern_set(sensor, ctrl->val);
> + ret = ov2680_test_pattern_set(sensor, ctrl->val);
> + break;
> default:
> + ret = -EINVAL;
> break;
> }
>
> - return -EINVAL;
> + pm_runtime_put(sensor->sd.dev);
> + return ret;
> }
>
> static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
@@ -100,7 +100,6 @@ struct ov2680_dev {
struct gpio_desc *pwdn_gpio;
struct mutex lock; /* protect members */
- bool is_enabled;
bool is_streaming;
struct ov2680_ctrls ctrls;
@@ -312,14 +311,9 @@ static int ov2680_stream_disable(struct ov2680_dev *sensor)
static int ov2680_power_off(struct ov2680_dev *sensor)
{
- if (!sensor->is_enabled)
- return 0;
-
clk_disable_unprepare(sensor->xvclk);
ov2680_power_down(sensor);
regulator_bulk_disable(OV2680_NUM_SUPPLIES, sensor->supplies);
- sensor->is_enabled = false;
-
return 0;
}
@@ -327,9 +321,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
{
int ret;
- if (sensor->is_enabled)
- return 0;
-
ret = regulator_bulk_enable(OV2680_NUM_SUPPLIES, sensor->supplies);
if (ret < 0) {
dev_err(sensor->dev, "failed to enable regulators: %d\n", ret);
@@ -353,8 +344,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
if (ret < 0)
goto err_disable_regulators;
- sensor->is_enabled = true;
-
return 0;
err_disable_regulators:
@@ -541,26 +530,37 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
struct ov2680_dev *sensor = to_ov2680_dev(sd);
+ int ret;
- if (!sensor->is_enabled)
+ /* Only apply changes to the controls if the device is powered up */
+ if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
+ ov2680_set_bayer_order(sensor, &sensor->fmt);
return 0;
+ }
switch (ctrl->id) {
case V4L2_CID_GAIN:
- return ov2680_gain_set(sensor, ctrl->val);
+ ret = ov2680_gain_set(sensor, ctrl->val);
+ break;
case V4L2_CID_EXPOSURE:
- return ov2680_exposure_set(sensor, ctrl->val);
+ ret = ov2680_exposure_set(sensor, ctrl->val);
+ break;
case V4L2_CID_VFLIP:
- return ov2680_set_vflip(sensor, ctrl->val);
+ ret = ov2680_set_vflip(sensor, ctrl->val);
+ break;
case V4L2_CID_HFLIP:
- return ov2680_set_hflip(sensor, ctrl->val);
+ ret = ov2680_set_hflip(sensor, ctrl->val);
+ break;
case V4L2_CID_TEST_PATTERN:
- return ov2680_test_pattern_set(sensor, ctrl->val);
+ ret = ov2680_test_pattern_set(sensor, ctrl->val);
+ break;
default:
+ ret = -EINVAL;
break;
}
- return -EINVAL;
+ pm_runtime_put(sensor->sd.dev);
+ return ret;
}
static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {