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

Hi Jonathan,

jic23@kernel.org wrote on Sat, 18 Sep 2021 17:31:54 +0100:

On Wed, 15 Sep 2021 17:58:49 +0200
Miquel Raynal [off-list ref] wrote:

quoted

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!

quoted

"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 :)

quoted

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.

quoted

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  

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