| Message ID | 20240125145007.748295-10-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
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 @@ > // 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 >
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.
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 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.
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);
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(-)