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