On Fri, Jan 26, 2024 at 2:49 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote: > > > > 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 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.
