Re: [PATCH v2 09/28] spi: s3c64xx: use bitfield access macros

From: Sam Protsenko
Date: Thu Jan 25 2024 - 14:50:34 EST

Next message: Jonathan Corbet: "Re: [PATCH 6/8] Documentation: Create a new folder for all timer internals"
Previous message: Stefan O'Rear: "Re: [RFC PATCH v1 07/28] riscv: kernel handling on trap entry/exit for user cfi"
In reply to: Tudor Ambarus: "[PATCH v2 09/28] spi: s3c64xx: use bitfield access macros"
Next in thread: Tudor Ambarus: "Re: [PATCH v2 09/28] spi: s3c64xx: use bitfield access macros"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaroorg> wrote:
>
> Use the bitfield access macros in order to clean and to make the driver
> easier to read.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> drivers/spi/spi-s3c64xx.c | 196 +++++++++++++++++++-------------------
> 1 file changed, 99 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 1e44b24f6401..d046810da51f 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -4,6 +4,7 @@
> // Jaswinder Singh <jassi.brar@xxxxxxxxxxx>
>
> #include <linux/bits.h>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/dma-mapping.h>
> @@ -18,91 +19,91 @@
> #include <linux/pm_runtime.h>
> #include <linux/spi/spi.h>
>
> -#define MAX_SPI_PORTS 12
> -#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1)
> -#define AUTOSUSPEND_TIMEOUT 2000
> +#define MAX_SPI_PORTS 12
> +#define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1)
> +#define AUTOSUSPEND_TIMEOUT 2000
>
> /* Registers and bit-fields */
>
> -#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 (1<<6) /* High Speed Enable */
> -#define S3C64XX_SPI_CH_SW_RST (1<<5)
> -#define S3C64XX_SPI_CH_SLAVE (1<<4)
> -#define S3C64XX_SPI_CPOL_L (1<<3)
> -#define S3C64XX_SPI_CPHA_B (1<<2)
> -#define S3C64XX_SPI_CH_RXCH_ON (1<<1)
> -#define S3C64XX_SPI_CH_TXCH_ON (1<<0)
> -
> -#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)

But it was 0xff (7:0) originally, and here you extend it up to 15:0.
Was it intentional? If so, I'd advice to keep non-functional changes
as a separate patch, and pull all functional changes like these into
another one, probably with an explanation why it's needed.

