Use the bitfield access macros in order to clean and to make the driver
easier to read.
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
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@samsung.com>
#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)
+
+#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);
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);
- 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-googOn Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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@linaro.org>
> ---
> 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@samsung.com>
>
> #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
>
Hi, Sam, I just noticed that I haven't responded to a question you had. On 1/25/24 19:50, Sam Protsenko wrote: > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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@linaro.org> >> --- >> 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 @@ cut >> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK GENMASK(28, 19) cut >> +#define S3C64XX_SPI_CS_NSC_CNT_MASK GENMASK(9, 4) I was wrong introducing this mask because I can't tell if it applies to all the versions of the IP. Thus I'll keep S3C64XX_SPI_CS_NSC_CNT_2 defined as (2 << 4) and add the following comment on top of it: /* * 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) cut >> -#define S3C64XX_SPI_MAX_TRAILCNT 0x3ff >> -#define S3C64XX_SPI_TRAILCNT_OFF 19 >> - >> -#define S3C64XX_SPI_TRAILCNT S3C64XX_SPI_MAX_TRAILCNT >> - cut >> @@ -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? No, I don't think it does. so above we wipe the mask, it's equivalent to: val &= ~(GENMASK(28, 19)) > >> - val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF); and above we set the entire mask: val |= GENMASK(28, 19) the wipe is not necessary. This can be done in a separate patch of course, but I considered that if I removed the shift, the value and replaced them with the mask, I get the liberty of using the mask directly. I'll split this op in a separate patch (it starts to feel tiring). I verified the entire patch again, apart of the problem with the wrong mask for S3C64XX_SPI_PSR_MASK and the problem that I specified with S3C64XX_SPI_CS_NSC_CNT_MASK everything shall be fine. All the bits handling shall be equivalent.
On 1/25/24 19:50, Sam Protsenko wrote: > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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@linaro.org> >> --- >> 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 @@ cut >> +#define S3C64XX_SPI_PSR_MASK GENMASK(15, 0) > > But it was 0xff (7:0) originally, and here you extend it up to 15:0. this is a bug from my side, I'll fix it, thanks! cut >> 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. I saw Andi complained about this too, maybe Mark can chime in. To me this is not a matter of taste, it's how it should be done. In this particular case you have more lines when using FIELD_PREP because the mask starts from bit 0. If the mask ever changes for new IPs then you'd have to hack the code, whereas if using FIELD_PREP you just have to update the mask field, something like: FIELD_PREP(drv_prv_data->whatever_reg.field_mask, S3C64XX_SPI_MODE_CH_TSZ_BYTE); Thus it makes the code generic and more friendly for new IP additions. And I have to admit I like it better too. I know from the start that we're dealing with register fields and not some internal driver mask.
On Fri, Jan 26, 2024 at 2:49 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 1/25/24 19:50, Sam Protsenko wrote: > > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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@linaro.org> > >> --- > >> 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 @@ > > cut > > >> +#define S3C64XX_SPI_PSR_MASK GENMASK(15, 0) > > > > But it was 0xff (7:0) originally, and here you extend it up to 15:0. > > this is a bug from my side, I'll fix it, thanks! > > cut > > >> 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. > > I saw Andi complained about this too, maybe Mark can chime in. > > To me this is not a matter of taste, it's how it should be done. In this Sure. But if you think it has to be done, I suggest it's done taking next things into the account: 1. It shouldn't make code harder to read 2. Preferably stick to canonical ways of doing things 3. IMHO patches like this *must* be tested thoroughly on different boards with different test-cases, to make sure there are no regressions. Because the benefits of cleanups are not that great, as I see it, but we are risking to break some hardware/software combination unintentionally while doing those cleanups. It's a good idea to describe how it was tested in commit message or PATCH #0. Just my $.02. For (1) and (2): I noticed a lot of drivers are carrying additional helper functions for read/write operations, to keep things tidy and functional at the same time. Another mechanism that comes into mind is regmap, though I'm not sure if it's needed for such low-level entities as bus drivers. Also I think Andi has a point about FIELD_PREP and how that can be handled. > particular case you have more lines when using FIELD_PREP because the > mask starts from bit 0. If the mask ever changes for new IPs then you'd > have to hack the code, whereas if using FIELD_PREP you just have to > update the mask field, something like: > > FIELD_PREP(drv_prv_data->whatever_reg.field_mask, > S3C64XX_SPI_MODE_CH_TSZ_BYTE); > > Thus it makes the code generic and more friendly for new IP additions. > And I have to admit I like it better too. I know from the start that > we're dealing with register fields and not some internal driver mask.
© 2016 - 2025 Red Hat, Inc.