Messages in this thread |  | | Subject | Re: [PATCH v3 3/8] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers | From | Rohit Kumar <> | Date | Thu, 9 Jul 2020 15:44:59 +0530 |
| |
On 7/9/2020 3:36 PM, Srinivas Kandagatla wrote: > > > On 09/07/2020 10:57, Rohit Kumar wrote: >> Thanks Srini for reviewing. >> >> On 7/9/2020 2:56 PM, Srinivas Kandagatla wrote: >>> >>> >>> On 08/07/2020 06:08, Rohit kumar wrote: >>>> I2SCTL and DMACTL registers has different bits alignment for newer >>>> LPASS variants of SC7180 soc. Use REG_FIELD_ID() to define the >>>> reg_fields in platform specific file and removed shifts and mask >>>> macros for such registers from header file. >>>> >>>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org> >>> >>> Thanks Rohit for doing this, this looks much better now! >>> I have few minor comments.. >>> >>>> --- >>>> sound/soc/qcom/lpass-apq8016.c | 24 ++++++ >>>> sound/soc/qcom/lpass-cpu.c | 163 >>>> +++++++++++++++++++++++---------------- >>>> sound/soc/qcom/lpass-ipq806x.c | 24 ++++++ >>>> sound/soc/qcom/lpass-lpaif-reg.h | 157 >>>> +++++++++++++++++++------------------ >>>> sound/soc/qcom/lpass-platform.c | 151 >>>> +++++++++++++++++++++++++++--------- >>>> sound/soc/qcom/lpass.h | 53 +++++++++++++ >>>> 6 files changed, 398 insertions(+), 174 deletions(-) >>>> >>> >>> index f0c7e93..f358d12 100644 >>>> --- a/sound/soc/qcom/lpass-cpu.c >>>> +++ b/sound/soc/qcom/lpass-cpu.c >>>> @@ -29,6 +29,32 @@ >>>> #define LPASS_CPU_I2S_SD0_1_2_MASK GENMASK(2, 0) >>>> #define LPASS_CPU_I2S_SD0_1_2_3_MASK GENMASK(3, 0) >>> >>> >>>> } >>>> -static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream >>>> *substream, >>>> - struct snd_soc_dai *dai) >>>> -{ >>>> - struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai); >>>> - int ret; >>>> - >>>> - ret = regmap_write(drvdata->lpaif_map, >>>> - LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), >>>> - 0); >>>> - if (ret) >>>> - dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); >>>> - >>>> - return ret; >>>> -} >>> >>> Any particular reason why this function remove >> >> This was causing issue in playback/capture concurrency. It sets >> I2SCTL register value to 0 >> >> when usecase ends. However, playback/capture specific bits are >> already cleared during trigger() stop >> >> function. So, this is not needed. > > This should be sent as separate fix with fixes tag! Ok. Will create separate patch with fixes tag and post. > >> >> >>> >>>> diff --git a/sound/soc/qcom/lpass-lpaif-reg.h >>>> b/sound/soc/qcom/lpass-lpaif-reg.h >>>> index 72a3e2f..5258e60 100644 >>>> --- a/sound/soc/qcom/lpass-lpaif-reg.h >>>> +++ b/sound/soc/qcom/lpass-lpaif-reg.h >>>> @@ -12,15 +12,12 @@ >>> ... >>>> #endif /* __LPASS_LPAIF_REG_H__ */ >>>> diff --git a/sound/soc/qcom/lpass-platform.c >>>> b/sound/soc/qcom/lpass-platform.c >>>> index 34f7fd1..445ca193 100644 >>>> --- a/sound/soc/qcom/lpass-platform.c >>>> +++ b/sound/soc/qcom/lpass-platform.c >>>> @@ -50,6 +50,53 @@ static const struct snd_pcm_hardware >>>> lpass_platform_pcm_hardware = { >>>> .fifo_size = 0, >>>> }; >>> ... >>>> static int lpass_platform_pcmops_open(struct snd_soc_component >>>> *component, >>>> struct snd_pcm_substream *substream) >>>> { >>>> @@ -59,9 +106,9 @@ static int lpass_platform_pcmops_open(struct >>>> snd_soc_component *component, >>>> struct lpass_data *drvdata = >>>> snd_soc_component_get_drvdata(component); >>>> struct lpass_variant *v = drvdata->variant; >>>> int ret, dma_ch, dir = substream->stream; >>>> - struct lpass_pcm_data *data; >>>> + struct lpass_pcm_data *data = NULL; >>>> - data = devm_kzalloc(soc_runtime->dev, sizeof(*data), >>>> GFP_KERNEL); >>>> + data = kzalloc(sizeof(*data), GFP_KERNEL); >>> >>> Does this change belong in this patch? >> >> >> As part of this change, I fixed memory leak too by adding kfree() in >> close() >> >> However, this was causing issue as memory was allocated using >> devm_kzalloc(). >> >> Should I move it to different patch? > > That would be the right thing to do, can also add fixes tag!
Sure, Will do that in next spin.
> > >> >>> >>>> if (!data) >>>> return -ENOMEM; >>>> @@ -111,13 +158,13 @@ static int >>>> lpass_platform_pcmops_close(struct snd_soc_component *component, >>>> struct snd_pcm_runtime *runtime = substream->runtime; >>>> struct lpass_data *drvdata = >>>> snd_soc_component_get_drvdata(component); >>>> struct lpass_variant *v = drvdata->variant; > >>> >>> Above two along with rddma members can be removed, these become >>> redundant after adding regmap field! >>> >> wrdma_channels is used in alloc_dma_channel() to get the channel id. >> >> Also, both are used for other DMA registers such as LPAIF_RDMABASE_REG, >> >> LPAIF_RDMABUFF_REG, LPAIF_RDMACURR_REG, etc. >> > Ah I see we are still using this in lpass_cpu_regmap_writeable! > ignore my previous comments about removing them! > > --srini
-- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.
|  |