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

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

On Mon, Jun 23, 2025 at 05:53:44PM -0400, Nirujogi, Pratap wrote:
> On 6/23/2025 8:06 AM, Laurent Pinchart wrote:
> > 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 ?
> 
> sorry to inform we don't have much details, we've consulted with the 
> sensor vendor, but they are not willing to share specifics regarding the 
> PLL calculations, register details, or configuration settings. They have 
> recommended reaching out to them directly for any PLL configurations 
> required for the modes we intend to support.

Do you have access to the sensor datasheet ?

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