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

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

On 6/26/2025 2:56 PM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Thu, Jun 26, 2025 at 02:22:00PM -0400, Nirujogi, Pratap wrote:
>> On 6/26/2025 8:23 AM, Laurent Pinchart wrote:
>>> On Thu, Jun 26, 2025 at 12:11:27PM +0100, Kieran Bingham wrote:
>>>> 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)
>>>
>>> I don't think we should implement page support in the CCI helpers
>>> themselves yet. We have too few drivers to look at to understand the
>>> requirements. Instead, I would store the page number in the driver
>>> private bits of the 32-bit address (that's bits 31:28), and add wrappers
>>> around cci_read() and cci_write() to the OV05C10 driver that handles the
>>> page configuration.
>>>
>>> Once we'll have multiple drivers doing the same, it will be easier to
>>> see what requirements they share, and move the feature to the CCI
>>> helpers.
>>
>> Thanks for clarifying. I agree it would be simple and safer approach too
>> to handle this way. We will add the following macros in v4l2-cci.h and
> 
> Please add the macros to the driver instead, not to v4l2-cci.h. Once
> multiple drivers will implement a similar mechanism we can study how to
> generalize it.
> 
Thanks Laurent. That makes it even easier - all changes can be included 
in the same patch. Its clear now, we will finalize the changes and work 
toward submitting v4.

Thanks,
Pratap

>> update the existing wrappers ov05c10_reg_write() / ov05c10_reg_read() in
>> the driver to retrieve the page and register values to call cci_write()
>> / cci_read(). We will add new wrappers too wherever necessary in the
>> driver (ex: wrapper for cci_multi_reg_write() on replacing CCI_REG8 with
>> CCI_PAGE_REG8)
>>
>> #define CCI_PAGE_REG8(x, p)           ((1 << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG16(x, p)          ((2 << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG24(x, p)          ((3 << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG32(x, p)          ((4 << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG64(x, p)          ((8 << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG16_LE(x, p)               (CCI_REG_LE | (2U << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG24_LE(x, p)               (CCI_REG_LE | (3U << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG32_LE(x, p)               (CCI_REG_LE | (4U << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_REG64_LE(x, p)               (CCI_REG_LE | (8U << CCI_REG_WIDTH_SHIFT) | (p << CCI_REG_PAGE_SHIFT) | (x))
>> #define CCI_PAGE_GET(x)                       FIELD_GET(CCI_REG_PAGE_MASK, x)
>>
>>>>> 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.
> 
> --
> Regards,
> 
> Laurent Pinchart