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

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

Hi Laurent,

On Mon, Nov 13, 2023 at 03:54:00PM +0200, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 03:53:22PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 03:44:54PM +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>
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  include/media/v4l2-cci.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
> > > index f2c2962e936b..ee469f03e440 100644
> > > --- a/include/media/v4l2-cci.h
> > > +++ b/include/media/v4l2-cci.h
> > > @@ -33,6 +33,11 @@ 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)
> > 
> > Should these be unsigned too for consistency ?
> > 
> > > +/*
> > > + * Private CCI register flags, for the use of drivers.
> > > + */
> > > +#define CCI_REG_PRIVATE_SHIFT		28U
> > > +#define CCI_REG_PRIVATE_MASK		GENMASK(31U, CCI_REG_PRIVATE_SHIFT)
> 
> And actually, for consistency again, I would write
> 
> #define CCI_REG_PRIVATE_MASK		GENMASK(31U, 28U)

I would prefer to use the macro instead.

I can post a patch doing that for the older definitions, too.

It's not urgent though. I'll wait for the endianness patches to get merged.

-- 
Regards,

Sakari Ailus