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

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

Hi Kieran,

On 6/26/2025 7:11 AM, Kieran Bingham wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Quoting Nirujogi, Pratap (2025-06-25 23:06:01)
>> Hi Sakari, Hi Laurent,
>>
>> On 6/23/2025 5:55 PM, Nirujogi, Pratap wrote:
>> [...]
>>>>>> I think it can live in the driver for now. Given that the device uses
>>>>>> only 8 bits of register address, I would store the page number in bits
>>>>>> 15:8 instead of bits 31:24, as the CCI helpers do not make bits 27:24
>>>>>> available for driver-specific purpose.
>>>>>
>>>>> I'd use the CCI private bits, the driver uses page numbers up to 4 so 4
>>>>> bits are plenty for that. If we add pages to CCI later, this may be
>>>>> refactored then.
>>>>
>>>> That works too.
>>>>
>>> Thanks for your support. We will add the page number in the register
>>> address 15:8 or 11:8 and will update the implementation accordingly in
>>> the next version.
>>>
>> I would like to share the approach we are taking to implement the CCI
>> helpers that support page value. Could you please review the steps and
>> let us know if they make sense or if any adjustments are needed?
>>
>> 1: Add new macros to embed page value into the register address.
>>
>> Ex:
>> #define CCI_PAGE_REG8(x, p)             ((1 << CCI_REG_WIDTH_SHIFT) | (p <<
>> CCI_REG_PRIVATE_SHIFT) | (x))
>> #define CCI_PAGE_REG16(x, p)            ((2 << CCI_REG_WIDTH_SHIFT) | (p <<
>> CCI_REG_PRIVATE_SHIFT) | (x))
>>
>> 2: Create V4L2 CCI context. Initialize page control reg, current_page,
>> regmap etc.
>>
>> Ex:
>> struct v4l2_cci_ctx {
>>          struct mutex lock;
>>          struct regmap *map;
>>          s16 current_page;
>>          u8 page_ctrl_reg;
>> }
>>
>> 3: Introduce new CCI helpers - cci_pwrite() and cci_pread() to handle
>> register read-writes updating the page control register as necessary.
> 
> Out of curiosity - but couldn't the existing cci_write and cci_read
> already be used by the users - and then the default behaviour is that
> the page isn't modified if there is no page_ctrl_reg - and by default
> CCI_REG() will simply have (initilised) as page 0 - so the pages will
> never change on those calls?
> 
> Then the users can indeed define
> 
> #define TEST_PATTERN_PAGE 5
> #define TEST_PATTERN_COLOUR_BARS BIT(3)
> 
> #define MY_TEST_PATTERN_REG CCI_PAGE_REG8(0x33, TEST_PATTERN_PAGE)
> 
> and can call
>   cci_write(regmap, MY_TEST_PATTERN_REG, TEST_PATTERN_COLOUR_BARS, &ret);
> 
> with everything handled transparently ?
> 
> 
> Or do you envisage more complications with the types of pages that might
> be supportable ?
> 
> (I perfectly understand if I'm wishing for an unreachable utopia -
> because I haven't considered something implicit about the page handling
> that I haven't yet used :D)
> 
Initally we too felt the existing cci_write() / cci_read() should be 
updated to support pages, but worried about the potential regressions 
with other drivers that are based on existing implementation. We felt 
adding new helper functions in V4L2 CCI — while keeping the original 
cci_write() and cci_read() unchanged — would be a safer and more stable 
approach.

Thanks,
Pratap

> --
> Kieran
> 
> 
>> int cci_pwrite(void *data, u32 reg, u64 val, int *err)
>> {
>>          /* get v4l2_cci_ctx context from data */
>>
>>          /* get page value from reg */
>>
>>          /* acquire mutex */
>>
>>          /* update cci page control reg, save current page value */
>>
>>          /* do cci_write */
>>
>>          /* release mutex */
>> }
>>
>> Similar steps for cci_pread() as well.
>>
>> Thanks,
>> Pratap