[PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[Niklas Söderlund]

Hello,

This series started as an effort to fix issues with querystd. To do that
it turned out the whole drivers design around how it controls the power
to the video decoder block inside the chip had to be reworked. As a
bonus this works removes the now deprecated .s_power callback from
adv7180.

The adv7180 drivers comes from a time before media controller and all
operation callbacks are, more or less, designed around the concept that
a video device is the only user-space facing API. In that world a vdev
would attached the subdevice, call .s_power and then perform format
configuration using the other operation callbacks and then start
streaming with .s_stream. Needles to say this mode of operation don't
work well with media controller where the subdevices itself have a
user-space API exposed thru a subdev device.

The initial problem I tried to solve (querystd) was that it stopped
functioning as expected after the subdev had been used to stream once
(.s_power(1), .s_power(0)). As it turns out different variants of the
adv7180 device have different reset beaver for if its video decoder
block is left running or powered off. On my device it was left running
so querystd functioned the first time, but not after the video decoder
had been switched off once by .s_power(0).

I first tried to fix this by introducing proper PM handling in the
driver to be able to remove the .s_power callback. I quickly learnt the
power on/off logic happening in the driver had noting to do with
controlling power to the chip itself, but to control if the chips video
decoder block was turned off.

When this block is powered on the device process video data, if there is
a video source else it free runs. However when the block is turned off
the device can still be configured, in fact some configuration requires
it to be off.

For this reason I dropped the effort to add proper PM handling and
treated the decoder power as a stream on/off switch. I still think
proper PM handling would be beneficial for this driver but to not
explode this already large series I left that for another time. Solving
the issue around .s_power will make that work easier as well as other
task such as converting to the v4l2_subdev active state API.

Patch 1/11 just moves code around to make the consecutive changes easier
to read. Patch 2/11 fix a locking issues when suspending the device.
Patch 3/11 and 4/11 improves the locking design to prepare to improve
the driver.

Patch 5/11 make sure the device controls are always programmed after the
device have been reset, fixing a possible issue when the device where
resumed from system sleep.

Patches 6/11, 7/11 and 8/11 is the real change where the .s_power
callback is reworked to fit the design of .s_stream instead.

And finally patch 9/11, 10/11 and 11/11 removes programming of the
device from operation callbacks and solves the issue with querystd.

The work is tested on R-Car M2 together with a ADV7180 device.

See individual patches for changelog.

Niklas Söderlund (11):
  media: adv7180: Move adv7180_set_power() and init_device()
  media: adv7180: Add missing lock in suspend callback
  media: adv7180: Move state mutex handling outside init_device()
  media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking
  media: adv7180: Setup controls every time the device is reset
  media: adv7180: Power down decoder when configuring the device
  media: adv7180: Split device initialization and reset
  media: adv7180: Remove the s_power callback
  media: adv7180: Do not write format to device in set_fmt
  media: adv7180: Only validate format in s_std
  media: adv7180: Only validate format in querystd

 drivers/media/i2c/adv7180.c | 338 +++++++++++++++++++-----------------
 1 file changed, 174 insertions(+), 164 deletions(-)

-- 
2.51.0

[PATCH v2 01/11] media: adv7180: Move adv7180_set_power() and init_device()

[Niklas Söderlund]

Move the two functions adv7180_set_power() and init_device() earlier in
the file so they in future changes can be used from .querystd and
.s_stream as the driver is reworked to drop the usage of .s_power.

While at it fix two style issues in init_device() that checkpatch
complains about.

  - Two cases of indentation issues for function arguments split over
    multiple lines.

  - The repetition of the word 'interrupts' in a comment.

Apart from these style fixes the functions are moved verbatim and there
are no functional changes.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv7180.c | 176 ++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 88 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 5d90b8ab9b6d..ef63b0ee9b8d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -274,6 +274,38 @@ static int adv7180_vpp_write(struct adv7180_state *state, unsigned int reg,
 	return i2c_smbus_write_byte_data(state->vpp_client, reg, value);
 }
 
+static int adv7180_set_power(struct adv7180_state *state, bool on)
+{
+	u8 val;
+	int ret;
+
+	if (on)
+		val = ADV7180_PWR_MAN_ON;
+	else
+		val = ADV7180_PWR_MAN_OFF;
+
+	ret = adv7180_write(state, ADV7180_REG_PWR_MAN, val);
+	if (ret)
+		return ret;
+
+	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
+		if (on) {
+			adv7180_csi_write(state, 0xDE, 0x02);
+			adv7180_csi_write(state, 0xD2, 0xF7);
+			adv7180_csi_write(state, 0xD8, 0x65);
+			adv7180_csi_write(state, 0xE0, 0x09);
+			adv7180_csi_write(state, 0x2C, 0x00);
+			if (state->field == V4L2_FIELD_NONE)
+				adv7180_csi_write(state, 0x1D, 0x80);
+			adv7180_csi_write(state, 0x00, 0x00);
+		} else {
+			adv7180_csi_write(state, 0x00, 0x80);
+		}
+	}
+
+	return 0;
+}
+
 static v4l2_std_id adv7180_std_to_v4l2(u8 status1)
 {
 	/* in case V4L2_IN_ST_NO_SIGNAL */
@@ -514,38 +546,6 @@ static void adv7180_set_reset_pin(struct adv7180_state *state, bool on)
 	}
 }
 
-static int adv7180_set_power(struct adv7180_state *state, bool on)
-{
-	u8 val;
-	int ret;
-
-	if (on)
-		val = ADV7180_PWR_MAN_ON;
-	else
-		val = ADV7180_PWR_MAN_OFF;
-
-	ret = adv7180_write(state, ADV7180_REG_PWR_MAN, val);
-	if (ret)
-		return ret;
-
-	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
-		if (on) {
-			adv7180_csi_write(state, 0xDE, 0x02);
-			adv7180_csi_write(state, 0xD2, 0xF7);
-			adv7180_csi_write(state, 0xD8, 0x65);
-			adv7180_csi_write(state, 0xE0, 0x09);
-			adv7180_csi_write(state, 0x2C, 0x00);
-			if (state->field == V4L2_FIELD_NONE)
-				adv7180_csi_write(state, 0x1D, 0x80);
-			adv7180_csi_write(state, 0x00, 0x00);
-		} else {
-			adv7180_csi_write(state, 0x00, 0x80);
-		}
-	}
-
-	return 0;
-}
-
 static int adv7180_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct adv7180_state *state = to_state(sd);