> +
> +#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
> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK GENMASK(28, 19)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK GENMASK(18, 17)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE 0
> +#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD 1
> +#define S3C64XX_SPI_MODE_BUS_TSZ_WORD 2
> #define S3C64XX_SPI_MODE_RX_RDY_LVL GENMASK(16, 11)
> -#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT 11
> -#define S3C64XX_SPI_MODE_SELF_LOOPBACK (1<<3)
> -#define S3C64XX_SPI_MODE_RXDMA_ON (1<<2)
> -#define S3C64XX_SPI_MODE_TXDMA_ON (1<<1)
> -#define S3C64XX_SPI_MODE_4BURST (1<<0)
> -
> -#define S3C64XX_SPI_CS_NSC_CNT_2 (2<<4)
> -#define S3C64XX_SPI_CS_AUTO (1<<1)
> -#define S3C64XX_SPI_CS_SIG_INACT (1<<0)
> -
> -#define S3C64XX_SPI_INT_TRAILING_EN (1<<6)
> -#define S3C64XX_SPI_INT_RX_OVERRUN_EN (1<<5)
> -#define S3C64XX_SPI_INT_RX_UNDERRUN_EN (1<<4)
> -#define S3C64XX_SPI_INT_TX_OVERRUN_EN (1<<3)
> -#define S3C64XX_SPI_INT_TX_UNDERRUN_EN (1<<2)
> -#define S3C64XX_SPI_INT_RX_FIFORDY_EN (1<<1)
> -#define S3C64XX_SPI_INT_TX_FIFORDY_EN (1<<0)
> -
> -#define S3C64XX_SPI_ST_RX_OVERRUN_ERR (1<<5)
> -#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR (1<<4)
> -#define S3C64XX_SPI_ST_TX_OVERRUN_ERR (1<<3)
> -#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR (1<<2)
> -#define S3C64XX_SPI_ST_RX_FIFORDY (1<<1)
> -#define S3C64XX_SPI_ST_TX_FIFORDY (1<<0)
> -
> -#define S3C64XX_SPI_PACKET_CNT_EN (1<<16)
> +#define S3C64XX_SPI_MODE_SELF_LOOPBACK BIT(3)
> +#define S3C64XX_SPI_MODE_RXDMA_ON BIT(2)
> +#define S3C64XX_SPI_MODE_TXDMA_ON BIT(1)
> +#define S3C64XX_SPI_MODE_4BURST BIT(0)
> +
> +#define S3C64XX_SPI_CS_NSC_CNT_MASK GENMASK(9, 4)
> +#define S3C64XX_SPI_CS_NSC_CNT_2 2
> +#define S3C64XX_SPI_CS_AUTO BIT(1)
> +#define S3C64XX_SPI_CS_SIG_INACT BIT(0)
> +
> +#define S3C64XX_SPI_INT_TRAILING_EN BIT(6)
> +#define S3C64XX_SPI_INT_RX_OVERRUN_EN BIT(5)
> +#define S3C64XX_SPI_INT_RX_UNDERRUN_EN BIT(4)
> +#define S3C64XX_SPI_INT_TX_OVERRUN_EN BIT(3)
> +#define S3C64XX_SPI_INT_TX_UNDERRUN_EN BIT(2)
> +#define S3C64XX_SPI_INT_RX_FIFORDY_EN BIT(1)
> +#define S3C64XX_SPI_INT_TX_FIFORDY_EN BIT(0)
> +
> +#define S3C64XX_SPI_ST_RX_OVERRUN_ERR BIT(5)
> +#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR BIT(4)
> +#define S3C64XX_SPI_ST_TX_OVERRUN_ERR BIT(3)
> +#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR BIT(2)
> +#define S3C64XX_SPI_ST_RX_FIFORDY BIT(1)
> +#define S3C64XX_SPI_ST_TX_FIFORDY BIT(0)
> +
> +#define S3C64XX_SPI_PACKET_CNT_EN BIT(16)
> #define S3C64XX_SPI_PACKET_CNT_MASK GENMASK(15, 0)
>
> -#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR (1<<4)
> -#define S3C64XX_SPI_PND_TX_OVERRUN_CLR (1<<3)
> -#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR (1<<2)
> -#define S3C64XX_SPI_PND_RX_OVERRUN_CLR (1<<1)
> -#define S3C64XX_SPI_PND_TRAILING_CLR (1<<0)
> +#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR BIT(4)
> +#define S3C64XX_SPI_PND_TX_OVERRUN_CLR BIT(3)
> +#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR BIT(2)
> +#define S3C64XX_SPI_PND_RX_OVERRUN_CLR BIT(1)
> +#define S3C64XX_SPI_PND_TRAILING_CLR BIT(0)
>
> -#define S3C64XX_SPI_SWAP_RX_HALF_WORD (1<<7)
> -#define S3C64XX_SPI_SWAP_RX_BYTE (1<<6)
> -#define S3C64XX_SPI_SWAP_RX_BIT (1<<5)
> -#define S3C64XX_SPI_SWAP_RX_EN (1<<4)
> -#define S3C64XX_SPI_SWAP_TX_HALF_WORD (1<<3)
> -#define S3C64XX_SPI_SWAP_TX_BYTE (1<<2)
> -#define S3C64XX_SPI_SWAP_TX_BIT (1<<1)
> -#define S3C64XX_SPI_SWAP_TX_EN (1<<0)
> +#define S3C64XX_SPI_SWAP_RX_HALF_WORD BIT(7)
> +#define S3C64XX_SPI_SWAP_RX_BYTE BIT(6)
> +#define S3C64XX_SPI_SWAP_RX_BIT BIT(5)
> +#define S3C64XX_SPI_SWAP_RX_EN BIT(4)
> +#define S3C64XX_SPI_SWAP_TX_HALF_WORD BIT(3)
> +#define S3C64XX_SPI_SWAP_TX_BYTE BIT(2)
> +#define S3C64XX_SPI_SWAP_TX_BIT BIT(1)
> +#define S3C64XX_SPI_SWAP_TX_EN BIT(0)
>
> -#define S3C64XX_SPI_FBCLK_MSK (3<<0)
> +#define S3C64XX_SPI_FBCLK_MASK GENMASK(1, 0)
>
> #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
> #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
> @@ -112,18 +113,13 @@
> FIFO_LVL_MASK(i))
> #define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
>
> -#define S3C64XX_SPI_MAX_TRAILCNT 0x3ff
> -#define S3C64XX_SPI_TRAILCNT_OFF 19
> -
> -#define S3C64XX_SPI_TRAILCNT S3C64XX_SPI_MAX_TRAILCNT
> -
> #define S3C64XX_SPI_POLLING_SIZE 32
>
> #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> #define is_polling(x) (x->cntrlr_info->polling)
>
> -#define RXBUSY (1<<2)
> -#define TXBUSY (1<<3)
> +#define RXBUSY BIT(2)
> +#define TXBUSY BIT(3)
>
> struct s3c64xx_spi_dma_data {
> struct dma_chan *ch;
> @@ -342,8 +338,9 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
> } else {
> u32 ssel = readl(sdd->regs + S3C64XX_SPI_CS_REG);
>
> - ssel |= (S3C64XX_SPI_CS_AUTO |
> - S3C64XX_SPI_CS_NSC_CNT_2);
> + ssel |= S3C64XX_SPI_CS_AUTO |
> + FIELD_PREP(S3C64XX_SPI_CS_NSC_CNT_MASK,
> + S3C64XX_SPI_CS_NSC_CNT_2);
> writel(ssel, sdd->regs + S3C64XX_SPI_CS_REG);
> }
> } else {
> @@ -666,16 +663,22 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>
> switch (sdd->cur_bpw) {
> case 32:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_WORD;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_WORD) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_WORD);
> break;
> case 16:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> break;
> default:
> - val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE;
> - val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE;
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> + S3C64XX_SPI_MODE_BUS_TSZ_BYTE) |
> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> + S3C64XX_SPI_MODE_CH_TSZ_BYTE);

