Hi Jonathan, jic23@xxxxxxxxxx wrote on Sat, 18 Sep 2021 17:31:54 +0100: > On Wed, 15 Sep 2021 17:58:49 +0200 > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > > Drop unused and useless definitions from the header. Besides the STEP > > ENABLE register which is highly unclear (and not used), drop all the > > Agreed - I started trying to figure out what they were in the earlier patch! > > > "masks" definitions which are only used by the following definition. It > > could be possible to got even further by removing these definitions > > entirely and use FIELD_PREP() macros from the code directly, but while I > > have no troubles making these changes in the header, changing the values > > in the code directly could IMHO darkening a bit the logic and > > furthermore hardening future git-blames. > > Hmm. Maybe on that... I'm not that bothered either way but there is > definitely clarity in FIELD_PREP being used inline for writes to a device. > You can very clearly see what is going on. > > Note that it only really works here because the driver only ever uses > the masks to 'set' the value, but never to read any of them back from the > hardware. > > Your point about it making a messy history is true of almost any change :) > > > > > Certain macros are using GENMASK() to define the value of a particular > > field, while this is purely "by chance" that the value and the mask have > > the same value. In this case, drop the "mask" definition, use > > FIELD_PREP() and GENMASK() in the macro defining the field, and use the > > new macro to define the particular value by feeding directly the actual > > number advertised in the datasheet into that macro, as in: > > -#define STEPCONFIG_RFM_VREFN GENMASK(24, 23) > > -#define STEPCONFIG_RFM(val) FIELD_PREP(STEPCONFIG_RFM_VREFN, (val)) > > +#define STEPCONFIG_RFM(val) FIELD_PREP(GENMASK(24, 23), (val)) > > +#define STEPCONFIG_RFM_VREFN STEPCONFIG_RFM(3) > > This is indeed an improvement. > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx> > > I'm a bit in two minds out about how you should handle the multiple patches > involved in cleaning these up. Definitely not good to do modifications on > elements you are going to drop - so for those pull them out earlier. > > The others are a little odd because you first introduce some of the GENMASK stuff > then rework it in this patch. Perhaps this split is the best way to handle those. I must admit I got lazy, the ordering reflects the order of my decisions and once these made, it was too painful to rebase and move this patch earlier but I fully understand the request :) I will ping Lee in the cover letter to ask him what is his feedback over the entire series and if he agrees with the main idea I whish I could only respin these three patches in the right order in v4 and request him to take v3 for all the other patches. Thanks, Miquèl
