Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers | From | Rohit Kumar <> | Date | Sat, 27 Jun 2020 23:32:09 +0530 |
| |
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@codeaurora.org> >> --- >> 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.
| |