Received-SPF: pass (google.com: domain of linux-kernel+bounces-39376-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Jan 2024 23:50:27 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: broonie@kernel.org, arnd@arndb.de, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, alim.akhtar@samsung.com, 
	linux-spi@vger.kernel.org, linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org, 
	andre.draszik@linaro.org, peter.griffin@linaro.org, semen.protsenko@linaro.org, 
	kernel-team@android.com, willmcvicker@google.com
Subject: Re: [PATCH 07/21] spi: s3c64xx: use bitfield access macros
Message-ID: <ri7gerw4ov4jnmmkhtumhhtgfgxtr6kpsopdxjlx6fylbqznna@3qgvejyhjirw>
References: <20240123153421.715951-1-tudor.ambarus@linaro.org>
 <20240123153421.715951-8-tudor.ambarus@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123153421.715951-8-tudor.ambarus@linaro.org>

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:06PM +0000, Tudor Ambarus wrote:
> Use the bitfield access macros in order to clean and to make the driver
> easier to read.

most of the changes done here are allignment. I would mention it
in the log.

> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

..

> -#define S3C64XX_SPI_CLKSEL_SRCMSK	(3<<9)
> -#define S3C64XX_SPI_CLKSEL_SRCSHFT	9
> -#define S3C64XX_SPI_ENCLK_ENABLE	(1<<8)
> -#define S3C64XX_SPI_PSR_MASK		0xff
> -
> -#define S3C64XX_SPI_MODE_CH_TSZ_BYTE		(0<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD	(1<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_WORD		(2<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_MASK		(3<<29)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE		(0<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	(1<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_WORD		(2<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_MASK		(3<<17)
> +#define S3C64XX_SPI_CH_CFG			0x00
> +#define S3C64XX_SPI_CLK_CFG			0x04
> +#define S3C64XX_SPI_MODE_CFG			0x08
> +#define S3C64XX_SPI_CS_REG			0x0C
> +#define S3C64XX_SPI_INT_EN			0x10
> +#define S3C64XX_SPI_STATUS			0x14
> +#define S3C64XX_SPI_TX_DATA			0x18
> +#define S3C64XX_SPI_RX_DATA			0x1C
> +#define S3C64XX_SPI_PACKET_CNT			0x20
> +#define S3C64XX_SPI_PENDING_CLR			0x24
> +#define S3C64XX_SPI_SWAP_CFG			0x28
> +#define S3C64XX_SPI_FB_CLK			0x2C
> +
> +#define S3C64XX_SPI_CH_HS_EN			BIT(6)	/* High Speed Enable */
> +#define S3C64XX_SPI_CH_SW_RST			BIT(5)
> +#define S3C64XX_SPI_CH_SLAVE			BIT(4)
> +#define S3C64XX_SPI_CPOL_L			BIT(3)
> +#define S3C64XX_SPI_CPHA_B			BIT(2)
> +#define S3C64XX_SPI_CH_RXCH_ON			BIT(1)
> +#define S3C64XX_SPI_CH_TXCH_ON			BIT(0)
> +
> +#define S3C64XX_SPI_CLKSEL_SRCMSK		GENMASK(10, 9)
> +#define S3C64XX_SPI_ENCLK_ENABLE		BIT(8)
> +#define S3C64XX_SPI_PSR_MASK			GENMASK(15, 0)

I find it easier as 0xff to be honest, but I'm not going to be
picky.

> +
> +#define S3C64XX_SPI_MODE_CH_TSZ_MASK		GENMASK(30, 29)
> +#define S3C64XX_SPI_MODE_CH_TSZ_BYTE		0
> +#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD	1
> +#define S3C64XX_SPI_MODE_CH_TSZ_WORD		2

I personally find this pattern harder to read. Perhaps you can
already define these as FIELD_PREP here.

> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK		GENMASK(28, 19)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK		GENMASK(18, 17)

..

> -#define S3C64XX_SPI_FBCLK_MSK			(3<<0)
> +#define S3C64XX_SPI_FBCLK_MASK			GENMASK(1, 0)

0x3 to me is more understandable than (3<<0) and GENMASK(1, 0).
Bit operation defines should be used when they really simplify
the reading. But when they make it more difficult, then, I don't
see the point.

Overall looks good, though.

Andi