| Message ID | 20231110094705.1367083-4-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| 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, Thank you for the review. On Fri, Nov 10, 2023 at 12:14:10PM +0100, Hans de Goede wrote: > 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. No worries, I mainly included them to have a user for the newly added features. I'll send v2 with FIELD_GET().
On Fri, Nov 10, 2023 at 11:17:46AM +0000, Sakari Ailus wrote: > Hi Hans, > > Thank you for the review. > > On Fri, Nov 10, 2023 at 12:14:10PM +0100, Hans de Goede wrote: > > 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. > > 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>
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 ?
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 ... Regards, Hans
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(+)