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