Re: [PATCH v5 9/9] tty: serial: atmel: Use FIELD_PREP/FIELD_GET

[Ilpo Järvinen]

On Thu, 22 Sep 2022, Sergiu Moga wrote:

> Convert all open-coded instances of bitfields retrieval/setting
> to FIELD_PREP/FIELD_GET where possible.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@xxxxxxxxxxxxx>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

With one caveat below which I leave up to you to decide as it might be
more a matter of taste thing.

> @@ -82,7 +84,7 @@
>  #define	ATMEL_US_INACK		BIT(20)	/* Inhibit Non Acknowledge */
>  #define	ATMEL_US_DSNACK		BIT(21)	/* Disable Successive NACK */
>  #define	ATMEL_US_MAX_ITER_MASK	GENMASK(26, 24)	/* Max Iterations */
> -#define	ATMEL_US_MAX_ITER(n)	(((n) << 24) & ATMEL_US_MAX_ITER_MASK)
> +#define	ATMEL_US_MAX_ITER(n)	FIELD_PREP(ATMEL_US_MAX_ITER_MASK, (n))
>  #define	ATMEL_US_FILTER		BIT(28)	/* Infrared Receive Line Filter */
>  
>  #define ATMEL_US_IER		0x08	/* Interrupt Enable Register */
> @@ -134,19 +136,19 @@
>  
>  #define ATMEL_US_CMPR		0x90	/* Comparaison Register */
>  #define ATMEL_US_FMR		0xa0	/* FIFO Mode Register */
> -#define	ATMEL_US_TXRDYM(data)	(((data) & 0x3) << 0)	/* TX Ready Mode */
> -#define	ATMEL_US_RXRDYM(data)	(((data) & 0x3) << 4)	/* RX Ready Mode */
> +#define	ATMEL_US_TXRDYM(data)	FIELD_PREP(GENMASK(1, 0), (data))	/* TX Ready Mode */
> +#define	ATMEL_US_RXRDYM(data)	FIELD_PREP(GENMASK(5, 4), (data))	/* RX Ready Mode */
>  #define		ATMEL_US_ONE_DATA	0x0
>  #define		ATMEL_US_TWO_DATA	0x1
>  #define		ATMEL_US_FOUR_DATA	0x2
>  #define	ATMEL_US_FRTSC		BIT(7)	/* FIFO RTS pin Control */
> -#define	ATMEL_US_TXFTHRES(thr)	(((thr) & 0x3f) << 8)	/* TX FIFO Threshold */
> -#define	ATMEL_US_RXFTHRES(thr)	(((thr) & 0x3f) << 16)	/* RX FIFO Threshold */
> -#define	ATMEL_US_RXFTHRES2(thr)	(((thr) & 0x3f) << 24)	/* RX FIFO Threshold2 */
> +#define	ATMEL_US_TXFTHRES(thr)	FIELD_PREP(GENMASK(13, 8), (thr))	/* TX FIFO Threshold */
> +#define	ATMEL_US_RXFTHRES(thr)	FIELD_PREP(GENMASK(21, 16), (thr))	/* RX FIFO Threshold */
> +#define	ATMEL_US_RXFTHRES2(thr)	FIELD_PREP(GENMASK(29, 24), (thr))	/* RX FIFO Threshold2 */
>  
>  #define ATMEL_US_FLR		0xa4	/* FIFO Level Register */
> -#define	ATMEL_US_TXFL(reg)	(((reg) >> 0) & 0x3f)	/* TX FIFO Level */
> -#define	ATMEL_US_RXFL(reg)	(((reg) >> 16) & 0x3f)	/* RX FIFO Level */
> +#define	ATMEL_US_TXFL(reg)	FIELD_GET(GENMASK(5, 0), (reg))		/* TX FIFO Level */
> +#define	ATMEL_US_RXFL(reg)	FIELD_GET(GENMASK(21, 16), (reg))	/* RX FIFO Level */

I don't particularly like this kind of constructs. IMHO, all these would 
be nice after removing the macro argument and moving the FIELD_PREP() to 
.c file. That is,

.h:
#define ATMEL_US_RXFL	GENMASK(21, 16)

.c:
	... FIELD_PREP(ATMEL_US_RXFL, arg) ...

But I guess there might be differing opinions on it (and both are 
correct from purely technical perspective).

-- 
 i.