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_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.
