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

[Rohit Kumar]

Thanks Srini for reviewing the change.


On 5/18/2020 3:19 PM, Srinivas Kandagatla wrote:
> 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.
Right. I will make the change as part of next patchset.
> > 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),
> ÂÂÂÂ...
> }
We can keep reg_field in lpass_variant, but assignment in the struct 
will be a problem as

reg is variable here. So, we need to expose an API in lpass_variant to 
assign reg_field.

regmap_field will still be in dmactl/i2sctl structs as it differs for 
different dma channel/i2s port

respectively. Please share your thoughts.

> in lpass-cpu.c we can do the real regmap_field_alloc
> asoc_qcom_lpass_cpu_platform_probe
Yes, I will move regmap_field_alloc to lpass_cpu.c in next patchset.
> > +
> > +ÂÂÂ 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!
Will take care in next patchset.

> --srini
>
> >
Thanks,

Rohit

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.