| Message ID | 1595413915-17867-1-git-send-email-rohitkr@codeaurora.org |
|---|---|
| Headers | show |
| Series | ASoC: qcom: Add support for SC7180 lpass variant | expand |
On 22/07/2020 11:31, Rohit kumar wrote: > lpass_pcm_data is never freed. Free it in close > ops to avoid memory leak. > > Fixes: 022d00ee0b55 ("ASoC: lpass-platform: Fix broken pcm data usage") > Signed-off-by: Rohit kumar <rohitkr@codeaurora.org> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/lpass-platform.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c > index fb9acfd..b195f3d 100644 > --- a/sound/soc/qcom/lpass-platform.c > +++ b/sound/soc/qcom/lpass-platform.c > @@ -61,7 +61,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component, > int ret, dma_ch, dir = substream->stream; > struct lpass_pcm_data *data; > > - data = devm_kzalloc(soc_runtime->dev, sizeof(*data), GFP_KERNEL); > + data = kzalloc(sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > @@ -118,6 +118,7 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component, > if (v->free_dma_channel) > v->free_dma_channel(drvdata, data->dma_ch); > > + kfree(data); > return 0; > } > >
On 22/07/2020 11:31, Rohit kumar wrote: > i2sctl register value is set to 0 during hw_free(). This > impacts any ongoing concurrent session on the same i2s > port. As trigger() stop already resets enable bit to 0, > there is no need of explicit hw_free. Removing it to > fix the issue. > > Fixes: 80beab8e1d86 ("ASoC: qcom: Add LPASS CPU DAI driver") > Signed-off-by: Rohit kumar <rohitkr@codeaurora.org> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/lpass-cpu.c | 16 ---------------- > 1 file changed, 16 deletions(-) > > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c > index 6b86f16..5d84f63 100644 > --- a/sound/soc/qcom/lpass-cpu.c > +++ b/sound/soc/qcom/lpass-cpu.c > @@ -266,21 +266,6 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, > return 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; > -} > - > static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > @@ -350,7 +335,6 @@ const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = { > .startup = lpass_cpu_daiops_startup, > .shutdown = lpass_cpu_daiops_shutdown, > .hw_params = lpass_cpu_daiops_hw_params, > - .hw_free = lpass_cpu_daiops_hw_free, > .prepare = lpass_cpu_daiops_prepare, > .trigger = lpass_cpu_daiops_trigger, > }; >
On 22/07/2020 11:31, Rohit kumar wrote: > platform_get_resource_byname() is used when there > is list of reg entries. As lpass-cpu node has only > one reg entry, use platform_get_resource() instead. > > Signed-off-by: Rohit kumar <rohitkr@codeaurora.org> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/lpass-cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c > index 5d84f63..1ee6d8b 100644 > --- a/sound/soc/qcom/lpass-cpu.c > +++ b/sound/soc/qcom/lpass-cpu.c > @@ -575,7 +575,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) > > of_lpass_cpu_parse_dai_data(dev, drvdata); > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-lpaif"); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > drvdata->lpaif = devm_ioremap_resource(dev, res); > if (IS_ERR((void const __force *)drvdata->lpaif)) { >
On 22/07/2020 11:31, Rohit kumar wrote: > This patch chain add audio support for SC7180 soc by doing the required > modification in existing common lpass-cpu/lpass-platform driver. > This also fixes some concurrency issue. > > Changes since v3: > - Fixed yaml documentation comments and make dt_binding_check issues. > - Moved general fixes out of sc7180 specific patches as suggested by Srinivas. > - Update clock-names to make it same as existing platforms. > > Ajit Pandey (4): > ASoC: qcom: Add common array to initialize soc based core clocks > ASoC: qcom: lpass-platform: Replace card->dev with component->dev > include: dt-bindings: sound: Add sc7180-lpass bindings header > ASoC: qcom: lpass-sc7180: Add platform driver for lpass audio > > Rohit kumar (8): > ASoC: qcom: lpass-cpu: Move ahbix clk to platform specific function > ASoC: qcom: lpass-platform: fix memory leak > ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers > ASoC: qcom: lpass-cpu: fix concurrency issue > dt-bindings: sound: lpass-cpu: Add sc7180 lpass cpu node > ASoC: qcom: lpass-cpu: Use platform_get_resource > ASoC: qcom: lpass-platform: Use platform_get_irq > dt-bindings: sound: lpass-cpu: Move to yaml format Tested this on Dragon Board 410c! Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --srini > > .../devicetree/bindings/sound/qcom,lpass-cpu.txt | 79 -------- > .../devicetree/bindings/sound/qcom,lpass-cpu.yaml | 185 ++++++++++++++++++ > include/dt-bindings/sound/sc7180-lpass.h | 10 + > sound/soc/qcom/Kconfig | 5 + > sound/soc/qcom/Makefile | 2 + > sound/soc/qcom/lpass-apq8016.c | 86 ++++++-- > sound/soc/qcom/lpass-cpu.c | 204 ++++++++++--------- > sound/soc/qcom/lpass-ipq806x.c | 67 +++++++ > sound/soc/qcom/lpass-lpaif-reg.h | 157 ++++++++------- > sound/soc/qcom/lpass-platform.c | 155 +++++++++++---- > sound/soc/qcom/lpass-sc7180.c | 216 +++++++++++++++++++++ > sound/soc/qcom/lpass.h | 63 +++++- > 12 files changed, 930 insertions(+), 299 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt > create mode 100644 Documentation/devicetree/bindings/sound/qcom,lpass-cpu.yaml > create mode 100644 include/dt-bindings/sound/sc7180-lpass.h > create mode 100644 sound/soc/qcom/lpass-sc7180.c >
On 7/24/2020 4:52 PM, Srinivas Kandagatla wrote: > > > On 22/07/2020 11:31, Rohit kumar wrote: >> This patch chain add audio support for SC7180 soc by doing the required >> modification in existing common lpass-cpu/lpass-platform driver. >> This also fixes some concurrency issue. >> >> Changes since v3: >> - Fixed yaml documentation comments and make dt_binding_check >> issues. >> - Moved general fixes out of sc7180 specific patches as suggested >> by Srinivas. >> - Update clock-names to make it same as existing platforms. >> >> Ajit Pandey (4): >> ASoC: qcom: Add common array to initialize soc based core clocks >> ASoC: qcom: lpass-platform: Replace card->dev with component->dev >> include: dt-bindings: sound: Add sc7180-lpass bindings header >> ASoC: qcom: lpass-sc7180: Add platform driver for lpass audio >> >> Rohit kumar (8): >> ASoC: qcom: lpass-cpu: Move ahbix clk to platform specific function >> ASoC: qcom: lpass-platform: fix memory leak >> ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers >> ASoC: qcom: lpass-cpu: fix concurrency issue >> dt-bindings: sound: lpass-cpu: Add sc7180 lpass cpu node >> ASoC: qcom: lpass-cpu: Use platform_get_resource >> ASoC: qcom: lpass-platform: Use platform_get_irq >> dt-bindings: sound: lpass-cpu: Move to yaml format > > Thanks Srini for review and testing. Mark, I am planning to repost patch07 onwards to address comments by Rob as there are no comments till patch06 and they are just fixes. > Tested this on Dragon Board 410c! > > Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > --srini >> >> .../devicetree/bindings/sound/qcom,lpass-cpu.txt | 79 -------- >> .../devicetree/bindings/sound/qcom,lpass-cpu.yaml | 185 >> ++++++++++++++++++ >> include/dt-bindings/sound/sc7180-lpass.h | 10 + >> sound/soc/qcom/Kconfig | 5 + >> sound/soc/qcom/Makefile | 2 + >> sound/soc/qcom/lpass-apq8016.c | 86 ++++++-- >> sound/soc/qcom/lpass-cpu.c | 204 >> ++++++++++--------- >> sound/soc/qcom/lpass-ipq806x.c | 67 +++++++ >> sound/soc/qcom/lpass-lpaif-reg.h | 157 >> ++++++++------- >> sound/soc/qcom/lpass-platform.c | 155 >> +++++++++++---- >> sound/soc/qcom/lpass-sc7180.c | 216 >> +++++++++++++++++++++ >> sound/soc/qcom/lpass.h | 63 +++++- >> 12 files changed, 930 insertions(+), 299 deletions(-) >> delete mode 100644 >> Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt >> create mode 100644 >> Documentation/devicetree/bindings/sound/qcom,lpass-cpu.yaml >> create mode 100644 include/dt-bindings/sound/sc7180-lpass.h >> create mode 100644 sound/soc/qcom/lpass-sc7180.c >>
On 7/27/2020 3:20 PM, Rohit Kumar wrote: > > On 7/24/2020 4:52 PM, Srinivas Kandagatla wrote: >> >> >> On 22/07/2020 11:31, Rohit kumar wrote: >>> This patch chain add audio support for SC7180 soc by doing the required >>> modification in existing common lpass-cpu/lpass-platform driver. >>> This also fixes some concurrency issue. >>> >>> Changes since v3: >>> - Fixed yaml documentation comments and make dt_binding_check >>> issues. >>> - Moved general fixes out of sc7180 specific patches as >>> suggested by Srinivas. >>> - Update clock-names to make it same as existing platforms. >>> >>> Ajit Pandey (4): >>> ASoC: qcom: Add common array to initialize soc based core clocks >>> ASoC: qcom: lpass-platform: Replace card->dev with component->dev >>> include: dt-bindings: sound: Add sc7180-lpass bindings header >>> ASoC: qcom: lpass-sc7180: Add platform driver for lpass audio >>> >>> Rohit kumar (8): >>> ASoC: qcom: lpass-cpu: Move ahbix clk to platform specific function >>> ASoC: qcom: lpass-platform: fix memory leak >>> ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers >>> ASoC: qcom: lpass-cpu: fix concurrency issue >>> dt-bindings: sound: lpass-cpu: Add sc7180 lpass cpu node >>> ASoC: qcom: lpass-cpu: Use platform_get_resource >>> ASoC: qcom: lpass-platform: Use platform_get_irq >>> dt-bindings: sound: lpass-cpu: Move to yaml format >> >> > Thanks Srini for review and testing. > > Mark, I am planning to repost patch07 onwards to address comments by > Rob as > > there are no comments till patch06 and they are just fixes. Hello Mark, Are you planning to review/merge the changes till patch06. If not, I will resend those patches along with comments addressed on patch07 onwards. Thanks, Rohit > >> Tested this on Dragon Board 410c! >> >> Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> --srini >>> >>> .../devicetree/bindings/sound/qcom,lpass-cpu.txt | 79 -------- >>> .../devicetree/bindings/sound/qcom,lpass-cpu.yaml | 185 >>> ++++++++++++++++++ >>> include/dt-bindings/sound/sc7180-lpass.h | 10 + >>> sound/soc/qcom/Kconfig | 5 + >>> sound/soc/qcom/Makefile | 2 + >>> sound/soc/qcom/lpass-apq8016.c | 86 ++++++-- >>> sound/soc/qcom/lpass-cpu.c | 204 >>> ++++++++++--------- >>> sound/soc/qcom/lpass-ipq806x.c | 67 +++++++ >>> sound/soc/qcom/lpass-lpaif-reg.h | 157 >>> ++++++++------- >>> sound/soc/qcom/lpass-platform.c | 155 >>> +++++++++++---- >>> sound/soc/qcom/lpass-sc7180.c | 216 >>> +++++++++++++++++++++ >>> sound/soc/qcom/lpass.h | 63 +++++- >>> 12 files changed, 930 insertions(+), 299 deletions(-) >>> delete mode 100644 >>> Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt >>> create mode 100644 >>> Documentation/devicetree/bindings/sound/qcom,lpass-cpu.yaml >>> create mode 100644 include/dt-bindings/sound/sc7180-lpass.h >>> create mode 100644 sound/soc/qcom/lpass-sc7180.c >>>
Quoting Rohit kumar (2020-07-22 03:31:44) > From: Ajit Pandey <ajitp@codeaurora.org> > > LPASS variants have their own soc specific clocks that needs to be > enabled for MI2S audio support. Added a common variable in drvdata to > initialize such clocks using bulk clk api. Such clock names is > defined in variants specific data and needs to fetched during init. Why not just get all the clks and not even care about the names of them? Use devm_clk_bulk_get_all() for that, unless some clks need to change rates?
Thanks Stephen for reviewing. On 8/6/2020 6:01 AM, Stephen Boyd wrote: > Quoting Rohit kumar (2020-07-22 03:31:44) >> From: Ajit Pandey <ajitp@codeaurora.org> >> >> LPASS variants have their own soc specific clocks that needs to be >> enabled for MI2S audio support. Added a common variable in drvdata to >> initialize such clocks using bulk clk api. Such clock names is >> defined in variants specific data and needs to fetched during init. > Why not just get all the clks and not even care about the names of them? > Use devm_clk_bulk_get_all() for that, unless some clks need to change > rates? There is ahbix clk which needs clk rate to be set. Please check below patch in the series for reference [PATCH v5 02/12] ASoC: qcom: lpass-cpu: Move ahbix clk to platform specific function Thanks, Rohit
Quoting Rohit Kumar (2020-08-05 20:59:48) > Thanks Stephen for reviewing. > > On 8/6/2020 6:01 AM, Stephen Boyd wrote: > > Quoting Rohit kumar (2020-07-22 03:31:44) > >> From: Ajit Pandey <ajitp@codeaurora.org> > >> > >> LPASS variants have their own soc specific clocks that needs to be > >> enabled for MI2S audio support. Added a common variable in drvdata to > >> initialize such clocks using bulk clk api. Such clock names is > >> defined in variants specific data and needs to fetched during init. > > Why not just get all the clks and not even care about the names of them? > > Use devm_clk_bulk_get_all() for that, unless some clks need to change > > rates? > > There is ahbix clk which needs clk rate to be set. Please check below > patch in > > the series for reference > > [PATCH v5 02/12] ASoC: qcom: lpass-cpu: Move ahbix clk to platform > specific function > Alright. I wonder if we could make the API better or the binding better and always have the rate settable clk first and then devm_clk_bulk_get_all() could be used along with clk_set_rate() on some array element 0 or something. Anyway, don't mind me, I'm just thinking how to make this simpler.