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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 25 Jan 2024 23:50:27 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: broonie@...nel.org, arnd@...db.de, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, alim.akhtar@...sung.com, 
	linux-spi@...r.kernel.org, linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org, 
	andre.draszik@...aro.org, peter.griffin@...aro.org, semen.protsenko@...aro.org, 
	kernel-team@...roid.com, willmcvicker@...gle.com
Subject: Re: [PATCH 07/21] spi: s3c64xx: use bitfield access macros

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@...aro.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