Re: [PATCH v4 2/3] media: i2c: Add driver for OmniVision OV32C4

[Sakari Ailus]

Hi Robert,

On Mon, Oct 05, 2026 at 12:14:46PM +0200, Robert Bozik wrote:
> Hi Sakari,
> 
> Thank you for the review.
> 
> On Mon, Oct 05, 2026 at 12:23:20PM +0300, Sakari Ailus wrote:
> > > +	{ 0x6ad0, 0x00 },
> > > +	{ 0x6ad1, 0x75 },
> > 
> > Are there any gaps in this deluge of 0x0 and 0x75? If not, could you write
> > it programmatically rather than using a huge array?
> 
> Two contiguous ranges, 0x6ad0-0x6bef and 0x6c00-0x707f, with one gap of
> 16 bytes between them; every even address holds 0x00 and the odd one
> 0x75, so it is 720 16-bit registers set to 0x0075. v5 writes them from a
> loop and the table shrinks to 345 entries. I'll verify the stream and
> the image after the change, since it alters the bus transactions.
> 
> > > +	ret = acpi_dev_get_resources(adev, &resources, ov32c4_i2c_res_cb, &ctx);
> > 
> > The presence of additional chips like VCM is very much dependent on the
> > module, and I think we should have parsing of the I²C address outside the
> > sensor driver.
> > 
> > One option could be to stuff it into the reg property in the ipu-bridge,
> > that way it'd work the same way for the driver on both DT and ACPI. In the
> > ipu-bridge, I'd use a static value and provide the reg property for this
> > sensor only (based on _HID). i2c_new_ancillary_device() will only use OF so
> > the driver will need to dig the address manually still.
> 
> Agreed, that is better. For v5 I'd give ipu-bridge a small table of
> sensors whose second I2C address belongs to the sensor itself,
> { "OVTI32C4", 0x3e }; for a sensor in it the bridge adds reg = <main,
> aon> to the sensor's software node and does not instantiate the VCM
> from SSDB - so the no-VCM exception of patch 3 folds into the same
> table. The driver reads reg with device_property_read_u32_array() on
> both DT and ACPI and the _CRS walk goes away with its CONFIG_ACPI
> guard. The property fits into the existing dev_properties slot that
> lens-focus uses when there is a VCM, so no change to the header.

You shouldn't assume there won't be a VCM; please do add an array entry for
reg instead.

> 
> The rest will be in v5 as suggested - the register names, the RGB/IR
> comments, the function name, the error label and the comments; the HTS
> define was unused and goes too.

-- 
Regards,

Sakari Ailus