Re: [PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[David Laight]

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.

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
>