@@ -874,6 +874,62 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
+static int init_device(struct adv7180_state *state)
+{
+	int ret;
+
+	mutex_lock(&state->mutex);
+
+	adv7180_set_power_pin(state, true);
+	adv7180_set_reset_pin(state, false);
+
+	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
+	usleep_range(5000, 10000);
+
+	ret = state->chip_info->init(state);
+	if (ret)
+		goto out_unlock;
+
+	ret = adv7180_program_std(state);
+	if (ret)
+		goto out_unlock;
+
+	adv7180_set_field_mode(state);
+
+	/* register for interrupts */
+	if (state->irq > 0) {
+		/* config the Interrupt pin to be active low */
+		ret = adv7180_write(state, ADV7180_REG_ICONF1,
+				    ADV7180_ICONF1_ACTIVE_LOW |
+				    ADV7180_ICONF1_PSYNC_ONLY);
+		if (ret < 0)
+			goto out_unlock;
+
+		ret = adv7180_write(state, ADV7180_REG_IMR1, 0);
+		if (ret < 0)
+			goto out_unlock;
+
+		ret = adv7180_write(state, ADV7180_REG_IMR2, 0);
+		if (ret < 0)
+			goto out_unlock;
+
+		/* enable AD change interrupts */
+		ret = adv7180_write(state, ADV7180_REG_IMR3,
+				    ADV7180_IRQ3_AD_CHANGE);
+		if (ret < 0)
+			goto out_unlock;
+
+		ret = adv7180_write(state, ADV7180_REG_IMR4, 0);
+		if (ret < 0)
+			goto out_unlock;
+	}
+
+out_unlock:
+	mutex_unlock(&state->mutex);
+
+	return ret;
+}
+
 static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct adv7180_state *state = to_state(sd);
@@ -1343,62 +1399,6 @@ static const struct adv7180_chip_info adv7282_m_info = {
 	.select_input = adv7182_select_input,
 };
 
-static int init_device(struct adv7180_state *state)
-{
-	int ret;
-
-	mutex_lock(&state->mutex);
-
-	adv7180_set_power_pin(state, true);
-	adv7180_set_reset_pin(state, false);
-
-	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
-	usleep_range(5000, 10000);
-
-	ret = state->chip_info->init(state);
-	if (ret)
-		goto out_unlock;
-
-	ret = adv7180_program_std(state);
-	if (ret)
-		goto out_unlock;
-
-	adv7180_set_field_mode(state);
-
-	/* register for interrupts */
-	if (state->irq > 0) {
-		/* config the Interrupt pin to be active low */
-		ret = adv7180_write(state, ADV7180_REG_ICONF1,
-						ADV7180_ICONF1_ACTIVE_LOW |
-						ADV7180_ICONF1_PSYNC_ONLY);
-		if (ret < 0)
-			goto out_unlock;
-
-		ret = adv7180_write(state, ADV7180_REG_IMR1, 0);
-		if (ret < 0)
-			goto out_unlock;
-
-		ret = adv7180_write(state, ADV7180_REG_IMR2, 0);
-		if (ret < 0)
-			goto out_unlock;
-
-		/* enable AD change interrupts interrupts */
-		ret = adv7180_write(state, ADV7180_REG_IMR3,
-						ADV7180_IRQ3_AD_CHANGE);
-		if (ret < 0)
-			goto out_unlock;
-
-		ret = adv7180_write(state, ADV7180_REG_IMR4, 0);
-		if (ret < 0)
-			goto out_unlock;
-	}
-
-out_unlock:
-	mutex_unlock(&state->mutex);
-
-	return ret;
-}
-
 static int adv7180_probe(struct i2c_client *client)
 {
 	struct device_node *np = client->dev.of_node;
-- 
2.51.0

[PATCH v2 02/11] media: adv7180: Add missing lock in suspend callback

[Niklas Söderlund]

The adv7180_set_power() utilizes adv7180_write() which in turn requires
the state mutex to be held, take it before calling adv7180_set_power()
to avoid tripping a lockdep_assert_held().

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Take the lock in more locations that was ignored as later patches in
  the series reworked them and solved the issue in the rework process.
---
 drivers/media/i2c/adv7180.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index ef63b0ee9b8d..b7f175650bd0 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -813,6 +813,8 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		if (state->field != format->format.field) {
+			guard(mutex)(&state->mutex);
+
 			state->field = format->format.field;
 			adv7180_set_power(state, false);
 			adv7180_set_field_mode(state);
@@ -1549,6 +1551,8 @@ static int adv7180_suspend(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct adv7180_state *state = to_state(sd);
 
+	guard(mutex)(&state->mutex);
+
 	return adv7180_set_power(state, false);
 }
 
@@ -1562,6 +1566,8 @@ static int adv7180_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
+	guard(mutex)(&state->mutex);
+
 	ret = adv7180_set_power(state, state->powered);
 	if (ret)
 		return ret;
-- 
2.51.0

[PATCH v2 03/11] media: adv7180: Move state mutex handling outside init_device()

[Niklas Söderlund]

Future rework to get rid of .s_power requires the state mutex to be
held for multiple operations where initializing the device is one of
them.

Move lock handling outside init_device() but enforce the lock is held
with a lockdep_assert_held().

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv7180.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index b7f175650bd0..9dbd33c4a30c 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -880,7 +880,7 @@ static int init_device(struct adv7180_state *state)
 {
 	int ret;
 
-	mutex_lock(&state->mutex);
+	lockdep_assert_held(&state->mutex);
 
 	adv7180_set_power_pin(state, true);
 	adv7180_set_reset_pin(state, false);
@@ -890,11 +890,11 @@ static int init_device(struct adv7180_state *state)
 
 	ret = state->chip_info->init(state);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	ret = adv7180_program_std(state);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	adv7180_set_field_mode(state);
 
@@ -905,31 +905,28 @@ static int init_device(struct adv7180_state *state)
 				    ADV7180_ICONF1_ACTIVE_LOW |
 				    ADV7180_ICONF1_PSYNC_ONLY);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
 		ret = adv7180_write(state, ADV7180_REG_IMR1, 0);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
 		ret = adv7180_write(state, ADV7180_REG_IMR2, 0);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
 		/* enable AD change interrupts */
 		ret = adv7180_write(state, ADV7180_REG_IMR3,
 				    ADV7180_IRQ3_AD_CHANGE);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
 		ret = adv7180_write(state, ADV7180_REG_IMR4, 0);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 	}
 
-out_unlock:
-	mutex_unlock(&state->mutex);
-
-	return ret;
+	return 0;
 }
 
 static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
