All of lore.kernel.org
 help / color / mirror / Atom feed
* 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; 15+ messages in thread
From: Srinivas Kandagatla @ 2020-05-18  9:49 UTC (permalink / raw)
  To: Ajit Pandey, broonie, plai, bgoswami; +Cc: devicetree, alsa-devel, 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] 15+ 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; 15+ 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] 15+ messages in thread
* Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers
@ 2020-06-29  4:06 Rohit Kumar
  0 siblings, 0 replies; 15+ messages in thread
From: Rohit Kumar @ 2020-06-29  4:06 UTC (permalink / raw)
  To: Srinivas Kandagatla, Ajit Pandey, broonie, plai, bgoswami
  Cc: devicetree, alsa-devel, linux-kernel

Hello Srini,

On 6/27/2020 11:32 PM, Rohit Kumar wrote:
> 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.

While making changes, I felt like there is no significance of keeping 
reg_field variables inside

lpass_variant struct. Below are the reasons:

* We anyway have to expose an API to fill the regfields as reg is 
variable in REG_FIELD(reg, 11, 11)

* In case of sc7180, RD_DMA and WR_DMA control register has different 
field mask. So, values

different for playback and capture. I was thinking of exposing an API 
which will just fill the reg_field

in the struct passed something like this:

static void apq8016_init_dmactl_regfields(struct lpaif_dmactl_regfields 
*dmactl,
                         unsigned int reg, int dir)
{
         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 = bursten;
         dmactl->wpscnt = wpscnt;
         dmactl->fifowm = fifowm;
         dmactl->intf = intf;
         dmactl->enable = enable;
         dmactl->dyncclk = dyncclk;
}

This will be called by lpass-platform.c where we can do regmap_field_alloc.

So, we will have common function for regmap_field_alloc. Please share 
your inputs.

>
>> 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
>
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] 15+ messages in thread
end of thread, other threads:[~2020-06-29  4:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <“1586592171-31644-1-git-send-email-ajitp@codeaurora.org”>
2020-05-14 16:38 ` [PATCH v2 0/7] ASoC: QCOM: Add support for SC7180 lpass variant Ajit Pandey
2020-05-14 16:38   ` [PATCH v2 1/7] Documentation: device-tree: sound: Update lpass-cpu driver binding Ajit Pandey
2020-05-14 16:44     ` Mark Brown
2020-05-14 16:38   ` [PATCH v2 2/7] ASoC: qcom: Add common array to initialize soc based core clocks Ajit Pandey
2020-05-14 16:38   ` Ajit Pandey
2020-05-14 16:45     ` Mark Brown
2020-05-14 16:38   ` [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers Ajit Pandey
2020-05-14 16:38   ` [PATCH v2 5/7] include: dt-bindings: sound: Add sc7180-lpass bindings header Ajit Pandey
2020-05-14 16:48     ` Mark Brown
2020-05-14 16:38   ` [PATCH v2 6/7] device-tree: bindings: sound: lpass-cpu: Add new compatible soc Ajit Pandey
2020-05-14 16:38   ` [PATCH v2 7/7] ASoC: qcom: lpass-sc7180: Add platform driver for lpass audio Ajit Pandey
2020-05-18  9:49 [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers Srinivas Kandagatla
2020-05-18  9:49 ` Srinivas Kandagatla
  -- strict thread matches above, loose matches on Subject: below --
2020-06-27 18:02 Rohit Kumar
2020-06-29  4:06 Rohit Kumar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.