Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

From: Sam Protsenko
Date: Fri Jan 26 2024 - 15:12:46 EST

Next message: Steven Rostedt: "[RESEND] [PATCH] eventfs: Have inodes have unique inode numbers"
Previous message: Sam Protsenko: "Re: [PATCH v3 16/17] spi: s3c64xx: drop a superfluous bitwise NOT operation"
In reply to: Tudor Ambarus: "[PATCH v3 17/17] spi: s3c64xx: use bitfield access macros"
Next in thread: Tudor Ambarus: "Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
>
> Use the bitfield access macros in order to clean and to make the driver
> easier to read. Introduce S3C64XX_SPI_MAX_TRAILCNT_MASK to replace value
> and offset equivalents (S3C64XX_SPI_MAX_TRAILCNT,
> S3C64XX_SPI_TRAILCNT_OFF). While touching the register definitions, align
> their values to the same offset.
>
> No functional change intended, the bit operations shall be equivalent.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> drivers/spi/spi-s3c64xx.c | 193 ++++++++++++++++++++------------------
> 1 file changed, 101 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 43b888c8812e..7f052d6cd2ba 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,96 @@
> #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(7, 0)
> +
> +#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)
> +
> +/*
> + * S3C64XX_SPI_CS_NSC_CNT_2 is a value into the NCS_TIME_COUNT field. In newer
> + * datasheets this field is defined as GENMASK(9, 4). We don't know if this mask
> + * applies to all the versions of the IP, thus we can't yet define
> + * S3C64XX_SPI_CS_NSC_CNT_2 as a value and the register field as a mask.
> + */
> +#define S3C64XX_SPI_CS_NSC_CNT_2 (2 << 4)
> +#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,16 +118,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_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;
> @@ -664,16 +667,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);

Two people complained it makes the code harder to read. Yet it's not
addressed in v3. Please see my comments for your previous submission
explaining what can be done, and also Andi's comment on that matter.
Also I think new patch series are being submitted a bit too fast,
people might not have enough time to provide the review.

> 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);
> break;
> }
>
> @@ -799,7 +808,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 */
> @@ -1072,8 +1081,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);
>
> @@ -1089,7 +1098,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);
> + 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: Steven Rostedt: "[RESEND] [PATCH] eventfs: Have inodes have unique inode numbers"
Previous message: Sam Protsenko: "Re: [PATCH v3 16/17] spi: s3c64xx: drop a superfluous bitwise NOT operation"
In reply to: Tudor Ambarus: "[PATCH v3 17/17] spi: s3c64xx: use bitfield access macros"
Next in thread: Tudor Ambarus: "Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]