Hi Laurent,
On Mon, Nov 13, 2023 at 06:11:38PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Mon, Nov 13, 2023 at 06:05:58PM +0200, 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.
> >
> > Also add CCI_REG_ADDR() macro to obtain the address of a register.
> >
> > Use both macros in v4l2-cci.c, too.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
> Please write per-patch changelogs. You would then have likely caught the
> issue below.
That forces the reviewer to read all the patches, I'm not convinced it
would have made any difference here.
>
> > ---
> > drivers/media/v4l2-core/v4l2-cci.c | 8 ++++----
> > include/media/v4l2-cci.h | 9 +++++++--
> > 2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c
> > index bc2dbec019b0..3179160abde3 100644
> > --- a/drivers/media/v4l2-core/v4l2-cci.c
> > +++ b/drivers/media/v4l2-core/v4l2-cci.c
> > @@ -25,8 +25,8 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
> > if (err && *err)
> > return *err;
> >
> > - len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
> > - reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
> > + len = CCI_REG_WIDTH_BYTES(reg);
> > + reg = CCI_REG_ADDR(reg);
> >
> > ret = regmap_bulk_read(map, reg, buf, len);
> > if (ret) {
> > @@ -75,8 +75,8 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
> > if (err && *err)
> > return *err;
> >
> > - len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
> > - reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
> > + len = CCI_REG_WIDTH_BYTES(reg);
> > + reg = CCI_REG_ADDR(reg);
> >
> > switch (len) {
> > case 1:
> > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
> > index ee469f03e440..50df3aa4af1d 100644
> > --- a/include/media/v4l2-cci.h
> > +++ b/include/media/v4l2-cci.h
> > @@ -7,6 +7,7 @@
> > #ifndef _V4L2_CCI_H
> > #define _V4L2_CCI_H
> >
> > +#include <linux/bitfield.h>
> > #include <linux/bits.h>
> > #include <linux/types.h>
> >
> > @@ -36,8 +37,12 @@ struct cci_reg_sequence {
> > /*
> > * 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)
> > +#define CCI_REG_PRIVATE_SHIFT 28
> > +#define CCI_REG_PRIVATE_MASK GENMASK(31, CCI_REG_PRIVATE_SHIFT)
>
> Was this meant to be in the previous patch ?
Yes. But this was actually there in v2 already, and missed in review. I'll
fix in v4.
>
> > +
> > +#define CCI_REG_WIDTH_BYTES(x) FIELD_GET(CCI_REG_WIDTH_MASK, x)
> > +#define CCI_REG_WIDTH(x) (CCI_REG_WIDTH_BYTES(x) << 3)
> > +#define CCI_REG_ADDR(x) FIELD_GET(CCI_REG_ADDR_MASK, x)
> >
> > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x))
> > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x))
>
--
Regards,
Sakari Ailus
