On Mon, Nov 13, 2023 at 04:18:28PM +0000, Sakari Ailus wrote:
> On Mon, Nov 13, 2023 at 06:11:38PM +0200, Laurent Pinchart wrote:
> > 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.
It makes review easier, as reviewers can then read each patch in
isolation and have the full context.
> > > ---
> > > 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,
Laurent Pinchart
