Re: [PATCH 2/6] media: v4l: cci: Add driver-private bit definitions

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Fri, Nov 10, 2023 at 04:44:07PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Nov 10, 2023 at 11:47:01AM +0200, Sakari Ailus wrote:
> > Provide a few bits for drivers to store private information on register
> > definitions.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/v4l2-cci.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
> > index f2c2962e936b..b4ce0a46092c 100644
> > --- a/include/media/v4l2-cci.h
> > +++ b/include/media/v4l2-cci.h
> > @@ -33,6 +33,12 @@ struct cci_reg_sequence {
> >  #define CCI_REG_ADDR_MASK		GENMASK(15, 0)
> >  #define CCI_REG_WIDTH_SHIFT		16
> >  #define CCI_REG_WIDTH_MASK		GENMASK(19, 16)
> > +/*
> > + * Private CCI register flags, for the use of drivers.
> > + */
> > +#define CCI_REG_FLAG_PRIVATE_START	28U
> 
> Could we name this CCI_REG_PRIVATE_SHIFT, like we do for WIDTH ?
> 
> > +#define CCI_REG_FLAG_PRIVATE_END	31U
> > +#define CCI_REG_PRIVATE_MASK		GENMASK(CCI_REG_FLAG_PRIVATE_END, CCI_REG_FLAG_PRIVATE_START)
> 
> CCI_REG_FLAG_PRIVATE_END isn't used in the rest of the series, so I
> would just write
> 
> #define CCI_REG_PRIVATE_MASK		GENMASK(31, 28)
> 
> for consistency.

Thank you for the thorough review. ;-)

I was actually thinking of adding BUILD_BUG_ON() for that. Or add
CCI_REG_FLAG_PRIVATE_BITS --- it's a bit nicer to test against. Although I
can't imagine a reason right now why we'd want to reduce the number of
these bits.

I'm fine with just changing the mask to use numbers though, with an added
comment on checking all users if reducing the number. Huh.

> 
> >  
> >  #define CCI_REG8(x)			((1 << CCI_REG_WIDTH_SHIFT) | (x))
> >  #define CCI_REG16(x)			((2 << CCI_REG_WIDTH_SHIFT) | (x))
> 

-- 
Regards,

Sakari Ailus