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

[PATCH 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.

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.50.0

[PATCH 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>
---
 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 6e50b14f888f..2519bc53333c 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);
@@ -889,6 +889,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);
@@ -1359,62 +1415,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.50.0

[PATCH 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>
---
 drivers/media/i2c/adv7180.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 2519bc53333c..0c5511a7667d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1565,6 +1565,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);
 }
 
-- 
2.50.0

[PATCH 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>
---
 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 0c5511a7667d..bef708916190 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -893,7 +893,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);
@@ -903,11 +903,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);
 
@@ -918,31 +918,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)
@@ -1493,7 +1490,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;
 
@@ -1576,6 +1575,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.50.0

[PATCH 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>
---
 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 bef708916190..c2a79eba4dcd 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.50.0

[PATCH 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 where 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>
---
 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 c2a79eba4dcd..3c7c896a21a4 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;
 }
@@ -911,6 +910,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.50.0

[PATCH 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 | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 3c7c896a21a4..eab3ae2331fd 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;
@@ -900,6 +897,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;
@@ -940,6 +944,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;
 }
 
@@ -1471,10 +1483,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);
@@ -1582,9 +1591,12 @@ static int adv7180_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	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.50.0

[PATCH 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 eab3ae2331fd..a6f2bf7caa73 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -891,6 +891,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);
 
@@ -908,14 +925,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 */
@@ -1501,7 +1514,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;
@@ -1587,7 +1600,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.50.0

[PATCH 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 a6f2bf7caa73..dcdc89b717a4 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",
@@ -973,18 +958,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;
 }
 
@@ -1014,7 +1010,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.50.0

[PATCH 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>
---
 drivers/media/i2c/adv7180.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index dcdc89b717a4..8ebb72b2bb61 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -793,12 +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) {
-			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.50.0

[PATCH 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 8ebb72b2bb61..122d5b08052e 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.50.0

[PATCH 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 122d5b08052e..a62ac39bcd00 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.50.0

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

[Laurent Pinchart]

On Thu, Jul 03, 2025 at 10:52:13PM +0200, Niklas Söderlund wrote:
> 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 6e50b14f888f..2519bc53333c 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);
> @@ -889,6 +889,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);
> @@ -1359,62 +1415,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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Jul 03, 2025 at 10:52:14PM +0200, Niklas Söderlund wrote:
> 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>
> ---
>  drivers/media/i2c/adv7180.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 2519bc53333c..0c5511a7667d 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -1565,6 +1565,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);

Doesn't adv7180_resume() suffer from the same issue ? And how about
adv7180_set_pad_format() ?

>  }
>  

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Thu, Jul 03, 2025 at 10:52:15PM +0200, Niklas Söderlund wrote:
> 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 0c5511a7667d..bef708916190 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -893,7 +893,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);
> @@ -903,11 +903,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);
>  
> @@ -918,31 +918,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)
> @@ -1493,7 +1490,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;
>  
> @@ -1576,6 +1575,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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Niklas,

On Thu, Jul 03, 2025 at 10:52:16PM +0200, 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 bef708916190..c2a79eba4dcd 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,

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Jul 03, 2025 at 10:52:17PM +0200, Niklas Söderlund wrote:
> 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 where never restored

s/where/were/

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

> ---
>  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 c2a79eba4dcd..3c7c896a21a4 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;
>  }
> @@ -911,6 +910,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 */

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Jul 03, 2025 at 10:52:21PM +0200, Niklas Söderlund wrote:
> 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 | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index dcdc89b717a4..8ebb72b2bb61 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -793,12 +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) {
> -			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;

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2025-07-04 01:43:26 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Jul 03, 2025 at 10:52:14PM +0200, Niklas Söderlund wrote:
> > 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>
> > ---
> >  drivers/media/i2c/adv7180.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index 2519bc53333c..0c5511a7667d 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -1565,6 +1565,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);
> 
> Doesn't adv7180_resume() suffer from the same issue ? And how about
> adv7180_set_pad_format() ?

They do. But they will be fixed / reworked in later commits in the 
series. So it seems a but of churn to add a guard in this commit only to 
remove it later ;-)

> 
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund

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

[Laurent Pinchart]

On Fri, Jul 04, 2025 at 12:51:39AM +0200, Niklas Söderlund wrote:
> On 2025-07-04 01:43:26 +0300, Laurent Pinchart wrote:
> > On Thu, Jul 03, 2025 at 10:52:14PM +0200, Niklas Söderlund wrote:
> > > 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>
> > > ---
> > >  drivers/media/i2c/adv7180.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > index 2519bc53333c..0c5511a7667d 100644
> > > --- a/drivers/media/i2c/adv7180.c
> > > +++ b/drivers/media/i2c/adv7180.c
> > > @@ -1565,6 +1565,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);
> > 
> > Doesn't adv7180_resume() suffer from the same issue ? And how about
> > adv7180_set_pad_format() ?
> 
> They do. But they will be fixed / reworked in later commits in the 
> series. So it seems a but of churn to add a guard in this commit only to 
> remove it later ;-)

It's a small churn :-) It would make the patch less confusing.

> > >  }
> > >  

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

On 2025-07-04 02:06:01 +0300, Laurent Pinchart wrote:
> On Fri, Jul 04, 2025 at 12:51:39AM +0200, Niklas Söderlund wrote:
> > On 2025-07-04 01:43:26 +0300, Laurent Pinchart wrote:
> > > On Thu, Jul 03, 2025 at 10:52:14PM +0200, Niklas Söderlund wrote:
> > > > 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>
> > > > ---
> > > >  drivers/media/i2c/adv7180.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > > index 2519bc53333c..0c5511a7667d 100644
> > > > --- a/drivers/media/i2c/adv7180.c
> > > > +++ b/drivers/media/i2c/adv7180.c
> > > > @@ -1565,6 +1565,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);
> > > 
> > > Doesn't adv7180_resume() suffer from the same issue ? And how about
> > > adv7180_set_pad_format() ?
> > 
> > They do. But they will be fixed / reworked in later commits in the 
> > series. So it seems a but of churn to add a guard in this commit only to 
> > remove it later ;-)
> 
> It's a small churn :-) It would make the patch less confusing.

Ack, will fix for v2.

> 
> > > >  }
> > > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund