diff mbox series

[v3,13/29] media: ov2680: Drop is_enabled flag

Message ID 20230627131830.54601-14-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand

Commit Message

Hans de Goede June 27, 2023, 1:18 p.m. UTC 
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

Dan Scally June 27, 2023, 2:50 p.m. UTC | #1 
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 = {
diff mbox series

Patch

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 = {