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@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.
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.
