| Message ID | 20231110094705.1367083-4-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use V4L2 CCI in CCS driver | expand |
Hi, On 11/10/23 10:47, Sakari Ailus wrote: > Add CCI_REG_WIDTH() macro to obtain register width in bits and similarly, > CCI_REG_WIDTH_BYTES() to obtain it in bytes. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > include/media/v4l2-cci.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > index b4ce0a46092c..80eaa7fdc606 100644 > --- a/include/media/v4l2-cci.h > +++ b/include/media/v4l2-cci.h > @@ -40,6 +40,9 @@ struct cci_reg_sequence { > #define CCI_REG_FLAG_PRIVATE_END 31U > #define CCI_REG_PRIVATE_MASK GENMASK(CCI_REG_FLAG_PRIVATE_END, CCI_REG_FLAG_PRIVATE_START) > > +#define CCI_REG_WIDTH_BYTES(x) (((x) & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) Please use FIELD_GET like v4l2-cci.c does: #define CCI_REG_WIDTH_BYTES(x) FIELD_GET(CCI_REG_WIDTH_MASK, (x)) With that fixed: Reviewed-by: Hans de Goede <hdegoede@redhat.com> As for the patch 4 - 6, those are interesting patches but I'm afraid I don't have time to review them. Regards, Hans > +#define CCI_REG_WIDTH(x) (CCI_REG_WIDTH_BYTES(x) << 3) > + > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x))
Hi Hans, On Fri, Nov 10, 2023 at 03:52:28PM +0100, Hans de Goede wrote: > Hi, > > On 11/10/23 15:49, Laurent Pinchart wrote: > > On Fri, Nov 10, 2023 at 04:44:46PM +0200, Laurent Pinchart wrote: > >> On Fri, Nov 10, 2023 at 11:17:46AM +0000, Sakari Ailus wrote: > >>> On Fri, Nov 10, 2023 at 12:14:10PM +0100, Hans de Goede wrote: > >>>> On 11/10/23 10:47, Sakari Ailus wrote: > >>>>> Add CCI_REG_WIDTH() macro to obtain register width in bits and similarly, > >>>>> CCI_REG_WIDTH_BYTES() to obtain it in bytes. > >>>>> > >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>>>> --- > >>>>> include/media/v4l2-cci.h | 3 +++ > >>>>> 1 file changed, 3 insertions(+) > >>>>> > >>>>> diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > >>>>> index b4ce0a46092c..80eaa7fdc606 100644 > >>>>> --- a/include/media/v4l2-cci.h > >>>>> +++ b/include/media/v4l2-cci.h > >>>>> @@ -40,6 +40,9 @@ struct cci_reg_sequence { > >>>>> #define CCI_REG_FLAG_PRIVATE_END 31U > >>>>> #define CCI_REG_PRIVATE_MASK GENMASK(CCI_REG_FLAG_PRIVATE_END, CCI_REG_FLAG_PRIVATE_START) > >>>>> > >>>>> +#define CCI_REG_WIDTH_BYTES(x) (((x) & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) > >>>> > >>>> Please use FIELD_GET like v4l2-cci.c does: > >>>> > >>>> #define CCI_REG_WIDTH_BYTES(x) FIELD_GET(CCI_REG_WIDTH_MASK, (x)) > >>>> > >>>> With that fixed: > >>>> > >>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >>>> > >>>> As for the patch 4 - 6, those are interesting patches but > >>>> I'm afraid I don't have time to review them. > >>> > >>> No worries, I mainly included them to have a user for the newly added > >>> features. > >>> > >>> I'll send v2 with FIELD_GET(). > >> > >> With FIELD_GET(), > >> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Actually, how about using the new macro in v4l2-cci.c ? > > I considered this too, but atm there are 2 FIELD_GET calls directly > after one another, e.g. : > > len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > Using the macro would make this look like this: > > len = CCI_REG_WIDTH_BYTES(reg); > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > Which IMHO just make this harder to read. Now if we also add a CCI_REG_ADDR macro > and use both, then using the macros would be fine ... I like the idea. I'll add that for v2.
diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index b4ce0a46092c..80eaa7fdc606 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -40,6 +40,9 @@ struct cci_reg_sequence { #define CCI_REG_FLAG_PRIVATE_END 31U #define CCI_REG_PRIVATE_MASK GENMASK(CCI_REG_FLAG_PRIVATE_END, CCI_REG_FLAG_PRIVATE_START) +#define CCI_REG_WIDTH_BYTES(x) (((x) & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) +#define CCI_REG_WIDTH(x) (CCI_REG_WIDTH_BYTES(x) << 3) + #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x))
Add CCI_REG_WIDTH() macro to obtain register width in bits and similarly, CCI_REG_WIDTH_BYTES() to obtain it in bytes. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- include/media/v4l2-cci.h | 3 +++ 1 file changed, 3 insertions(+)