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@xxxxxxxxxx> > --- ... > -#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
