Re: [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers

[johannes . goede]

Hi Xiaolei,

Thank you for the new version and thank you for
addressing my comments about the register lists.

A few more comments inline, sorry for not catching these
in my earlier review.

On 31-Dec-25 09:39, Xiaolei Wang wrote:
> Use the new common CCI register access helpers to replace the private
> register access helpers in the ov5647 driver. This simplifies the driver
> by reducing the amount of code.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> ---

...

> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..cbcb760ba5cd 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c

...

>  static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>  {
> -	u8 channel_id;
> +	struct ov5647 *sensor = to_sensor(sd);
> +	u64 channel_id;
>  	int ret;
>  
> -	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
> +	ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
>  	if (ret < 0)
>  		return ret;
>  
>  	channel_id &= ~(3 << 6);
>  
> -	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
> -			    channel_id | (channel << 6));
> +	return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
> +			 channel_id | (channel << 6), NULL);
>  }

This can be replaced with:

At the top below:

#define OV5647_REG_MIPI_CTRL14		CCI_REG8(0x4814)

add:

#define OV5647_REG_MIPI_CTRL14_CHANNEL_MASK	GENMASK(7, 6)
#define OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT	6

And then replace ov5647_set_virtual_channel() with:

static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
{
	struct ov5647 *sensor = to_sensor(sd);

	return cci_update_bits(sensor->regmap, OV5647_REG_MIPI_CTRL14,
			       OV5647_REG_MIPI_CTRL14_CHANNEL_MASK,
			       channel << OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT,
			       NULL);
}

...

> @@ -815,23 +726,23 @@ static int ov5647_power_on(struct device *dev)
>  static int ov5647_power_off(struct device *dev)
>  {
>  	struct ov5647 *sensor = dev_get_drvdata(dev);
> -	u8 rdval;
> +	u64 rdval;
>  	int ret;
>  
>  	dev_dbg(dev, "OV5647 power off\n");
>  
> -	ret = ov5647_write_array(&sensor->sd, sensor_oe_disable_regs,
> -				 ARRAY_SIZE(sensor_oe_disable_regs));
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_disable_regs,
> +				     ARRAY_SIZE(sensor_oe_disable_regs));
>  	if (ret < 0)
>  		dev_dbg(dev, "disable oe failed\n");

And here replace this read + write:

>  
>  	/* Enter software standby */
> -	ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");
>  
>  	rdval &= ~0x01;
> -	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
> +	ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");

With:

	ret = cci_update_bits(sensor->regmap, OV5647_SW_STANDBY, 0x01, 0x00, NULL);
	if (ret < 0)
		dev_dbg(dev, "software standby failed\n");

...

>  static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 reg;
> +	u64 reg;
>  
>  	/* Non-zero turns on AGC by clearing bit 1.*/
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>  	if (ret)
>  		return ret;
>  
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> -							: reg | BIT(1));
> +	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> +							: reg | BIT(1), NULL);
>  }

And this is another opportunity to use cci_update_bits():

	return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(1),
			       val ? 0 : BIT(1), NULL);


>  static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 reg;
> +	u64 reg;
>  
>  	/*
>  	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
>  	 * clearing bit 0.
>  	 */
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>  	if (ret)
>  		return ret;
>  
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC,
> +	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
>  			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
> -							: reg & ~BIT(0));
> +							: reg & ~BIT(0), NULL);
>  }


Same here, please switch this to use cci_update_bits()

Regards,

Hans