* Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers
@ 2020-05-18 9:49 Srinivas Kandagatla
0 siblings, 0 replies; 2+ messages in thread
From: Srinivas Kandagatla @ 2020-05-18 9:49 UTC (permalink / raw)
To: Ajit Pandey, broonie, plai, bgoswami; +Cc: alsa-devel, devicetree, linux-kernel
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.
> 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers
@ 2020-06-27 18:02 Rohit Kumar
0 siblings, 0 replies; 2+ messages in thread
From: Rohit Kumar @ 2020-06-27 18:02 UTC (permalink / raw)
To: Srinivas Kandagatla, Ajit Pandey, broonie, plai, bgoswami
Cc: devicetree, alsa-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-27 18:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 9:49 [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers Srinivas Kandagatla
2020-06-27 18:02 Rohit Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).