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

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

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.

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