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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> ---
>
> 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 |
