Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver

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

On Mon, Jun 23, 2025 at 11:27:39AM +0000, Sakari Ailus wrote:
> On Sun, Jun 15, 2025 at 01:52:57AM +0300, Laurent Pinchart wrote:
> > > > > +#define OV05C10_REF_CLK			(24 * HZ_PER_MHZ)
> > > > 
> > > > Seems your module use 24 MHz clock input. The Dell's modules always use
> > > > 19.2MHz, which means your the PLL settings will not work on Dell's.
> > > 
> > > This is ok as further work. Please send a patch. :-)
> > 
> > The patch should calculate PLL configuration dynamically, perhaps using
> > the ccs-pll calculator if applicable.
> 
> As much as I do like your suggestion, I don't think it's really feasible to
> often do this for Omnivision sensors (most others largely do just work
> without much hassle wrt. PLL, as long as a PLL calculator exists). This
> sensor's PLL tree is different from CCS and badly documented, as expected.

How much do we know about the PLL structure ?

> > > > Seems there are already several camera sensors using page-based registers.
> > > > Is it a good idea to add page support in CCI interface?
> > > 
> > > Sounds like a good idea as such but I'm not sure how common this really is,
> > > I think I've seen a few Omnivision sensors doing this. If implemented, I
> > > think it would be nice if the page could be encoded in the register address
> > > which V4L2 CCI would store and switch page if needed only. This would
> > > require serialising accesses, too. There's some room in CCI register raw
> > > value space so this could be done without even changing that, say, with
> > > 8-bit page and 8-bit register address.
> > 
> > Ack. I've worked on a driver for the AP1302 external ISP, which also
> > uses pages registers. The full address space spans 32 bits though, but
> > fortunately the driver doesn't need to access anything above 0x00ffffff.
> 
> 0xffffff?

Yes.

> The current CCI register addresses are limited to 16 bits. To
> support that, we'd need to use u64 most likely.

I handled it in the ap1302 driver, by using bits 31:24 of address to
store a 8 bits page value. It's a hack as the CCI helper currently only
allocates 4 bits of the address to driver-specific purpose.

> For 16-bit register
> addresses and 8-bit values which probably are the most common, that starts
> to appear a bit wasteful.

It is wasteful, I don't want to turn the register address to a 64-bit
value.

-- 
Regards,

Laurent Pinchart