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

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

Hi Laurent,

On 6/26/2025 8:23 AM, 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 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 
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)

Thanks,
Pratap

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