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

[Nirujogi, Pratap]

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