[PATCH v2 05/19] ASoC: fsl_micfil: use GENMASK to define register bit fields

[Sascha Hauer]

On Thu, Apr 07, 2022 at 10:08:38AM +0800, Shengjiu Wang wrote:
>    On Mon, Mar 28, 2022 at 7:28 PM Sascha Hauer <[1]s.hauer at pengutronix.de>
>    wrote:
> 
>      Use GENMASK along with FIELD_PREP and FIELD_GET to access bitfields in
>      registers to straighten register access and to drop a lot of defines.
> 
>      Signed-off-by: Sascha Hauer <[2]s.hauer at pengutronix.de>
>      ---
> 
>      Notes:
>          Changes since v1:
>          - add missing include linux/bitfield.h
> 
>       sound/soc/fsl/fsl_micfil.c |  52 ++++++-------
>       sound/soc/fsl/fsl_micfil.h | 147 ++++++++-----------------------------
>       2 files changed, 58 insertions(+), 141 deletions(-)
> 
>      diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c
>      index 878d24fde3581..cfa8af668d921 100644
>      --- a/sound/soc/fsl/fsl_micfil.c
>      +++ b/sound/soc/fsl/fsl_micfil.c
>      @@ -1,6 +1,7 @@
>       // SPDX-License-Identifier: GPL-2.0
>       // Copyright 2018 NXP
> 
>      +#include <linux/bitfield.h>
>       #include <linux/clk.h>
>       #include <linux/device.h>
>       #include <linux/interrupt.h>
>      @@ -116,23 +117,22 @@ static inline int get_pdm_clk(struct fsl_micfil
>      *micfil,
>              int bclk;
> 
>              regmap_read(micfil->regmap, REG_MICFIL_CTRL2, &ctrl2_reg);
>      -       osr = 16 - ((ctrl2_reg & MICFIL_CTRL2_CICOSR_MASK)
>      -                   >> MICFIL_CTRL2_CICOSR_SHIFT);
>      -       qsel = ctrl2_reg & MICFIL_CTRL2_QSEL_MASK;
>      +       osr = 16 - FIELD_GET(MICFIL_CTRL2_CICOSR, ctrl2_reg);
>      +       qsel = FIELD_GET(MICFIL_CTRL2_QSEL, ctrl2_reg);
> 
>              switch (qsel) {
>      -       case MICFIL_HIGH_QUALITY:
>      +       case MICFIL_QSEL_HIGH_QUALITY:
>                      bclk = rate * 8 * osr / 2; /* kfactor = 0.5 */
>                      break;
>      -       case MICFIL_MEDIUM_QUALITY:
>      -       case MICFIL_VLOW0_QUALITY:
>      +       case MICFIL_QSEL_MEDIUM_QUALITY:
>      +       case MICFIL_QSEL_VLOW0_QUALITY:
>                      bclk = rate * 4 * osr * 1; /* kfactor = 1 */
>                      break;
>      -       case MICFIL_LOW_QUALITY:
>      -       case MICFIL_VLOW1_QUALITY:
>      +       case MICFIL_QSEL_LOW_QUALITY:
>      +       case MICFIL_QSEL_VLOW1_QUALITY:
>                      bclk = rate * 2 * osr * 2; /* kfactor = 2 */
>                      break;
>      -       case MICFIL_VLOW2_QUALITY:
>      +       case MICFIL_QSEL_VLOW2_QUALITY:
>                      bclk = rate * osr * 4; /* kfactor = 4 */
>                      break;
>              default:
>      @@ -244,8 +244,8 @@ static int fsl_micfil_trigger(struct
>      snd_pcm_substream *substream, int cmd,
>                       * 11 - reserved
>                       */
>                      ret = regmap_update_bits(micfil->regmap,
>      REG_MICFIL_CTRL1,
>      -                                        MICFIL_CTRL1_DISEL_MASK,
>      -                                        (1 <<
>      MICFIL_CTRL1_DISEL_SHIFT));
>      +                               MICFIL_CTRL1_DISEL,
>      +                               FIELD_PREP(MICFIL_CTRL1_DISEL,
>      MICFIL_CTRL1_DISEL_DMA));
> 
>    Alignment should match open parenthesis?

Generally yes, but in this case this would introduce an additional
linebreak inside the FIELD_PREP macro which reduces readability:

Instead of:

	ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL1,
			MICFIL_CTRL1_DISEL,
			FIELD_PREP(MICFIL_CTRL1_DISEL, MICFIL_CTRL1_DISEL_DMA));

We would have:

	ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL1,
				 MICFIL_CTRL1_DISEL,
				 FIELD_PREP(MICFIL_CTRL1_DISEL,
				 MICFIL_CTRL1_DISEL_DMA));

> 
>                      ret = regmap_update_bits(micfil->regmap,
>      REG_MICFIL_CTRL1,
>      -                                        MICFIL_CTRL1_DISEL_MASK,
>      -                                        (0 <<
>      MICFIL_CTRL1_DISEL_SHIFT));
>      +                               MICFIL_CTRL1_DISEL,
>      +                               FIELD_PREP(MICFIL_CTRL1_DISEL,
>      MICFIL_CTRL1_DISEL_DISABLE));
> 
>    Alignment should match open parenthesis? 

Same here.

>     
> 
>                      if (ret) {
>                              dev_err(dev, "failed to update DISEL bits\n");
>                              return ret;
>      @@ -300,8 +300,8 @@ static int fsl_set_clock_params(struct device *dev,
>      unsigned int rate)
> 
>              /* set CICOSR */
>              ret |= regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2,
>      -                                MICFIL_CTRL2_CICOSR_MASK,
>      -                                MICFIL_CTRL2_OSR_DEFAULT);
>      +                                MICFIL_CTRL2_CICOSR,
>      +                                FIELD_PREP(MICFIL_CTRL2_CICOSR,
>      MICFIL_CTRL2_CICOSR_DEFAULT));
> 
>     Alignment should match open parenthesis? 

This is fixed in one of the next patches where the '|=' is replaced with '='.
It reduces the number of lines changed in that patch, so I think this is ok
here.

> 
>              if (ret)
>                      dev_err(dev, "failed to set CICOSR in reg 0x%X\n",
>                              REG_MICFIL_CTRL2);
>      @@ -312,7 +312,8 @@ static int fsl_set_clock_params(struct device *dev,
>      unsigned int rate)
>                      ret = -EINVAL;
> 
>              ret |= regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2,
>      -                                MICFIL_CTRL2_CLKDIV_MASK, clk_div);
>      +                                MICFIL_CTRL2_CLKDIV,
>      +                                FIELD_PREP(MICFIL_CTRL2_CLKDIV,
>      clk_div));
> 
>    Alignment should match open parenthesis?

Ditto.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |