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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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