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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

> 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