Re: [PATCH v3 28/47] mfd: ti_am335x_tscadc: Drop useless definitions from the header

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux