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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Pratap, Kieran,

On 6/17/25 02:46, Nirujogi, Pratap wrote:
> Hi Kieran,
> 
> Thank you for reviewing and sharing your feedback.
> 
> On 6/13/2025 7:02 AM, Kieran Bingham wrote:
>> Caution: This message originated from an External Source. Use proper 
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Quoting Hao Yao (2025-06-13 05:55:46)
>> I think it will highlight that werever possible - the code below should
>> be factored out to support the different configuration requirements.
>> Cleaning up the large tables of register addresses and making those
>> configurable functions for example configuring the link rate
>> independently would be really beneficial!
>>
>> That's precisely why we continually push for reducing the large
>> "undocumented register" tables in sensor drivers...
>>
> Yes, I agree, it is best to have documented settings and make 
> calculation for sensor modes instead of array of undocumented settings. 
> However the usual practice is that we send requirements to sensor vendor 
> and they provide us the settings array to be applied. May be we can try 
> to move PLL settings to different array but it is not always just the 
> PLL, there are a lot of undocumented settings which are in sync with PLL 
> and sometimes changing the PLL may result in misbehaviour of the sensor...
> 
> We will try to move PLL settings to separate array but cannot guarantee 
> changing these settings alone will make the sensor functional.

I did discuss this with Laurent as well and based on that I'd suggest 
the following split:

- power-on register list,
- external clock / link frequency specific list (often these are tied)
   and
- sensor mode

Which of these lists a particular register is written in isn't crucial 
at the moment, this can be always changed later on.

I'd leave the possible PLL calculator for later. Having more 
configurations is also useful for validating its function. A different 
PLL configuration from the original doesn't necessarily mean it would be 
wrong.

...

>>>> +     page = OV05C10_GET_PAGE_NUM(reg);
>>>> +     addr = OV05C10_GET_REG_ADDR(reg);
>>>> +     ov05c10_switch_page(ov05c10, page, &ret);
>>>> +     cci_write(ov05c10->regmap, addr, val, &ret);
>>>> +     if (err)
>>>> +             *err = ret;
>>>> +
>>>> +     return ret;
>>>> +}
>>
>> One of the main goals of CCI helpers was to avoid all of the custom
>> device accessors being duplicated in each driver, so I think extending
>> the CCI helpers to support page based accesses in some common way would
>> be beneficial.
>>
> Yes, I agree. We can take on this enhancement either now or in the 
> future, depending on the direction. We'll wait for Sakari's feedback to 
> determine the best way to proceed.

As Laurent suggested, keep the page-based helper macros/functions in 
this driver, we can generalise them later on as needed.

-- 
Kind regards,

Sakari Ailus