| Message ID | 20241227115154.56154-5-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: riic: driver cleanup and improvements | expand |
Fri, Dec 27, 2024 at 11:51:50AM +0000, Prabhakar kirjoitti: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic` > driver to improve readability and maintain consistency. ... > -#define ICBR_RESERVED 0xe0 /* Should be 1 on writes */ > +#define ICBR_RESERVED GENMASK(7, 5) /* Should be 1 on writes */ I don't understand the comment. Does it mean the value should be 0x20?
On Fri, 27 Dec 2024 11:51:50 +0000 Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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@bp.renesas.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > 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. 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' ? David > > #define RIIC_INIT_MSG -1 >
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) @@ -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 */ #define RIIC_INIT_MSG -1