Re: [PATCH 07/21] spi: s3c64xx: use bitfield access macros

[Andi Shyti <andi.shyti@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---

...

> -#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