Hi Andi, On Wed, Jan 1, 2025 at 12:11 PM David Laight <david.laight.linux@xxxxxxxxx> wrote: > > On Fri, 27 Dec 2024 11:51:50 +0000 > Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic` > > driver to improve readability and maintain consistency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > Reviewed-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > > Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > --- > > v2->v3 > > - Collected RB and tested tags > > > > v1->v2 > > - Collected RB tag from Geert > > --- > > drivers/i2c/busses/i2c-riic.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > > index 954e066d61a8..ddae4b74a86b 100644 > > --- a/drivers/i2c/busses/i2c-riic.c > > +++ b/drivers/i2c/busses/i2c-riic.c > > @@ -55,7 +55,7 @@ > > #define ICCR2_RS BIT(2) > > #define ICCR2_ST BIT(1) > > > > -#define ICMR1_CKS_MASK 0x70 > > +#define ICMR1_CKS_MASK GENMASK(6, 4) > > #define ICMR1_BCWP BIT(3) > > #define ICMR1_CKS(_x) ((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP) > > I'm really not at all sure how this makes it 'more readable'. > Call me 'old fashioned' but I like hex constants - especially for bytes. > In this case it might be best to be consistent with the hardware datasheet. > Let me know if you dont accept such patches. I'll drop it from the next version. > Pretty much the only time I've actually used anything like BIT() was > for a spec that numbered the bits from 1 to 32 with bit 1 being the most > significant. > > > > > @@ -73,7 +73,7 @@ > > > > #define ICSR2_NACKF BIT(4) > > > > -#define ICBR_RESERVED 0xe0 /* Should be 1 on writes */ > > +#define ICBR_RESERVED GENMASK(7, 5) /* Should be 1 on writes */ > > 'Should all be set on writes' ? > Yes. Cheers, Prabhakar
