| Message ID | 20210915155908.476767-29-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Commit | 0fd12262613116a38fd76aefa396e9bc116fbfe2 |
| Headers | show |
| Series | TI AM437X ADC1 | expand |
On Wed, 15 Sep 2021 17:58:49 +0200 Miquel Raynal <miquel.raynal@bootlin.com> 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@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. Jonathan > --- > include/linux/mfd/ti_am335x_tscadc.h | 51 +++++++++------------------- > 1 file changed, 16 insertions(+), 35 deletions(-) > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h > index babc2e36c5d0..32b26e56eebb 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -40,13 +40,6 @@ > /* IRQ wakeup enable */ > #define IRQWKUP_ENB BIT(0) > > -/* Step Enable */ > -#define STEPENB_MASK GENMASK(16, 0) > -#define STEPENB(val) FIELD_PREP(STEPENB_MASK, (val)) > -#define ENB(val) BIT(val) > -#define STPENB_STEPENB STEPENB(GENMASK(16, 0)) > -#define STPENB_STEPENB_TC STEPENB(GENMASK(12, 0)) > - With this first block moved much earlier in the series - to before any of the other patches touch it. Reviwed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > /* IRQ enable */ > #define IRQENB_HW_PEN BIT(0) > #define IRQENB_EOS BIT(1) > @@ -59,12 +52,10 @@ > #define IRQENB_PENUP BIT(9) > > /* Step Configuration */ > -#define STEPCONFIG_MODE_MASK GENMASK(1, 0) > -#define STEPCONFIG_MODE(val) FIELD_PREP(STEPCONFIG_MODE_MASK, (val)) > +#define STEPCONFIG_MODE(val) FIELD_PREP(GENMASK(1, 0), (val)) > #define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1) > #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2) > -#define STEPCONFIG_AVG_MASK GENMASK(4, 2) > -#define STEPCONFIG_AVG(val) FIELD_PREP(STEPCONFIG_AVG_MASK, (val)) > +#define STEPCONFIG_AVG(val) FIELD_PREP(GENMASK(4, 2), (val)) > #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4) > #define STEPCONFIG_XPP BIT(5) > #define STEPCONFIG_XNN BIT(6) > @@ -72,45 +63,36 @@ > #define STEPCONFIG_YNN BIT(8) > #define STEPCONFIG_XNP BIT(9) > #define STEPCONFIG_YPN BIT(10) > -#define STEPCONFIG_RFP_VREFP GENMASK(13, 12) > -#define STEPCONFIG_RFP(val) FIELD_PREP(STEPCONFIG_RFP_VREFP, (val)) > -#define STEPCONFIG_INM_MASK GENMASK(18, 15) > -#define STEPCONFIG_INM(val) FIELD_PREP(STEPCONFIG_INM_MASK, (val)) > +#define STEPCONFIG_RFP(val) FIELD_PREP(GENMASK(13, 12), (val)) > +#define STEPCONFIG_RFP_VREFP STEPCONFIG_RFP(3) > +#define STEPCONFIG_INM(val) FIELD_PREP(GENMASK(18, 15), (val)) > #define STEPCONFIG_INM_ADCREFM STEPCONFIG_INM(8) > -#define STEPCONFIG_INP_MASK GENMASK(22, 19) > -#define STEPCONFIG_INP(val) FIELD_PREP(STEPCONFIG_INP_MASK, (val)) > +#define STEPCONFIG_INP(val) FIELD_PREP(GENMASK(22, 19), (val)) > #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4) > #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8) > #define STEPCONFIG_FIFO1 BIT(26) > -#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) > > /* Delay register */ > -#define STEPDELAY_OPEN_MASK GENMASK(17, 0) > -#define STEPDELAY_OPEN(val) FIELD_PREP(STEPDELAY_OPEN_MASK, (val)) > +#define STEPDELAY_OPEN(val) FIELD_PREP(GENMASK(17, 0), (val)) > #define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098) > -#define STEPDELAY_SAMPLE_MASK GENMASK(31, 24) > -#define STEPDELAY_SAMPLE(val) FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val)) > #define STEPCONFIG_MAX_OPENDLY GENMASK(17, 0) > +#define STEPDELAY_SAMPLE(val) FIELD_PREP(GENMASK(31, 24), (val)) > #define STEPCONFIG_SAMPLEDLY STEPDELAY_SAMPLE(0) > #define STEPCONFIG_MAX_SAMPLE GENMASK(7, 0) > > /* Charge Config */ > -#define STEPCHARGE_RFP_MASK GENMASK(14, 12) > -#define STEPCHARGE_RFP(val) FIELD_PREP(STEPCHARGE_RFP_MASK, (val)) > +#define STEPCHARGE_RFP(val) FIELD_PREP(GENMASK(14, 12), (val)) > #define STEPCHARGE_RFP_XPUL STEPCHARGE_RFP(1) > -#define STEPCHARGE_INM_MASK GENMASK(18, 15) > -#define STEPCHARGE_INM(val) FIELD_PREP(STEPCHARGE_INM_MASK, (val)) > +#define STEPCHARGE_INM(val) FIELD_PREP(GENMASK(18, 15), (val)) > #define STEPCHARGE_INM_AN1 STEPCHARGE_INM(1) > -#define STEPCHARGE_INP_MASK GENMASK(22, 19) > -#define STEPCHARGE_INP(val) FIELD_PREP(STEPCHARGE_INP_MASK, (val)) > -#define STEPCHARGE_RFM_MASK GENMASK(24, 23) > -#define STEPCHARGE_RFM(val) FIELD_PREP(STEPCHARGE_RFM_MASK, (val)) > +#define STEPCHARGE_INP(val) FIELD_PREP(GENMASK(22, 19), (val)) > +#define STEPCHARGE_RFM(val) FIELD_PREP(GENMASK(24, 23), (val)) > #define STEPCHARGE_RFM_XNUR STEPCHARGE_RFM(1) > > /* Charge delay */ > -#define CHARGEDLY_OPEN_MASK GENMASK(17, 0) > -#define CHARGEDLY_OPEN(val) FIELD_PREP(CHARGEDLY_OPEN_MASK, (val)) > +#define CHARGEDLY_OPEN(val) FIELD_PREP(GENMASK(17, 0), (val)) > #define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(0x400) > > /* Control register */ > @@ -118,8 +100,7 @@ > #define CNTRLREG_STEPID BIT(1) > #define CNTRLREG_STEPCONFIGWRT BIT(2) > #define CNTRLREG_POWERDOWN BIT(4) > -#define CNTRLREG_AFE_CTRL_MASK GENMASK(6, 5) > -#define CNTRLREG_AFE_CTRL(val) FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val)) > +#define CNTRLREG_AFE_CTRL(val) FIELD_PREP(GENMASK(6, 5), (val)) > #define CNTRLREG_4WIRE CNTRLREG_AFE_CTRL(1) > #define CNTRLREG_5WIRE CNTRLREG_AFE_CTRL(2) > #define CNTRLREG_8WIRE CNTRLREG_AFE_CTRL(3)
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 <miquel.raynal@bootlin.com> 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@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
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h index babc2e36c5d0..32b26e56eebb 100644 --- a/include/linux/mfd/ti_am335x_tscadc.h +++ b/include/linux/mfd/ti_am335x_tscadc.h @@ -40,13 +40,6 @@ /* IRQ wakeup enable */ #define IRQWKUP_ENB BIT(0) -/* Step Enable */ -#define STEPENB_MASK GENMASK(16, 0) -#define STEPENB(val) FIELD_PREP(STEPENB_MASK, (val)) -#define ENB(val) BIT(val) -#define STPENB_STEPENB STEPENB(GENMASK(16, 0)) -#define STPENB_STEPENB_TC STEPENB(GENMASK(12, 0)) - /* IRQ enable */ #define IRQENB_HW_PEN BIT(0) #define IRQENB_EOS BIT(1) @@ -59,12 +52,10 @@ #define IRQENB_PENUP BIT(9) /* Step Configuration */ -#define STEPCONFIG_MODE_MASK GENMASK(1, 0) -#define STEPCONFIG_MODE(val) FIELD_PREP(STEPCONFIG_MODE_MASK, (val)) +#define STEPCONFIG_MODE(val) FIELD_PREP(GENMASK(1, 0), (val)) #define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1) #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2) -#define STEPCONFIG_AVG_MASK GENMASK(4, 2) -#define STEPCONFIG_AVG(val) FIELD_PREP(STEPCONFIG_AVG_MASK, (val)) +#define STEPCONFIG_AVG(val) FIELD_PREP(GENMASK(4, 2), (val)) #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4) #define STEPCONFIG_XPP BIT(5) #define STEPCONFIG_XNN BIT(6) @@ -72,45 +63,36 @@ #define STEPCONFIG_YNN BIT(8) #define STEPCONFIG_XNP BIT(9) #define STEPCONFIG_YPN BIT(10) -#define STEPCONFIG_RFP_VREFP GENMASK(13, 12) -#define STEPCONFIG_RFP(val) FIELD_PREP(STEPCONFIG_RFP_VREFP, (val)) -#define STEPCONFIG_INM_MASK GENMASK(18, 15) -#define STEPCONFIG_INM(val) FIELD_PREP(STEPCONFIG_INM_MASK, (val)) +#define STEPCONFIG_RFP(val) FIELD_PREP(GENMASK(13, 12), (val)) +#define STEPCONFIG_RFP_VREFP STEPCONFIG_RFP(3) +#define STEPCONFIG_INM(val) FIELD_PREP(GENMASK(18, 15), (val)) #define STEPCONFIG_INM_ADCREFM STEPCONFIG_INM(8) -#define STEPCONFIG_INP_MASK GENMASK(22, 19) -#define STEPCONFIG_INP(val) FIELD_PREP(STEPCONFIG_INP_MASK, (val)) +#define STEPCONFIG_INP(val) FIELD_PREP(GENMASK(22, 19), (val)) #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4) #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8) #define STEPCONFIG_FIFO1 BIT(26) -#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) /* Delay register */ -#define STEPDELAY_OPEN_MASK GENMASK(17, 0) -#define STEPDELAY_OPEN(val) FIELD_PREP(STEPDELAY_OPEN_MASK, (val)) +#define STEPDELAY_OPEN(val) FIELD_PREP(GENMASK(17, 0), (val)) #define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098) -#define STEPDELAY_SAMPLE_MASK GENMASK(31, 24) -#define STEPDELAY_SAMPLE(val) FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val)) #define STEPCONFIG_MAX_OPENDLY GENMASK(17, 0) +#define STEPDELAY_SAMPLE(val) FIELD_PREP(GENMASK(31, 24), (val)) #define STEPCONFIG_SAMPLEDLY STEPDELAY_SAMPLE(0) #define STEPCONFIG_MAX_SAMPLE GENMASK(7, 0) /* Charge Config */ -#define STEPCHARGE_RFP_MASK GENMASK(14, 12) -#define STEPCHARGE_RFP(val) FIELD_PREP(STEPCHARGE_RFP_MASK, (val)) +#define STEPCHARGE_RFP(val) FIELD_PREP(GENMASK(14, 12), (val)) #define STEPCHARGE_RFP_XPUL STEPCHARGE_RFP(1) -#define STEPCHARGE_INM_MASK GENMASK(18, 15) -#define STEPCHARGE_INM(val) FIELD_PREP(STEPCHARGE_INM_MASK, (val)) +#define STEPCHARGE_INM(val) FIELD_PREP(GENMASK(18, 15), (val)) #define STEPCHARGE_INM_AN1 STEPCHARGE_INM(1) -#define STEPCHARGE_INP_MASK GENMASK(22, 19) -#define STEPCHARGE_INP(val) FIELD_PREP(STEPCHARGE_INP_MASK, (val)) -#define STEPCHARGE_RFM_MASK GENMASK(24, 23) -#define STEPCHARGE_RFM(val) FIELD_PREP(STEPCHARGE_RFM_MASK, (val)) +#define STEPCHARGE_INP(val) FIELD_PREP(GENMASK(22, 19), (val)) +#define STEPCHARGE_RFM(val) FIELD_PREP(GENMASK(24, 23), (val)) #define STEPCHARGE_RFM_XNUR STEPCHARGE_RFM(1) /* Charge delay */ -#define CHARGEDLY_OPEN_MASK GENMASK(17, 0) -#define CHARGEDLY_OPEN(val) FIELD_PREP(CHARGEDLY_OPEN_MASK, (val)) +#define CHARGEDLY_OPEN(val) FIELD_PREP(GENMASK(17, 0), (val)) #define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(0x400) /* Control register */ @@ -118,8 +100,7 @@ #define CNTRLREG_STEPID BIT(1) #define CNTRLREG_STEPCONFIGWRT BIT(2) #define CNTRLREG_POWERDOWN BIT(4) -#define CNTRLREG_AFE_CTRL_MASK GENMASK(6, 5) -#define CNTRLREG_AFE_CTRL(val) FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val)) +#define CNTRLREG_AFE_CTRL(val) FIELD_PREP(GENMASK(6, 5), (val)) #define CNTRLREG_4WIRE CNTRLREG_AFE_CTRL(1) #define CNTRLREG_5WIRE CNTRLREG_AFE_CTRL(2) #define CNTRLREG_8WIRE CNTRLREG_AFE_CTRL(3)
Drop unused and useless definitions from the header. Besides the STEP ENABLE register which is highly unclear (and not used), drop all the "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. 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) Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/linux/mfd/ti_am335x_tscadc.h | 51 +++++++++------------------- 1 file changed, 16 insertions(+), 35 deletions(-)