Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 15:12:35 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Sergiu Moga <sergiu.moga@microchip.com>
cc:     lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, nicolas.ferre@microchip.com,
        alexandre.belloni@bootlin.com, claudiu.beznea@microchip.com,
        radu_nicolae.pirea@upb.ro, richard.genoud@gmail.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        kavyasree.kotagiri@microchip.com, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        LKML <linux-kernel@vger.kernel.org>, linux-spi@vger.kernel.org,
        linux-serial <linux-serial@vger.kernel.org>
Subject: Re: [PATCH v5 9/9] tty: serial: atmel: Use FIELD_PREP/FIELD_GET
In-Reply-To: <20220922113347.144383-10-sergiu.moga@microchip.com>
Message-ID: <a894518-4112-b99a-c2b7-657c23ee4974@linux.intel.com>
References: <20220922113347.144383-1-sergiu.moga@microchip.com> <20220922113347.144383-10-sergiu.moga@microchip.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-152948573-1663848765=:1702"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-152948573-1663848765=:1702
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT

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@microchip.com>

Reviewed-by: Ilpo J?rvinen <ilpo.jarvinen@linux.intel.com>

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.

--8323329-152948573-1663848765=:1702--