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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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