@@ -1479,7 +1476,9 @@ static int adv7180_probe(struct i2c_client *client)
 	if (ret)
 		goto err_free_ctrl;
 
+	mutex_lock(&state->mutex);
 	ret = init_device(state);
+	mutex_unlock(&state->mutex);
 	if (ret)
 		goto err_media_entity_cleanup;
 
@@ -1562,6 +1561,8 @@ static int adv7180_resume(struct device *dev)
 	struct adv7180_state *state = to_state(sd);
 	int ret;
 
+	guard(mutex)(&state->mutex);
+
 	ret = init_device(state);
 	if (ret < 0)
 		return ret;
-- 
2.51.0

[PATCH v2 04/11] media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking

[Niklas Söderlund]

Instead of handling the state lock ourself in .s_ctrl use the v4l2-ctrls
core to handle it for us. This will allow us later to use the unlocked
__v4l2_ctrl_handler_setup() in initialization code where the state lock
is already held.

Add a lockdep assert to demonstrate the mutex must be held when setting
controls.

There is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv7180.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 9dbd33c4a30c..7b0387151c3a 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -601,11 +601,11 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
 	struct adv7180_state *state = to_state(sd);
-	int ret = mutex_lock_interruptible(&state->mutex);
+	int ret = 0;
 	int val;
 
-	if (ret)
-		return ret;
+	lockdep_assert_held(&state->mutex);
+
 	val = ctrl->val;
 	switch (ctrl->id) {
 	case V4L2_CID_BRIGHTNESS:
@@ -647,7 +647,6 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = -EINVAL;
 	}
 
-	mutex_unlock(&state->mutex);
 	return ret;
 }
 
