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