Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers

[Srinivas Kandagatla]

On 14/05/2020 17:38, Ajit Pandey wrote:
> I2SCTL and DMACTL registers has different bits alignment for newer
> LPASS variants of SC7180 soc. Instead of adding extra overhead for
> calculating masks and shifts for newer variants registers layout we
> changed the approach to use regmap_field_write() API to update bit.
> Such API's will internally do the required bit shift and mask based
> on reg_field struct defined for bit fields. We'll define REG_FIELD()
> macros with bit layout for both lpass variants and use such macros
> to initialize register fields in variant specific driver callbacks.
> Also added new bitfieds values for I2SCTL and DMACTL registers and
> removed shifts and mask macros for such registers from header file.
>
> Signed-off-by: Ajit Pandey <ajitp@xxxxxxxxxxxxxx>
> ---
>   sound/soc/qcom/lpass-apq8016.c   |  61 ++++++++++++
>   sound/soc/qcom/lpass-cpu.c       | 114 +++++++++++++---------
>   sound/soc/qcom/lpass-lpaif-reg.h | 203 ++++++++++++++++++++++++---------------
>   sound/soc/qcom/lpass-platform.c  |  86 +++++++++++------
>   sound/soc/qcom/lpass.h           |  30 ++++++
>   5 files changed, 340 insertions(+), 154 deletions(-)

Thanks for moving this to regmap fields, looks clean!
However this patch just removed support to lpass-ipq806x.c variant, 
which should to be taken care of while doing patches that apply to all 
variants.


> diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c
> index 8210e37..3149645 100644
> --- a/sound/soc/qcom/lpass-apq8016.c
> +++ b/sound/soc/qcom/lpass-apq8016.c
> @@ -124,6 +124,32 @@
>   	},
>   };
>   
> +static int apq8016_init_dmactl_bitfields(struct lpaif_dmactl *dmactl,
> +					 struct regmap *map,
> +					 unsigned int reg)
> +{
> +	struct reg_field bursten = DMACTL_BURSTEN_FLD(reg);
> +	struct reg_field wpscnt = DMACTL_WPSCNT_FLD(reg);
> +	struct reg_field fifowm = DMACTL_FIFOWM_FLD(reg);
> +	struct reg_field intf = DMACTL_AUDINTF_FLD(reg);
> +	struct reg_field enable = DMACTL_ENABLE_FLD(reg);
> +	struct reg_field dyncclk = DMACTL_DYNCLK_FLD(reg);
> +
> +	dmactl->bursten = regmap_field_alloc(map, bursten);
> +	dmactl->wpscnt = regmap_field_alloc(map, wpscnt);
> +	dmactl->fifowm = regmap_field_alloc(map, fifowm);
> +	dmactl->intf = regmap_field_alloc(map, intf);
> +	dmactl->enable = regmap_field_alloc(map, enable);
> +	dmactl->dyncclk = regmap_field_alloc(map, dyncclk);

My idea was to move this all regmap fields to variant structure and 
common code will do the regmap_filed_alloc rather than each variant 
duplicating the same code for each variant, also am guessing some of the 
members in the lpass_variant structure tp become redundant due to regmap 
field which can be removed as well.

ex :

struct lpass_variant {
	...
	struct reg_field bursten
	...
};

in lpass-apq8016.c

we do
static struct lpass_variant apq8016_data = {

	.bursten = REG_FIELD(reg, 11, 11),
	...
}

in lpass-cpu.c we can do the real regmap_field_alloc	
asoc_qcom_lpass_cpu_platform_probe



> +
> +	if (IS_ERR(dmactl->bursten) || IS_ERR(dmactl->wpscnt) ||
> +	    IS_ERR(dmactl->fifowm) || IS_ERR(dmactl->intf) ||
> +	    IS_ERR(dmactl->enable) || IS_ERR(dmactl->dyncclk))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static int apq8016_lpass_alloc_dma_channel(struct lpass_data *drvdata,
>   					   int direction)
>   {
> @@ -158,6 +184,39 @@ static int apq8016_lpass_free_dma_channel(struct lpass_data *drvdata, int chan)
>   	return 0;
>   }
>   
> +static int sc7180_init_i2sctl_bitfields(struct lpaif_i2sctl *i2sctl,
> +					struct regmap *map, unsigned int reg)
> +{
Should this be apq8016_init_i2sctl_bitfields

Please make sure that you compile the code before sending it out!

--srini

>