@@ -668,6 +667,7 @@ static const struct v4l2_ctrl_config adv7180_ctrl_fast_switch = {
 static int adv7180_init_controls(struct adv7180_state *state)
 {
 	v4l2_ctrl_handler_init(&state->ctrl_hdl, 4);
+	state->ctrl_hdl.lock = &state->mutex;
 
 	v4l2_ctrl_new_std(&state->ctrl_hdl, &adv7180_ctrl_ops,
 			  V4L2_CID_BRIGHTNESS, ADV7180_BRI_MIN,
-- 
2.51.0

Re: [PATCH v2 04/11] media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking

[Hans Verkuil]

On 28/08/2025 18:06, Niklas Söderlund wrote:
> Instead of handling the state lock ourself in .s_ctrl use the v4l2-ctrls
> core to handle it for us. This will allow us later to use the unlocked
> __v4l2_ctrl_handler_setup() in initialization code where the state lock
> is already held.
> 
> Add a lockdep assert to demonstrate the mutex must be held when setting
> controls.
> 
> There is no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/adv7180.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 9dbd33c4a30c..7b0387151c3a 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -601,11 +601,11 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
>  	struct adv7180_state *state = to_state(sd);
> -	int ret = mutex_lock_interruptible(&state->mutex);
> +	int ret = 0;
>  	int val;
>  
> -	if (ret)
> -		return ret;
> +	lockdep_assert_held(&state->mutex);
> +
>  	val = ctrl->val;
>  	switch (ctrl->id) {
>  	case V4L2_CID_BRIGHTNESS:
> @@ -647,7 +647,6 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = -EINVAL;
>  	}
>  
> -	mutex_unlock(&state->mutex);
>  	return ret;
>  }
>  
> @@ -668,6 +667,7 @@ static const struct v4l2_ctrl_config adv7180_ctrl_fast_switch = {
>  static int adv7180_init_controls(struct adv7180_state *state)
>  {
>  	v4l2_ctrl_handler_init(&state->ctrl_hdl, 4);
> +	state->ctrl_hdl.lock = &state->mutex;

While perfectly legal, I would really like to avoid drivers messing with internal
fields of the v4l2_ctrl_handler structure. I wondered why I hadn't noticed this
construct before, and it is primarily used in sensor drivers, which I typically
don't review.

What I would prefer to see is a new function: v4l2_ctrl_handler_init_with_mutex()
where the mutex pointer is passed as an extra argument.

And a static inline for the old function like this:

static inline int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
					 unsigned nr_of_controls_hint)
{
	mutex_init(&hdl->_lock);
	return v4l2_ctrl_handler_init_with_mutex(hdl, nr_of_controls_hint, &hdl->_lock);
}

(it's actually a bit more work due to LOCKDEP class handling)

If a driver uses v4l2_ctrl_handler_init_with_mutex then hdl->_lock is never inited
(and will typically be all zeroes), so any use of that lock will cause errors.

v4l2_ctrl_handler_init_with_mutex() could actually check if the mutex is != _lock
and zero _lock explicitly, clearly marking it as unused.

If you prefer to do this as a follow-up series (also updating existing drivers
that use this), then that would be fine.

Regards,

	Hans

>  
>  	v4l2_ctrl_new_std(&state->ctrl_hdl, &adv7180_ctrl_ops,
>  			  V4L2_CID_BRIGHTNESS, ADV7180_BRI_MIN,

[PATCH v2 05/11] media: adv7180: Setup controls every time the device is reset

[Niklas Söderlund]

The device initialization code resets the whole device, thus the initial
controls set at probe are lost as adv7180_init_controls() are called
before init_device(). Additionally the controls were never restored
after the device where reset coming back from suspend.

Solve this by separate the setup of the controls from the creation of
them, and always set them up when the device is reset.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
* Changes since v1
- Fix spelling in commit message.
---
 drivers/media/i2c/adv7180.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 7b0387151c3a..8409ee9acc4f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -700,7 +700,6 @@ static int adv7180_init_controls(struct adv7180_state *state)
 		v4l2_ctrl_handler_free(&state->ctrl_hdl);
 		return err;
 	}
-	v4l2_ctrl_handler_setup(&state->ctrl_hdl);
 
 	return 0;
 }
@@ -898,6 +897,8 @@ static int init_device(struct adv7180_state *state)
 
 	adv7180_set_field_mode(state);
 
+	__v4l2_ctrl_handler_setup(&state->ctrl_hdl);
+
 	/* register for interrupts */
 	if (state->irq > 0) {
 		/* config the Interrupt pin to be active low */
-- 
2.51.0

[PATCH v2 06/11] media: adv7180: Power down decoder when configuring the device

[Niklas Söderlund]

Some variants of the chip (ADV7180) have it's decoder powered up after
reset, while others (ADV7280, ADV7281, ADV7282, ADV7283) have it powered
down.

This is tracked by the feature flag ADV7180_FLAG_RESET_POWERED. At probe
this flag is used to initialize the state variable powered which keeps
track of if the decoder is powered on, or off, for the resume callback.

This however misses that the decoder needs to be powered off for some
configuration of the device to take hold. So for devices where it's left
on (ADV7180) the format configuration at probe time have little effect.
This worked as the .set_fmt callback powers down the decoder, updates
the format, and powers back on the decoder.

Before moving all configuration to .s_stream this needs to be fixed.
Instead of tracking if the decoder is powered on or off, use the
flag to determine if needs to be powered down after a reset to do the
configuration.

To keep the behavior consistent with the currents implementation switch
the decoder back on for devices where this is the reset behavior. The
primary reason for this is that if not done the first 35+ frames or so
of the capture session is garbage.

To keep the support of starting the decoder when resuming from sleep on
devices where the reset behavior is to start with the decoder powered
off, use the state variable streaming. If it is set the decoder was
powered on when the system suspended so we know to start it again when
resuming.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 8409ee9acc4f..0bc608291df7 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -214,7 +214,6 @@ struct adv7180_state {
 	struct gpio_desc	*pwdn_gpio;
 	struct gpio_desc	*rst_gpio;
 	v4l2_std_id		curr_norm;
-	bool			powered;
 	bool			streaming;
 	u8			input;
 
@@ -556,8 +555,6 @@ static int adv7180_s_power(struct v4l2_subdev *sd, int on)
 		return ret;
 
 	ret = adv7180_set_power(state, on);
-	if (ret == 0)
-		state->powered = on;
 
 	mutex_unlock(&state->mutex);
 	return ret;
@@ -887,6 +884,13 @@ static int init_device(struct adv7180_state *state)
 	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
 	usleep_range(5000, 10000);
 
+	/*
+	 * If the devices decoder is power on after reset, power off so the
+	 * device can be configured.
+	 */
+	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
+		adv7180_set_power(state, false);
+
 	ret = state->chip_info->init(state);
 	if (ret)
 		return ret;
@@ -927,6 +931,14 @@ static int init_device(struct adv7180_state *state)
 			return ret;
 	}
 
+	/*
+	 * If the devices decoder is power on after reset, restore the power
+	 * after configuration. This is to preserve the behavior of the driver,
+	 * not doing this result in the first 35+ frames captured being garbage.
+	 */
+	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
+		adv7180_set_power(state, true);
+
 	return 0;
 }
 
@@ -1457,10 +1469,7 @@ static int adv7180_probe(struct i2c_client *client)
 	state->irq = client->irq;
 	mutex_init(&state->mutex);
 	state->curr_norm = V4L2_STD_NTSC;
-	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
-		state->powered = true;
-	else
-		state->powered = false;
+
 	state->input = 0;
 	sd = &state->sd;
 	v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
@@ -1568,11 +1577,12 @@ static int adv7180_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	guard(mutex)(&state->mutex);
-
-	ret = adv7180_set_power(state, state->powered);
-	if (ret)
-		return ret;
+	/* If we where streaming when suspending, start decoder. */
+	if (state->streaming) {
+		ret = adv7180_set_power(state, true);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.51.0

Re: [PATCH v2 06/11] media: adv7180: Power down decoder when configuring the device

[Hans Verkuil]

On 28/08/2025 18:06, Niklas Söderlund wrote:
> Some variants of the chip (ADV7180) have it's decoder powered up after
> reset, while others (ADV7280, ADV7281, ADV7282, ADV7283) have it powered
> down.
> 
> This is tracked by the feature flag ADV7180_FLAG_RESET_POWERED. At probe
> this flag is used to initialize the state variable powered which keeps
> track of if the decoder is powered on, or off, for the resume callback.
> 
> This however misses that the decoder needs to be powered off for some
> configuration of the device to take hold. So for devices where it's left
> on (ADV7180) the format configuration at probe time have little effect.
> This worked as the .set_fmt callback powers down the decoder, updates
> the format, and powers back on the decoder.
> 
> Before moving all configuration to .s_stream this needs to be fixed.
> Instead of tracking if the decoder is powered on or off, use the
> flag to determine if needs to be powered down after a reset to do the
> configuration.
> 
> To keep the behavior consistent with the currents implementation switch
> the decoder back on for devices where this is the reset behavior. The
> primary reason for this is that if not done the first 35+ frames or so
> of the capture session is garbage.
> 
> To keep the support of starting the decoder when resuming from sleep on
> devices where the reset behavior is to start with the decoder powered
> off, use the state variable streaming. If it is set the decoder was
> powered on when the system suspended so we know to start it again when
> resuming.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv7180.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 8409ee9acc4f..0bc608291df7 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -214,7 +214,6 @@ struct adv7180_state {
>  	struct gpio_desc	*pwdn_gpio;
>  	struct gpio_desc	*rst_gpio;
>  	v4l2_std_id		curr_norm;
> -	bool			powered;
>  	bool			streaming;
>  	u8			input;
>  
> @@ -556,8 +555,6 @@ static int adv7180_s_power(struct v4l2_subdev *sd, int on)
>  		return ret;
>  
>  	ret = adv7180_set_power(state, on);
> -	if (ret == 0)
> -		state->powered = on;
>  
>  	mutex_unlock(&state->mutex);
>  	return ret;
> @@ -887,6 +884,13 @@ static int init_device(struct adv7180_state *state)
>  	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
>  	usleep_range(5000, 10000);
>  
> +	/*
> +	 * If the devices decoder is power on after reset, power off so the
> +	 * device can be configured.
> +	 */
> +	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
> +		adv7180_set_power(state, false);
> +
>  	ret = state->chip_info->init(state);
>  	if (ret)
>  		return ret;
> @@ -927,6 +931,14 @@ static int init_device(struct adv7180_state *state)
>  			return ret;
>  	}
>  
> +	/*
> +	 * If the devices decoder is power on after reset, restore the power
> +	 * after configuration. This is to preserve the behavior of the driver,
> +	 * not doing this result in the first 35+ frames captured being garbage.
> +	 */
> +	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
> +		adv7180_set_power(state, true);
> +
>  	return 0;
>  }
>  
> @@ -1457,10 +1469,7 @@ static int adv7180_probe(struct i2c_client *client)
>  	state->irq = client->irq;
>  	mutex_init(&state->mutex);
>  	state->curr_norm = V4L2_STD_NTSC;
> -	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
> -		state->powered = true;
> -	else
> -		state->powered = false;
> +
>  	state->input = 0;
>  	sd = &state->sd;
>  	v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> @@ -1568,11 +1577,12 @@ static int adv7180_resume(struct device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	guard(mutex)(&state->mutex);
> -
> -	ret = adv7180_set_power(state, state->powered);
> -	if (ret)
> -		return ret;
> +	/* If we where streaming when suspending, start decoder. */

where -> were

> +	if (state->streaming) {
> +		ret = adv7180_set_power(state, true);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }

[PATCH v2 07/11] media: adv7180: Split device initialization and reset

[Niklas Söderlund]

The two different tasks of resetting and initializing the devices, and
configured the video formats are grouped lumped together in a single
function. These two tasks are then only performed at probe time, or when
resuming from suspend. Configuration of formats are then done directly
by the IOCTL callbacks, such as .set_fmt.

Prepare for reworking the driver to only reset the device at probe and
resume, and configuring all video formats in .s_stream instead of in
each IOCTL callback by splitting the two tasks in two different
functions.

At this point there is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 0bc608291df7..ecec13fee49e 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -878,6 +878,23 @@ static int init_device(struct adv7180_state *state)
 
 	lockdep_assert_held(&state->mutex);
 
+	ret = adv7180_program_std(state);
+	if (ret)
+		return ret;
+
+	adv7180_set_field_mode(state);
+
+	__v4l2_ctrl_handler_setup(&state->ctrl_hdl);
+
+	return ret;
+}
+
+static int adv7180_reset_device(struct adv7180_state *state)
+{
+	int ret;
+
+	lockdep_assert_held(&state->mutex);
+
 	adv7180_set_power_pin(state, true);
 	adv7180_set_reset_pin(state, false);
 
@@ -895,14 +912,10 @@ static int init_device(struct adv7180_state *state)
 	if (ret)
 		return ret;
 
-	ret = adv7180_program_std(state);
+	ret = init_device(state);
 	if (ret)
 		return ret;
 
-	adv7180_set_field_mode(state);
-
-	__v4l2_ctrl_handler_setup(&state->ctrl_hdl);
-
 	/* register for interrupts */
 	if (state->irq > 0) {
 		/* config the Interrupt pin to be active low */
@@ -1487,7 +1500,7 @@ static int adv7180_probe(struct i2c_client *client)
 		goto err_free_ctrl;
 
 	mutex_lock(&state->mutex);
-	ret = init_device(state);
+	ret = adv7180_reset_device(state);
 	mutex_unlock(&state->mutex);
 	if (ret)
 		goto err_media_entity_cleanup;
@@ -1573,7 +1586,7 @@ static int adv7180_resume(struct device *dev)
 
 	guard(mutex)(&state->mutex);
 
-	ret = init_device(state);
+	ret = adv7180_reset_device(state);
 	if (ret < 0)
 		return ret;
 
-- 
2.51.0

[PATCH v2 08/11] media: adv7180: Remove the s_power callback

[Niklas Söderlund]

The s_power callback is used a bit oddly in adv7180, the callback
adv7180_set_power() do not control power to the chip itself, but
rather the power to the chips decoder.

When the decoder is powered the chip process video data, or if no video
source is available freeruns. When the decoder is off the device i2c
registers are still powered and the device can be configured.

In the current s_power implementation the device starts to transmit
video data as soon as it's powered, and the s_stream operation only
tracks if s_stream have been called or not.

The actual configuration of the device happens when the configuration
IOCTLs are called. Sometimes with very odd implementations where the
decoder have to first be power off, the device configured, and then
unconditionally power on, see adv7180_set_pad_format() for an example.

As a first step to remedy this remove the s_power callback altogether
and instead completely initialize the device from s_stream. Future work
will clean up the IOCTL callbacks that directly configures the device
that is also done by init_device().

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 43 ++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index ecec13fee49e..c8b63c985b69 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -545,21 +545,6 @@ static void adv7180_set_reset_pin(struct adv7180_state *state, bool on)
 	}
 }
 
-static int adv7180_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct adv7180_state *state = to_state(sd);
-	int ret;
-
-	ret = mutex_lock_interruptible(&state->mutex);
-	if (ret)
-		return ret;
-
-	ret = adv7180_set_power(state, on);
-
-	mutex_unlock(&state->mutex);
-	return ret;
-}
-
 static const char * const test_pattern_menu[] = {
 	"Single color",
 	"Color bars",
@@ -960,18 +945,29 @@ static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
 	struct adv7180_state *state = to_state(sd);
 	int ret;
 
-	/* It's always safe to stop streaming, no need to take the lock */
-	if (!enable) {
-		state->streaming = enable;
-		return 0;
-	}
-
 	/* Must wait until querystd released the lock */
-	ret = mutex_lock_interruptible(&state->mutex);
+	guard(mutex)(&state->mutex);
+
+	/*
+	 * Always power off the decoder even if streaming is to be enabled, the
+	 * decoder needs to be off for the device to be configured.
+	 */
+	ret = adv7180_set_power(state, false);
 	if (ret)
 		return ret;
+
+	if (enable) {
+		ret = init_device(state);
+		if (ret)
+			return ret;
+
+		ret = adv7180_set_power(state, true);
+		if (ret)
+			return ret;
+	}
+
 	state->streaming = enable;
-	mutex_unlock(&state->mutex);
+
 	return 0;
 }
 
@@ -1000,7 +996,6 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 };
 
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {
-	.s_power = adv7180_s_power,
 	.subscribe_event = adv7180_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
-- 
2.51.0

[PATCH v2 09/11] media: adv7180: Do not write format to device in set_fmt

[Niklas Söderlund]

The .set_fmt callback should not write the new format directly do the
device, it should only store it and have it applied by .s_stream.

The .s_stream callback already calls adv7180_set_field_mode() so it's
safe to remove programming of the device and just store the format and
have .s_stream apply it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv7180.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index c8b63c985b69..641cf698df5e 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -793,14 +793,7 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 	ret = adv7180_mbus_fmt(sd,  &format->format);
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		if (state->field != format->format.field) {
-			guard(mutex)(&state->mutex);
-
-			state->field = format->format.field;
-			adv7180_set_power(state, false);
-			adv7180_set_field_mode(state);
-			adv7180_set_power(state, true);
-		}
+		state->field = format->format.field;
 	} else {
 		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
 		*framefmt = format->format;
-- 
2.51.0

[PATCH v2 10/11] media: adv7180: Only validate format in s_std

[Niklas Söderlund]

The .s_std callback should not write the new format directly do the
device, it should only store it and have it applied by .s_stream.

As .s_stream already calls adv7180_program_std() all that is needed
is to check the standard is valid, and store it for .s_stream to
program.

While at it add a scoped guard to simplify the error handling.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 641cf698df5e..47112b43769d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -468,22 +468,18 @@ static int adv7180_program_std(struct adv7180_state *state)
 static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct adv7180_state *state = to_state(sd);
-	int ret = mutex_lock_interruptible(&state->mutex);
+	int ret;
 
-	if (ret)
-		return ret;
+	guard(mutex)(&state->mutex);
 
 	/* Make sure we can support this std */
 	ret = v4l2_std_to_adv7180(std);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	state->curr_norm = std;
 
-	ret = adv7180_program_std(state);
-out:
-	mutex_unlock(&state->mutex);
-	return ret;
+	return 0;
 }
 
 static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
-- 
2.51.0

[PATCH v2 11/11] media: adv7180: Only validate format in querystd

[Niklas Söderlund]

The .querystd callback should not program the device with the detected
standard, it should only report the standard to user-space. User-space
may then use .s_std to set the standard, if it wants to use it.

All that is required of .querystd is to setup the auto detection of
standards and report its findings.

While at it add some documentation on why this can't happen while
streaming and improve the error handling using a scoped guard.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 47112b43769d..ca0bdfa9dcda 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -388,32 +388,27 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
 static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
 {
 	struct adv7180_state *state = to_state(sd);
-	int err = mutex_lock_interruptible(&state->mutex);
-	if (err)
-		return err;
+	int ret;
 
-	if (state->streaming) {
-		err = -EBUSY;
-		goto unlock;
-	}
+	guard(mutex)(&state->mutex);
 
-	err = adv7180_set_video_standard(state,
-			ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
-	if (err)
-		goto unlock;
+	/*
+	 * We can't sample the standard if the device is streaming as that would
+	 * interfere with the capture session as the VID_SEL reg is touched.
+	 */
+	if (state->streaming)
+		return -EBUSY;
 
+	/* Set the standard to autodetect PAL B/G/H/I/D, NTSC J or SECAM */
+	ret = adv7180_set_video_standard(state,
+					 ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
+	if (ret)
+		return ret;
+
+	/* Allow some time for the autodetection to run. */
 	msleep(100);
-	__adv7180_status(state, NULL, std);
 
-	err = v4l2_std_to_adv7180(state->curr_norm);
-	if (err < 0)
-		goto unlock;
-
-	err = adv7180_set_video_standard(state, err);
-
-unlock:
-	mutex_unlock(&state->mutex);
-	return err;
+	return __adv7180_status(state, NULL, std);
 }
 
 static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
-- 
2.51.0

Re: [PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[Hans Verkuil]

On 28/08/2025 18:06, Niklas Söderlund wrote:
> Hello,
> 
> This series started as an effort to fix issues with querystd. To do that
> it turned out the whole drivers design around how it controls the power
> to the video decoder block inside the chip had to be reworked. As a
> bonus this works removes the now deprecated .s_power callback from
> adv7180.
> 
> The adv7180 drivers comes from a time before media controller and all
> operation callbacks are, more or less, designed around the concept that
> a video device is the only user-space facing API. In that world a vdev
> would attached the subdevice, call .s_power and then perform format
> configuration using the other operation callbacks and then start
> streaming with .s_stream. Needles to say this mode of operation don't
> work well with media controller where the subdevices itself have a
> user-space API exposed thru a subdev device.
> 
> The initial problem I tried to solve (querystd) was that it stopped
> functioning as expected after the subdev had been used to stream once
> (.s_power(1), .s_power(0)). As it turns out different variants of the
> adv7180 device have different reset beaver for if its video decoder
> block is left running or powered off. On my device it was left running
> so querystd functioned the first time, but not after the video decoder
> had been switched off once by .s_power(0).
> 
> I first tried to fix this by introducing proper PM handling in the
> driver to be able to remove the .s_power callback. I quickly learnt the
> power on/off logic happening in the driver had noting to do with
> controlling power to the chip itself, but to control if the chips video
> decoder block was turned off.
> 
> When this block is powered on the device process video data, if there is
> a video source else it free runs. However when the block is turned off
> the device can still be configured, in fact some configuration requires
> it to be off.
> 
> For this reason I dropped the effort to add proper PM handling and
> treated the decoder power as a stream on/off switch. I still think
> proper PM handling would be beneficial for this driver but to not
> explode this already large series I left that for another time. Solving
> the issue around .s_power will make that work easier as well as other
> task such as converting to the v4l2_subdev active state API.
> 
> Patch 1/11 just moves code around to make the consecutive changes easier
> to read. Patch 2/11 fix a locking issues when suspending the device.
> Patch 3/11 and 4/11 improves the locking design to prepare to improve
> the driver.
> 
> Patch 5/11 make sure the device controls are always programmed after the
> device have been reset, fixing a possible issue when the device where
> resumed from system sleep.
> 
> Patches 6/11, 7/11 and 8/11 is the real change where the .s_power
> callback is reworked to fit the design of .s_stream instead.
> 
> And finally patch 9/11, 10/11 and 11/11 removes programming of the
> device from operation callbacks and solves the issue with querystd.
> 
> The work is tested on R-Car M2 together with a ADV7180 device.
> 
> See individual patches for changelog.

This series looks good to me, other than the one typo and the
control handler kAPI issue, but that can be done in a follow-up
series.

If you want I can take this series, let me know.

Regards,

	Hans

> 
> Niklas Söderlund (11):
>   media: adv7180: Move adv7180_set_power() and init_device()
>   media: adv7180: Add missing lock in suspend callback
>   media: adv7180: Move state mutex handling outside init_device()
>   media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking
>   media: adv7180: Setup controls every time the device is reset
>   media: adv7180: Power down decoder when configuring the device
>   media: adv7180: Split device initialization and reset
>   media: adv7180: Remove the s_power callback
>   media: adv7180: Do not write format to device in set_fmt
>   media: adv7180: Only validate format in s_std
>   media: adv7180: Only validate format in querystd
> 
>  drivers/media/i2c/adv7180.c | 338 +++++++++++++++++++-----------------
>  1 file changed, 174 insertions(+), 164 deletions(-)
> 

Re: [PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[Niklas Söderlund]

Hi Hans,

Thanks for your review effort!

On 2025-09-03 10:46:11 +0200, Hans Verkuil wrote:
> On 28/08/2025 18:06, Niklas Söderlund wrote:
> > Hello,
> > 
> > This series started as an effort to fix issues with querystd. To do that
> > it turned out the whole drivers design around how it controls the power
> > to the video decoder block inside the chip had to be reworked. As a
> > bonus this works removes the now deprecated .s_power callback from
> > adv7180.
> > 
> > The adv7180 drivers comes from a time before media controller and all
> > operation callbacks are, more or less, designed around the concept that
> > a video device is the only user-space facing API. In that world a vdev
> > would attached the subdevice, call .s_power and then perform format
> > configuration using the other operation callbacks and then start
> > streaming with .s_stream. Needles to say this mode of operation don't
> > work well with media controller where the subdevices itself have a
> > user-space API exposed thru a subdev device.
> > 
> > The initial problem I tried to solve (querystd) was that it stopped
> > functioning as expected after the subdev had been used to stream once
> > (.s_power(1), .s_power(0)). As it turns out different variants of the
> > adv7180 device have different reset beaver for if its video decoder
> > block is left running or powered off. On my device it was left running
> > so querystd functioned the first time, but not after the video decoder
> > had been switched off once by .s_power(0).
> > 
> > I first tried to fix this by introducing proper PM handling in the
> > driver to be able to remove the .s_power callback. I quickly learnt the
> > power on/off logic happening in the driver had noting to do with
> > controlling power to the chip itself, but to control if the chips video
> > decoder block was turned off.
> > 
> > When this block is powered on the device process video data, if there is
> > a video source else it free runs. However when the block is turned off
> > the device can still be configured, in fact some configuration requires
> > it to be off.
> > 
> > For this reason I dropped the effort to add proper PM handling and
> > treated the decoder power as a stream on/off switch. I still think
> > proper PM handling would be beneficial for this driver but to not
> > explode this already large series I left that for another time. Solving
> > the issue around .s_power will make that work easier as well as other
> > task such as converting to the v4l2_subdev active state API.
> > 
> > Patch 1/11 just moves code around to make the consecutive changes easier
> > to read. Patch 2/11 fix a locking issues when suspending the device.
> > Patch 3/11 and 4/11 improves the locking design to prepare to improve
> > the driver.
> > 
> > Patch 5/11 make sure the device controls are always programmed after the
> > device have been reset, fixing a possible issue when the device where
> > resumed from system sleep.
> > 
> > Patches 6/11, 7/11 and 8/11 is the real change where the .s_power
> > callback is reworked to fit the design of .s_stream instead.
> > 
> > And finally patch 9/11, 10/11 and 11/11 removes programming of the
> > device from operation callbacks and solves the issue with querystd.
> > 
> > The work is tested on R-Car M2 together with a ADV7180 device.
> > 
> > See individual patches for changelog.
> 
> This series looks good to me, other than the one typo and the
> control handler kAPI issue, but that can be done in a follow-up
> series.
> 
> If you want I can take this series, let me know.

Thanks for pointing out the improvement in the kAPI, I agree with it.  
But as you point out I can do that in follow up work so I would be happy 
if you would take this series. Can you correct the typo while applying, 
or would you prefer I send an undated series?

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Niklas Söderlund (11):
> >   media: adv7180: Move adv7180_set_power() and init_device()
> >   media: adv7180: Add missing lock in suspend callback
> >   media: adv7180: Move state mutex handling outside init_device()
> >   media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking
> >   media: adv7180: Setup controls every time the device is reset
> >   media: adv7180: Power down decoder when configuring the device
> >   media: adv7180: Split device initialization and reset
> >   media: adv7180: Remove the s_power callback
> >   media: adv7180: Do not write format to device in set_fmt
> >   media: adv7180: Only validate format in s_std
> >   media: adv7180: Only validate format in querystd
> > 
> >  drivers/media/i2c/adv7180.c | 338 +++++++++++++++++++-----------------
> >  1 file changed, 174 insertions(+), 164 deletions(-)
> > 
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 00/11] media: adv7180: Improve the control over decoder power

[Hans Verkuil]

On 03/09/2025 16:35, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your review effort!
> 
> On 2025-09-03 10:46:11 +0200, Hans Verkuil wrote:
>> On 28/08/2025 18:06, Niklas Söderlund wrote:
>>> Hello,
>>>
>>> This series started as an effort to fix issues with querystd. To do that
>>> it turned out the whole drivers design around how it controls the power
>>> to the video decoder block inside the chip had to be reworked. As a
>>> bonus this works removes the now deprecated .s_power callback from
>>> adv7180.
>>>
>>> The adv7180 drivers comes from a time before media controller and all
>>> operation callbacks are, more or less, designed around the concept that
>>> a video device is the only user-space facing API. In that world a vdev
>>> would attached the subdevice, call .s_power and then perform format
>>> configuration using the other operation callbacks and then start
>>> streaming with .s_stream. Needles to say this mode of operation don't
>>> work well with media controller where the subdevices itself have a
>>> user-space API exposed thru a subdev device.
>>>
>>> The initial problem I tried to solve (querystd) was that it stopped
>>> functioning as expected after the subdev had been used to stream once
>>> (.s_power(1), .s_power(0)). As it turns out different variants of the
>>> adv7180 device have different reset beaver for if its video decoder
>>> block is left running or powered off. On my device it was left running
>>> so querystd functioned the first time, but not after the video decoder
>>> had been switched off once by .s_power(0).
>>>
>>> I first tried to fix this by introducing proper PM handling in the
>>> driver to be able to remove the .s_power callback. I quickly learnt the
>>> power on/off logic happening in the driver had noting to do with
>>> controlling power to the chip itself, but to control if the chips video
>>> decoder block was turned off.
>>>
>>> When this block is powered on the device process video data, if there is
>>> a video source else it free runs. However when the block is turned off
>>> the device can still be configured, in fact some configuration requires
>>> it to be off.
>>>
>>> For this reason I dropped the effort to add proper PM handling and
>>> treated the decoder power as a stream on/off switch. I still think
>>> proper PM handling would be beneficial for this driver but to not
>>> explode this already large series I left that for another time. Solving
>>> the issue around .s_power will make that work easier as well as other
>>> task such as converting to the v4l2_subdev active state API.
>>>
>>> Patch 1/11 just moves code around to make the consecutive changes easier
>>> to read. Patch 2/11 fix a locking issues when suspending the device.
>>> Patch 3/11 and 4/11 improves the locking design to prepare to improve
>>> the driver.
>>>
>>> Patch 5/11 make sure the device controls are always programmed after the
>>> device have been reset, fixing a possible issue when the device where
>>> resumed from system sleep.
>>>
>>> Patches 6/11, 7/11 and 8/11 is the real change where the .s_power
>>> callback is reworked to fit the design of .s_stream instead.
>>>
>>> And finally patch 9/11, 10/11 and 11/11 removes programming of the
>>> device from operation callbacks and solves the issue with querystd.
>>>
>>> The work is tested on R-Car M2 together with a ADV7180 device.
>>>
>>> See individual patches for changelog.
>>
>> This series looks good to me, other than the one typo and the
>> control handler kAPI issue, but that can be done in a follow-up
>> series.
>>
>> If you want I can take this series, let me know.
> 
> Thanks for pointing out the improvement in the kAPI, I agree with it.  
> But as you point out I can do that in follow up work so I would be happy 
> if you would take this series. Can you correct the typo while applying, 
> or would you prefer I send an undated series?

I'll correct the typo.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Niklas Söderlund (11):
>>>   media: adv7180: Move adv7180_set_power() and init_device()
>>>   media: adv7180: Add missing lock in suspend callback
>>>   media: adv7180: Move state mutex handling outside init_device()
>>>   media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking
>>>   media: adv7180: Setup controls every time the device is reset
>>>   media: adv7180: Power down decoder when configuring the device
>>>   media: adv7180: Split device initialization and reset
>>>   media: adv7180: Remove the s_power callback
>>>   media: adv7180: Do not write format to device in set_fmt
>>>   media: adv7180: Only validate format in s_std
>>>   media: adv7180: Only validate format in querystd
>>>
>>>  drivers/media/i2c/adv7180.c | 338 +++++++++++++++++++-----------------
>>>  1 file changed, 174 insertions(+), 164 deletions(-)
>>>
>>
>