Re: [PATCH v2 25/46] mfd: ti_am335x_tscadc: Use BIT() and GENMASK() when relevant in the header

Jonathan Cameron
2021-09-05T13:06:25Z

On Thu, 2 Sep 2021 23:51:23 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Convert masks to GENMASK() and regular shifts to BIT() in
> ti_am335x_tscadc.h header.
>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Might be better two squash this and next patch together. I wrote a bunch
of 'use FIELD_PREP() here' comments before realising you've done that separately.

Still, for what is here looks good.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> include/linux/mfd/ti_am335x_tscadc.h | 40 ++++++++++++++--------------
> 1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index efafecfc87a7..4de16fc3d74f 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -39,11 +39,11 @@
> #define IRQWKUP_ENB BIT(0)
>
> /* Step Enable */
> -#define STEPENB_MASK (0x1FFFF << 0)
> +#define STEPENB_MASK GENMASK(16, 0)
> #define STEPENB(val) ((val) << 0)
> -#define ENB(val) (1 << (val))
> -#define STPENB_STEPENB STEPENB(0x1FFFF)
> -#define STPENB_STEPENB_TC STEPENB(0x1FFF)
> +#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)
> @@ -57,11 +57,11 @@
> #define IRQENB_PENUP BIT(9)
>
> /* Step Configuration */
> -#define STEPCONFIG_MODE_MASK (3 << 0)
> +#define STEPCONFIG_MODE_MASK GENMASK(1, 0)
> #define STEPCONFIG_MODE(val) ((val) << 0)
> #define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
> #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
> -#define STEPCONFIG_AVG_MASK (7 << 2)
> +#define STEPCONFIG_AVG_MASK GENMASK(4, 2)
> #define STEPCONFIG_AVG(val) ((val) << 2)
> #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4)
> #define STEPCONFIG_XPP BIT(5)
> @@ -71,41 +71,41 @@
> #define STEPCONFIG_XNP BIT(9)
> #define STEPCONFIG_YPN BIT(10)
> #define STEPCONFIG_RFP(val) ((val) << 12)
> -#define STEPCONFIG_RFP_VREFP (0x3 << 12)
> -#define STEPCONFIG_INM_MASK (0xF << 15)
> +#define STEPCONFIG_RFP_VREFP GENMASK(13, 12)
> +#define STEPCONFIG_INM_MASK GENMASK(18, 15)
> #define STEPCONFIG_INM(val) ((val) << 15)
> #define STEPCONFIG_INM_ADCREFM STEPCONFIG_INM(8)
> -#define STEPCONFIG_INP_MASK (0xF << 19)
> +#define STEPCONFIG_INP_MASK GENMASK(22, 19)
> #define STEPCONFIG_INP(val) ((val) << 19)
> #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4)
> #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8)
> #define STEPCONFIG_FIFO1 BIT(26)
> #define STEPCONFIG_RFM(val) ((val) << 23)
> -#define STEPCONFIG_RFM_VREFN (0x3 << 23)
> +#define STEPCONFIG_RFM_VREFN GENMASK(24, 23)
>
> /* Delay register */
> -#define STEPDELAY_OPEN_MASK (0x3FFFF << 0)
> +#define STEPDELAY_OPEN_MASK GENMASK(17, 0)
> #define STEPDELAY_OPEN(val) ((val) << 0)
> #define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098)
> -#define STEPDELAY_SAMPLE_MASK (0xFF << 24)
> +#define STEPDELAY_SAMPLE_MASK GENMASK(31, 24)
> #define STEPDELAY_SAMPLE(val) ((val) << 24)
> #define STEPCONFIG_SAMPLEDLY STEPDELAY_SAMPLE(0)
>
> /* Charge Config */
> -#define STEPCHARGE_RFP_MASK (7 << 12)
> +#define STEPCHARGE_RFP_MASK GENMASK(14, 12)
> #define STEPCHARGE_RFP(val) ((val) << 12)
> #define STEPCHARGE_RFP_XPUL STEPCHARGE_RFP(1)
> -#define STEPCHARGE_INM_MASK (0xF << 15)
> +#define STEPCHARGE_INM_MASK GENMASK(18, 15)
> #define STEPCHARGE_INM(val) ((val) << 15)
> #define STEPCHARGE_INM_AN1 STEPCHARGE_INM(1)
> -#define STEPCHARGE_INP_MASK (0xF << 19)
> +#define STEPCHARGE_INP_MASK GENMASK(22, 19)
> #define STEPCHARGE_INP(val) ((val) << 19)
> -#define STEPCHARGE_RFM_MASK (3 << 23)
> +#define STEPCHARGE_RFM_MASK GENMASK(24, 23)
> #define STEPCHARGE_RFM(val) ((val) << 23)
> #define STEPCHARGE_RFM_XNUR STEPCHARGE_RFM(1)
>
> /* Charge delay */
> -#define CHARGEDLY_OPEN_MASK (0x3FFFF << 0)
> +#define CHARGEDLY_OPEN_MASK GENMASK(17, 0)
> #define CHARGEDLY_OPEN(val) ((val) << 0)
> #define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(0x400)
>
> @@ -114,7 +114,7 @@
> #define CNTRLREG_STEPID BIT(1)
> #define CNTRLREG_STEPCONFIGWRT BIT(2)
> #define CNTRLREG_POWERDOWN BIT(4)
> -#define CNTRLREG_AFE_CTRL_MASK (3 << 5)
> +#define CNTRLREG_AFE_CTRL_MASK GENMASK(6, 5)
> #define CNTRLREG_AFE_CTRL(val) ((val) << 5)
> #define CNTRLREG_4WIRE CNTRLREG_AFE_CTRL(1)
> #define CNTRLREG_5WIRE CNTRLREG_AFE_CTRL(2)
> @@ -122,8 +122,8 @@
> #define CNTRLREG_TSCENB BIT(7)
>
> /* FIFO READ Register */
> -#define FIFOREAD_DATA_MASK (0xfff << 0)
> -#define FIFOREAD_CHNLID_MASK (0xf << 16)
> +#define FIFOREAD_DATA_MASK GENMASK(11, 0)
> +#define FIFOREAD_CHNLID_MASK GENMASK(19, 16)
>
> /* DMA ENABLE/CLEAR Register */
> #define DMA_FIFO0 BIT(0)