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@xxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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@xxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxxxxxx>
> >
> > 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.
--
Regards,
Sakari Ailus
