| Message ID | 20171214173402.19074-15-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > uThis patch adds support to DB820c machine driver. Drop 'u' and expand the message to claim that this is the machine driver for 8996, used by the db820c. [..] > +static struct snd_soc_dai_link msm8996_dai_links[] = { Are there any differences between the DAI links of apq8096 and msm8996? > + /* FrontEnd DAI Links */ > + { > + .name = "MultiMedia1 Playback", > + .stream_name = "MultiMedia1", > + .cpu_dai_name = "MM_DL1", > + .platform_name = "q6asm_dai", > + .dynamic = 1, > + .dpcm_playback = 1, > + > + .codec_dai_name = "snd-soc-dummy-dai", > + .codec_name = "snd-soc-dummy", > + }, > + /* Backend DAI Links */ > + { > + .name = "HDMI Playback", > + .stream_name = "q6afe_dai", > + .cpu_dai_name = "HDMI", > + .platform_name = "q6routing", > + .no_pcm = 1, > + .dpcm_playback = 1, > + .be_hw_params_fixup = msm8996_be_hw_params_fixup, > + .codec_dai_name = "i2s-hifi", > + .codec_name = "hdmi-audio-codec.0.auto", > + }, > +}; > + > +static int apq8096_sbc_parse_of(struct snd_soc_card *card) msm8996_parse_of() > +{ > + struct device *dev = card->dev; > + int ret; > + > + ret = snd_soc_of_parse_card_name(card, "qcom,model"); > + if (ret) > + dev_err(dev, "Error parsing card name: %d\n", ret); > + > + return ret; > +} > + > +static int msm_snd_apq8096_probe(struct platform_device *pdev) msm_snd_msm8996_probe()? > +{ > + int ret; > + struct snd_soc_card *card; > + > + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); > + if (!card) > + return -ENOMEM; > + > + card->dev = &pdev->dev; > + > + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > + if (ret) > + return ret; > + > + card->dai_link = msm8996_dai_links; > + card->num_links = ARRAY_SIZE(msm8996_dai_links); > + > + ret = apq8096_sbc_parse_of(card); > + if (ret) { > + dev_err(&pdev->dev, "Error parsing OF data\n"); No need to print in both parse_of() and here. > + return ret; > + } > + > + ret = devm_snd_soc_register_card(&pdev->dev, card); > + if (ret) > + dev_err(&pdev->dev, "sound card register failed (%d)!\n", ret); > + else > + dev_err(&pdev->dev, "sound card register Sucessfull\n"); This isn't an error, skip the print here. > + > + return ret; > +} > + > +static const struct of_device_id msm_snd_apq8096_dt_match[] = { > + {.compatible = "qcom,apq8096-sndcard"}, > + {} > +}; > + > +static struct platform_driver msm_snd_apq8096_driver = { > + .probe = msm_snd_apq8096_probe, > + .driver = { > + .name = "msm-snd-apq8096", > + .owner = THIS_MODULE, Drop the .owner Regards, Bjorn
Thanks for the review comments. On 03/01/18 00:16, Bjorn Andersson wrote: > On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla@linaro.org wrote: > >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> uThis patch adds support to DB820c machine driver. > > Drop 'u' and expand the message to claim that this is the machine driver > for 8996, used by the db820c. > > [..] >> +static struct snd_soc_dai_link msm8996_dai_links[] = { > > Are there any differences between the DAI links of apq8096 and msm8996? > This driver is more of board specific, Am not 100% sure about msm8996, on apq8096 in particularly on db820c we got hdmi and analog audio connected via slimbus and also I2S on lowspeed connector. >> + /* FrontEnd DAI Links */ >> + { >> + .name = "MultiMedia1 Playback", >> + .stream_name = "MultiMedia1", >> + .cpu_dai_name = "MM_DL1", >> + .platform_name = "q6asm_dai", >> + .dynamic = 1, >> + .dpcm_playback = 1, >> + >> + .codec_dai_name = "snd-soc-dummy-dai", >> + .codec_name = "snd-soc-dummy", >> + }, >> + /* Backend DAI Links */ >> + { >> + .name = "HDMI Playback", >> + .stream_name = "q6afe_dai", >> + .cpu_dai_name = "HDMI", >> + .platform_name = "q6routing", >> + .no_pcm = 1, >> + .dpcm_playback = 1, >> + .be_hw_params_fixup = msm8996_be_hw_params_fixup, >> + .codec_dai_name = "i2s-hifi", >> + .codec_name = "hdmi-audio-codec.0.auto", >> + }, >> +}; >> + >> +static int apq8096_sbc_parse_of(struct snd_soc_card *card) > > msm8996_parse_of() sure if it helps. > >> +{ >> + struct device *dev = card->dev; >> + int ret; >> + >> + ret = snd_soc_of_parse_card_name(card, "qcom,model"); >> + if (ret) >> + dev_err(dev, "Error parsing card name: %d\n", ret); >> + >> + return ret; >> +} >> + >> +static int msm_snd_apq8096_probe(struct platform_device *pdev) > > msm_snd_msm8996_probe()? sure > >> +{ >> + int ret; >> + struct snd_soc_card *card; >> + >> + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); >> + if (!card) >> + return -ENOMEM; >> + >> + card->dev = &pdev->dev; >> + >> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); >> + if (ret) >> + return ret; >> + >> + card->dai_link = msm8996_dai_links; >> + card->num_links = ARRAY_SIZE(msm8996_dai_links); >> + >> + ret = apq8096_sbc_parse_of(card); >> + if (ret) { >> + dev_err(&pdev->dev, "Error parsing OF data\n"); > > No need to print in both parse_of() and here. > yep. >> + return ret; >> + } >> + >> + ret = devm_snd_soc_register_card(&pdev->dev, card); >> + if (ret) >> + dev_err(&pdev->dev, "sound card register failed (%d)!\n", ret); >> + else >> + dev_err(&pdev->dev, "sound card register Sucessfull\n"); > > This isn't an error, skip the print here. > yep. >> + >> + return ret; >> +} >> + >> +static const struct of_device_id msm_snd_apq8096_dt_match[] = { >> + {.compatible = "qcom,apq8096-sndcard"}, >> + {} >> +}; >> + >> +static struct platform_driver msm_snd_apq8096_driver = { >> + .probe = msm_snd_apq8096_probe, >> + .driver = { >> + .name = "msm-snd-apq8096", >> + .owner = THIS_MODULE, > > Drop the .owner > yep > Regards, > Bjorn >
On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > uThis patch adds support to DB820c machine driver. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/Kconfig | 9 +++- > sound/soc/qcom/apq8096.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 132 insertions(+), 1 deletion(-) > create mode 100644 sound/soc/qcom/apq8096.c > > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig > index ecd1e4ba834d..748ccc3edefa 100644 > --- a/sound/soc/qcom/Kconfig > +++ b/sound/soc/qcom/Kconfig > @@ -72,7 +72,6 @@ config SND_SOC_QDSP6_ASM_DAI > tristate > default n > > - > config SND_SOC_QDSP6 > tristate "SoC ALSA audio driver for QDSP6" > select SND_SOC_QDSP6_AFE > @@ -87,3 +86,11 @@ config SND_SOC_QDSP6 > This will enable sound soc platform specific > audio drivers. This includes q6asm, q6adm, > q6afe interfaces to DSP using apr. > + > +config SND_SOC_MSM8996 > + tristate "SoC Machine driver for MSM8996 and APQ8096 boards" > + select SND_SOC_QDSP6V2 > + default n > + help > + To add support for SoC audio on MSM8996 and APQ8096 boards > + > diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c > new file mode 100644 > index 000000000000..5b2036317f71 > --- /dev/null > +++ b/sound/soc/qcom/apq8096.c > @@ -0,0 +1,124 @@ > +/* > + * Copyright (c) 2017 The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/clk.h> No clk usage though? > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <sound/soc.h> [...] > +static struct snd_soc_dai_link msm8996_dai_links[] = { > + /* FrontEnd DAI Links */ > + { > + .name = "MultiMedia1 Playback", > + .stream_name = "MultiMedia1", > + .cpu_dai_name = "MM_DL1", > + .platform_name = "q6asm_dai", > + .dynamic = 1, > + .dpcm_playback = 1, > + > + .codec_dai_name = "snd-soc-dummy-dai", > + .codec_name = "snd-soc-dummy", > + }, > + /* Backend DAI Links */ > + { > + .name = "HDMI Playback", > + .stream_name = "q6afe_dai", > + .cpu_dai_name = "HDMI", > + .platform_name = "q6routing", > + .no_pcm = 1, > + .dpcm_playback = 1, > + .be_hw_params_fixup = msm8996_be_hw_params_fixup, > + .codec_dai_name = "i2s-hifi", > + .codec_name = "hdmi-audio-codec.0.auto", > + }, > +}; > + > +static int apq8096_sbc_parse_of(struct snd_soc_card *card) > +{ > + struct device *dev = card->dev; > + int ret; > + > + ret = snd_soc_of_parse_card_name(card, "qcom,model"); > + if (ret) > + dev_err(dev, "Error parsing card name: %d\n", ret); So this prints an error, and the caller also prints an error when it fails. Double error messages? > + > + return ret; > +} > + > +static int msm_snd_apq8096_probe(struct platform_device *pdev) > +{ > + int ret; > + struct snd_soc_card *card; > + > + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); > + if (!card) > + return -ENOMEM; > + > + card->dev = &pdev->dev; > + > + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); Why do we need to do this? Can you add some sort of comment in the code about why?
Thanks for your review comments, On 03/01/18 17:20, Stephen Boyd wrote: > On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c >> new file mode 100644 >> index 000000000000..5b2036317f71 >> --- /dev/null >> +++ b/sound/soc/qcom/apq8096.c >> @@ -0,0 +1,124 @@ >> + */ >> +#include <linux/clk.h> > > No clk usage though? Will remove it in next version. > >> +#include <linux/platform_device.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <sound/soc.h> > [...] >> +static int apq8096_sbc_parse_of(struct snd_soc_card *card) >> +{ >> + struct device *dev = card->dev; >> + int ret; >> + >> + ret = snd_soc_of_parse_card_name(card, "qcom,model"); >> + if (ret) >> + dev_err(dev, "Error parsing card name: %d\n", ret); > > So this prints an error, and the caller also prints an error when it > fails. Double error messages? > looks redundant, will remove it. >> + >> + return ret; >> +} >> + >> +static int msm_snd_apq8096_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + struct snd_soc_card *card; >> + >> + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); >> + if (!card) >> + return -ENOMEM; >> + >> + card->dev = &pdev->dev; >> + >> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > Why do we need to do this? Can you add some sort of comment in the code > about why? Even though dsp supports 64 bit addresses, but the sid sits at offset of 32, which brings this restriction of supporting only 32 bit iova. thanks, srini >
On 01/03, Srinivas Kandagatla wrote: > Thanks for your review comments, > > On 03/01/18 17:20, Stephen Boyd wrote: > >>+ > >>+ return ret; > >>+} > >>+ > >>+static int msm_snd_apq8096_probe(struct platform_device *pdev) > >>+{ > >>+ int ret; > >>+ struct snd_soc_card *card; > >>+ > >>+ card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); > >>+ if (!card) > >>+ return -ENOMEM; > >>+ > >>+ card->dev = &pdev->dev; > >>+ > >>+ ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > > >Why do we need to do this? Can you add some sort of comment in the code > >about why? > > Even though dsp supports 64 bit addresses, but the sid sits at > offset of 32, which brings this restriction of supporting only 32 > bit iova. > Doesn't the dsp have an iommu in place to make the address translation from 64 to 32 bits transparent? I thought this was what dma-ranges and iommu binding was for, but I'm not well versed on all the details here.
On Wed 03 Jan 08:27 PST 2018, Srinivas Kandagatla wrote: > Thanks for the review comments. > > > On 03/01/18 00:16, Bjorn Andersson wrote: > > On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla@linaro.org wrote: > > > > > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > > > > uThis patch adds support to DB820c machine driver. > > > > Drop 'u' and expand the message to claim that this is the machine driver > > for 8996, used by the db820c. > > > > [..] > > > +static struct snd_soc_dai_link msm8996_dai_links[] = { > > > > Are there any differences between the DAI links of apq8096 and msm8996? > > > This driver is more of board specific, > Am not 100% sure about msm8996, on apq8096 in particularly on db820c we got > hdmi and analog audio connected via slimbus and also I2S on lowspeed > connector. > Can this be kept platform specific and the board specific routing put in DT? I don't think we want to have a machine-driver for each board. Right now you have 25 dtbs in mainline, that's going to be a lot of machine drivers... Regards, Bjorn
On 03/01/18 19:41, Stephen Boyd wrote: >>>> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); >>> Why do we need to do this? Can you add some sort of comment in the code >>> about why? >> Even though dsp supports 64 bit addresses, but the sid sits at >> offset of 32, which brings this restriction of supporting only 32 >> bit iova. >> > Doesn't the dsp have an iommu in place to make the address > translation from 64 to 32 bits transparent? I thought this was > what dma-ranges and iommu binding was for, but I'm not well > versed on all the details here. Thanks for reminding, dma-ranges would work too, I will give that a go in next version. --srini
On Wed, Jan 03, 2018 at 09:20:45AM -0800, Stephen Boyd wrote: > On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: > > uThis patch adds support to DB820c machine driver. > > + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > Why do we need to do this? Can you add some sort of comment in the code > about why? And why are we applying DMA restrictions in a machine driver?
On 04/01/18 12:02, Mark Brown wrote: > On Wed, Jan 03, 2018 at 09:20:45AM -0800, Stephen Boyd wrote: >> On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: > >>> uThis patch adds support to DB820c machine driver. > >>> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > >> Why do we need to do this? Can you add some sort of comment in the code >> about why? > > And why are we applying DMA restrictions in a machine driver? Initially I had this in pcm driver, but looking at example usage of snd_dma_alloc_pages, most of them use card->dev and some of them use pcm device for allocating dma memory. Also, as I moved most dsp static services and dais out of DT, except codec and sound card, sound card device was the only choice I had for binding with iommu and enforcing iova range restrictions. This call will be replaced by dma-ranges property in DT either way. --srini >
On Thu, Jan 04, 2018 at 01:44:30PM +0000, Srinivas Kandagatla wrote: > Initially I had this in pcm driver, but looking at example usage of > snd_dma_alloc_pages, most of them use card->dev and some of them use pcm > device for allocating dma memory. If they're using card->dev for DMA they're messing up, they need to use the device associated with the DMA controller.
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig index ecd1e4ba834d..748ccc3edefa 100644 --- a/sound/soc/qcom/Kconfig +++ b/sound/soc/qcom/Kconfig @@ -72,7 +72,6 @@ config SND_SOC_QDSP6_ASM_DAI tristate default n - config SND_SOC_QDSP6 tristate "SoC ALSA audio driver for QDSP6" select SND_SOC_QDSP6_AFE @@ -87,3 +86,11 @@ config SND_SOC_QDSP6 This will enable sound soc platform specific audio drivers. This includes q6asm, q6adm, q6afe interfaces to DSP using apr. + +config SND_SOC_MSM8996 + tristate "SoC Machine driver for MSM8996 and APQ8096 boards" + select SND_SOC_QDSP6V2 + default n + help + To add support for SoC audio on MSM8996 and APQ8096 boards + diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c new file mode 100644 index 000000000000..5b2036317f71 --- /dev/null +++ b/sound/soc/qcom/apq8096.c @@ -0,0 +1,124 @@ +/* + * Copyright (c) 2017 The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#include <linux/clk.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <linux/dma-mapping.h> +#include <sound/pcm.h> + +int msm8996_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + struct snd_interval *rate = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_RATE); + struct snd_interval *channels = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_CHANNELS); + + rate->min = rate->max = 48000; + channels->min = channels->max = 2; + + return 0; +} +static struct snd_soc_dai_link msm8996_dai_links[] = { + /* FrontEnd DAI Links */ + { + .name = "MultiMedia1 Playback", + .stream_name = "MultiMedia1", + .cpu_dai_name = "MM_DL1", + .platform_name = "q6asm_dai", + .dynamic = 1, + .dpcm_playback = 1, + + .codec_dai_name = "snd-soc-dummy-dai", + .codec_name = "snd-soc-dummy", + }, + /* Backend DAI Links */ + { + .name = "HDMI Playback", + .stream_name = "q6afe_dai", + .cpu_dai_name = "HDMI", + .platform_name = "q6routing", + .no_pcm = 1, + .dpcm_playback = 1, + .be_hw_params_fixup = msm8996_be_hw_params_fixup, + .codec_dai_name = "i2s-hifi", + .codec_name = "hdmi-audio-codec.0.auto", + }, +}; + +static int apq8096_sbc_parse_of(struct snd_soc_card *card) +{ + struct device *dev = card->dev; + int ret; + + ret = snd_soc_of_parse_card_name(card, "qcom,model"); + if (ret) + dev_err(dev, "Error parsing card name: %d\n", ret); + + return ret; +} + +static int msm_snd_apq8096_probe(struct platform_device *pdev) +{ + int ret; + struct snd_soc_card *card; + + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); + if (!card) + return -ENOMEM; + + card->dev = &pdev->dev; + + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); + if (ret) + return ret; + + card->dai_link = msm8996_dai_links; + card->num_links = ARRAY_SIZE(msm8996_dai_links); + + ret = apq8096_sbc_parse_of(card); + if (ret) { + dev_err(&pdev->dev, "Error parsing OF data\n"); + return ret; + } + + ret = devm_snd_soc_register_card(&pdev->dev, card); + if (ret) + dev_err(&pdev->dev, "sound card register failed (%d)!\n", ret); + else + dev_err(&pdev->dev, "sound card register Sucessfull\n"); + + return ret; +} + +static const struct of_device_id msm_snd_apq8096_dt_match[] = { + {.compatible = "qcom,apq8096-sndcard"}, + {} +}; + +static struct platform_driver msm_snd_apq8096_driver = { + .probe = msm_snd_apq8096_probe, + .driver = { + .name = "msm-snd-apq8096", + .owner = THIS_MODULE, + .of_match_table = msm_snd_apq8096_dt_match, + }, +}; +module_platform_driver(msm_snd_apq8096_driver); +MODULE_AUTHOR("Srinivas Kandagatla <srinivas.kandagatla@linaro.org"); +MODULE_DESCRIPTION("APQ8096 ASoC Machine Driver"); +MODULE_LICENSE("GPL v2");