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

["Nirujogi, Pratap" <pnirujog@amd.com>]

Hi Laurent, Sakari,

On 6/23/2025 8:06 AM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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.

Thanks,
Pratap

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