I don't know. Maybe it's me, but using this FIELD_PREP() macro seems
to only making the code harder to read. At least in cases like this. I
would vote against its usage, to keep the code compact and easy to
read.

> break;
> }
>
> @@ -801,7 +804,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>
> val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
> - val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
> + val |= FIELD_PREP(S3C64XX_SPI_MODE_RX_RDY_LVL, rdy_lv);
> writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
>
> /* Enable FIFO_RDY_EN IRQ */
> @@ -1074,8 +1077,8 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
> writel(0, regs + S3C64XX_SPI_INT_EN);
>
> if (!sdd->port_conf->clk_from_cmu)
> - writel(sci->src_clk_nr << S3C64XX_SPI_CLKSEL_SRCSHFT,
> - regs + S3C64XX_SPI_CLK_CFG);
> + writel(FIELD_PREP(S3C64XX_SPI_CLKSEL_SRCMSK, sci->src_clk_nr),
> + regs + S3C64XX_SPI_CLK_CFG);
> writel(0, regs + S3C64XX_SPI_MODE_CFG);
> writel(0, regs + S3C64XX_SPI_PACKET_CNT);
>
> @@ -1091,8 +1094,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>
> val = readl(regs + S3C64XX_SPI_MODE_CFG);
> val &= ~S3C64XX_SPI_MODE_4BURST;
> - val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);

Doesn't it change the behavior?

> - val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> + val |= S3C64XX_SPI_MAX_TRAILCNT_MASK;
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
> s3c64xx_flush_fifo(sdd);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Next message: Jonathan Corbet: "Re: [PATCH 6/8] Documentation: Create a new folder for all timer internals"
Previous message: Stefan O'Rear: "Re: [RFC PATCH v1 07/28] riscv: kernel handling on trap entry/exit for user cfi"
In reply to: Tudor Ambarus: "[PATCH v2 09/28] spi: s3c64xx: use bitfield access macros"
Next in thread: Tudor Ambarus: "Re: [PATCH v2 09/28] spi: s3c64xx: use bitfield access macros"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]