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