| Message ID | 20220307122202.2251639-1-codrin.ciubotariu@microchip.com |
|---|---|
| Headers | show |
| Series | Add driver for SAMA7G5's PDMC | expand |
On Mon, 7 Mar 2022 14:21:56 +0200, Codrin Ciubotariu wrote: > This patch series adds support for Pulse Density Microphone Controller > (PDMC), present on Microchip's SAMA7G5. > The PDMC interfaces up to 4 digital microphones having Pulse Density > Modulated (PDM) outputs. It generates a single clock line and samples 1 or > 2 data lines. The signal path includes an audio grade programmable > decimation filter and outputs 24-bit audio words. > The source of each channel can be independently defined as PDMC_DS0 or > PDMC_DS1, sampled at the rising or falling edge of PDMC_CLK. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback commit: 9a1e13440a4f2e7566fd4c5eae6a53e6400e08a4 [2/6] ASoC: dt-bindings: Document Microchip's PDMC commit: 015044e9610c8523794ea6cb55d5388bc00ba96a [3/6] ASoC: atmel: mchp-pdmc: add PDMC driver commit: 50291652af5269813baa6024eb0e81b5f0bbb451 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi, On Mon, Mar 07, 2022 at 02:21:57PM +0200, Codrin Ciubotariu wrote: > Even if struct snd_dmaengine_pcm_config is used, prepare_slave_config() > callback might not be set. Check if this callback is set before using it. > > Fixes: fa654e085300 ("ASoC: dmaengine-pcm: Provide default config") > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > Changes in v2,v3: > - none > > sound/soc/soc-generic-dmaengine-pcm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > index 285441d6aeed..2ab2ddc1294d 100644 > --- a/sound/soc/soc-generic-dmaengine-pcm.c > +++ b/sound/soc/soc-generic-dmaengine-pcm.c > @@ -86,10 +86,10 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, > > memset(&slave_config, 0, sizeof(slave_config)); > > - if (!pcm->config) > - prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; > - else > + if (pcm->config && pcm->config->prepare_slave_config) > prepare_slave_config = pcm->config->prepare_slave_config; > + else > + prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; > > if (prepare_slave_config) { > int ret = prepare_slave_config(substream, params, &slave_config); I wonder if this patch is correct. There are drivers like sound/soc/mxs/mxs-pcm.c which call snd_dmaengine_pcm_register() with a config which has the prepare_slave_config callback unset. For these drivers dmaengine_pcm_hw_params() previously was a no-op. Now with this patch snd_dmaengine_pcm_prepare_slave_config() and dmaengine_slave_config() are called. At least for the mxs-pcm driver calling dmaengine_slave_config() will return -ENOSYS. At least the "Check if this callback is set before using it" part is wrong, the callback is checked before using it with if (prepare_slave_config) { ... } I don't have any mxs hardware at hand to test this. I just stumbled upon the change of behaviour when rebasing https://patchwork.kernel.org/project/alsa-devel/patch/20220301122111.1073174-1-s.hauer@pengutronix.de/ on current master. Sascha
On 20.04.2022 12:15, Sascha Hauer wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, Hi Sascha, > > On Mon, Mar 07, 2022 at 02:21:57PM +0200, Codrin Ciubotariu wrote: >> Even if struct snd_dmaengine_pcm_config is used, prepare_slave_config() >> callback might not be set. Check if this callback is set before using it. >> >> Fixes: fa654e085300 ("ASoC: dmaengine-pcm: Provide default config") >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> >> Changes in v2,v3: >> - none >> >> sound/soc/soc-generic-dmaengine-pcm.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c >> index 285441d6aeed..2ab2ddc1294d 100644 >> --- a/sound/soc/soc-generic-dmaengine-pcm.c >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c >> @@ -86,10 +86,10 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, >> >> memset(&slave_config, 0, sizeof(slave_config)); >> >> - if (!pcm->config) >> - prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; >> - else >> + if (pcm->config && pcm->config->prepare_slave_config) >> prepare_slave_config = pcm->config->prepare_slave_config; >> + else >> + prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; >> >> if (prepare_slave_config) { >> int ret = prepare_slave_config(substream, params, &slave_config); > > I wonder if this patch is correct. There are drivers like > sound/soc/mxs/mxs-pcm.c which call snd_dmaengine_pcm_register() with a > config which has the prepare_slave_config callback unset. For these > drivers dmaengine_pcm_hw_params() previously was a no-op. Now with this > patch snd_dmaengine_pcm_prepare_slave_config() and > dmaengine_slave_config() are called. At least for the mxs-pcm driver > calling dmaengine_slave_config() will return -ENOSYS. > > At least the "Check if this callback is set before using it" part is > wrong, the callback is checked before using it with > > if (prepare_slave_config) { > ... > } > > I don't have any mxs hardware at hand to test this. I just stumbled upon > the change of behaviour when rebasing > https://patchwork.kernel.org/project/alsa-devel/patch/20220301122111.1073174-1-s.hauer@pengutronix.de/ > on current master. You are right. I changed the behavior from: if (pmc->config && !pcm->config->prepare_slave_config) <do nothing> to: if (pmc->config && !pcm->config->prepare_slave_config) snd_dmaengine_pcm_prepare_slave_config() It was not intended and I agree that the commit message is not accurate. I guess some drivers might not need dmaengine_slave_config()... However, in my case, for the mchp-pdmc driver, I do have pcm->config with pcm->config->prepare_slave_config NULL, but I still need snd_dmaengine_pcm_prepare_slave_config() to be called. Should we add a separate flag to call snd_dmaengine_pcm_prepare_slave_config() if pcm->config->prepare_slave_config is NULL? Nice catch! Best regards, Codrin
On Wed, Apr 20, 2022 at 09:58:06AM +0000, Codrin.Ciubotariu@microchip.com wrote: > On 20.04.2022 12:15, Sascha Hauer wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Hi, > > Hi Sascha, > > > > > On Mon, Mar 07, 2022 at 02:21:57PM +0200, Codrin Ciubotariu wrote: > >> Even if struct snd_dmaengine_pcm_config is used, prepare_slave_config() > >> callback might not be set. Check if this callback is set before using it. > >> > >> Fixes: fa654e085300 ("ASoC: dmaengine-pcm: Provide default config") > >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > >> --- > >> > >> Changes in v2,v3: > >> - none > >> > >> sound/soc/soc-generic-dmaengine-pcm.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > >> index 285441d6aeed..2ab2ddc1294d 100644 > >> --- a/sound/soc/soc-generic-dmaengine-pcm.c > >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c > >> @@ -86,10 +86,10 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, > >> > >> memset(&slave_config, 0, sizeof(slave_config)); > >> > >> - if (!pcm->config) > >> - prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; > >> - else > >> + if (pcm->config && pcm->config->prepare_slave_config) > >> prepare_slave_config = pcm->config->prepare_slave_config; > >> + else > >> + prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; > >> > >> if (prepare_slave_config) { > >> int ret = prepare_slave_config(substream, params, &slave_config); > > > > I wonder if this patch is correct. There are drivers like > > sound/soc/mxs/mxs-pcm.c which call snd_dmaengine_pcm_register() with a > > config which has the prepare_slave_config callback unset. For these > > drivers dmaengine_pcm_hw_params() previously was a no-op. Now with this > > patch snd_dmaengine_pcm_prepare_slave_config() and > > dmaengine_slave_config() are called. At least for the mxs-pcm driver > > calling dmaengine_slave_config() will return -ENOSYS. > > > > At least the "Check if this callback is set before using it" part is > > wrong, the callback is checked before using it with > > > > if (prepare_slave_config) { > > ... > > } > > > > I don't have any mxs hardware at hand to test this. I just stumbled upon > > the change of behaviour when rebasing > > https://patchwork.kernel.org/project/alsa-devel/patch/20220301122111.1073174-1-s.hauer@pengutronix.de/ > > on current master. > > You are right. I changed the behavior from: > if (pmc->config && !pcm->config->prepare_slave_config) > <do nothing> > to: > if (pmc->config && !pcm->config->prepare_slave_config) > snd_dmaengine_pcm_prepare_slave_config() > > It was not intended and I agree that the commit message is not accurate. > I guess some drivers might not need dmaengine_slave_config()... > However, in my case, for the mchp-pdmc driver, I do have pcm->config > with pcm->config->prepare_slave_config NULL, but I still need > snd_dmaengine_pcm_prepare_slave_config() to be called. Should we add a > separate flag to call snd_dmaengine_pcm_prepare_slave_config() if > pcm->config->prepare_slave_config is NULL? Other drivers set pcm->config->prepare_slave_config to snd_dmaengine_pcm_prepare_slave_config() explicitly: sound/soc/fsl/imx-pcm-dma.c:33: .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, I think that's the way to go. Regards, Sascha
On 20.04.2022 13:06, Sascha Hauer wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Apr 20, 2022 at 09:58:06AM +0000, Codrin.Ciubotariu@microchip.com wrote: >> On 20.04.2022 12:15, Sascha Hauer wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi, >> >> Hi Sascha, >> >>> >>> On Mon, Mar 07, 2022 at 02:21:57PM +0200, Codrin Ciubotariu wrote: >>>> Even if struct snd_dmaengine_pcm_config is used, prepare_slave_config() >>>> callback might not be set. Check if this callback is set before using it. >>>> >>>> Fixes: fa654e085300 ("ASoC: dmaengine-pcm: Provide default config") >>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >>>> --- >>>> >>>> Changes in v2,v3: >>>> - none >>>> >>>> sound/soc/soc-generic-dmaengine-pcm.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c >>>> index 285441d6aeed..2ab2ddc1294d 100644 >>>> --- a/sound/soc/soc-generic-dmaengine-pcm.c >>>> +++ b/sound/soc/soc-generic-dmaengine-pcm.c >>>> @@ -86,10 +86,10 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, >>>> >>>> memset(&slave_config, 0, sizeof(slave_config)); >>>> >>>> - if (!pcm->config) >>>> - prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; >>>> - else >>>> + if (pcm->config && pcm->config->prepare_slave_config) >>>> prepare_slave_config = pcm->config->prepare_slave_config; >>>> + else >>>> + prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; >>>> >>>> if (prepare_slave_config) { >>>> int ret = prepare_slave_config(substream, params, &slave_config); >>> >>> I wonder if this patch is correct. There are drivers like >>> sound/soc/mxs/mxs-pcm.c which call snd_dmaengine_pcm_register() with a >>> config which has the prepare_slave_config callback unset. For these >>> drivers dmaengine_pcm_hw_params() previously was a no-op. Now with this >>> patch snd_dmaengine_pcm_prepare_slave_config() and >>> dmaengine_slave_config() are called. At least for the mxs-pcm driver >>> calling dmaengine_slave_config() will return -ENOSYS. >>> >>> At least the "Check if this callback is set before using it" part is >>> wrong, the callback is checked before using it with >>> >>> if (prepare_slave_config) { >>> ... >>> } >>> >>> I don't have any mxs hardware at hand to test this. I just stumbled upon >>> the change of behaviour when rebasing >>> https://patchwork.kernel.org/project/alsa-devel/patch/20220301122111.1073174-1-s.hauer@pengutronix.de/ >>> on current master. >> >> You are right. I changed the behavior from: >> if (pmc->config && !pcm->config->prepare_slave_config) >> <do nothing> >> to: >> if (pmc->config && !pcm->config->prepare_slave_config) >> snd_dmaengine_pcm_prepare_slave_config() >> >> It was not intended and I agree that the commit message is not accurate. >> I guess some drivers might not need dmaengine_slave_config()... >> However, in my case, for the mchp-pdmc driver, I do have pcm->config >> with pcm->config->prepare_slave_config NULL, but I still need >> snd_dmaengine_pcm_prepare_slave_config() to be called. Should we add a >> separate flag to call snd_dmaengine_pcm_prepare_slave_config() if >> pcm->config->prepare_slave_config is NULL? > > Other drivers set pcm->config->prepare_slave_config to > snd_dmaengine_pcm_prepare_slave_config() explicitly: > > sound/soc/fsl/imx-pcm-dma.c:33: .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > > I think that's the way to go. That's more elegant, right. I will revert this patch and use your suggestion for the mchp-pdmc driver. Thanks and best regards, Codrin
On 07/03/2022 at 13:22, Codrin Ciubotariu wrote: > Enable drivers needed for Microchip's PDMC and PDM microphones. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > Changes in v2,v3: > - none; > > arch/arm/configs/sama7_defconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/configs/sama7_defconfig b/arch/arm/configs/sama7_defconfig > index 0368068e04d9..bc29badab890 100644 > --- a/arch/arm/configs/sama7_defconfig > +++ b/arch/arm/configs/sama7_defconfig > @@ -138,6 +138,8 @@ CONFIG_SND_SOC_MIKROE_PROTO=m > CONFIG_SND_MCHP_SOC_I2S_MCC=y > CONFIG_SND_MCHP_SOC_SPDIFTX=y > CONFIG_SND_MCHP_SOC_SPDIFRX=y > +CONFIG_SND_MCHP_SOC_PDMC=y > +CONFIG_SND_SOC_DMIC=y I'm fine with that, but I see that some Kconfig entries "select" this SND_SOC_DMIC directly (amd, intel, mediatek, stm). If it's absolutely needed for PDMC to work, what about doing the same as it would prevent some broken configurations? Regards, Nicolas > CONFIG_SND_SOC_PCM5102A=y > CONFIG_SND_SOC_SPDIF=y > CONFIG_SND_SIMPLE_CARD=y
On 05.05.2022 16:58, Nicolas Ferre wrote: > On 07/03/2022 at 13:22, Codrin Ciubotariu wrote: >> Enable drivers needed for Microchip's PDMC and PDM microphones. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> >> Changes in v2,v3: >> - none; >> >> arch/arm/configs/sama7_defconfig | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm/configs/sama7_defconfig >> b/arch/arm/configs/sama7_defconfig >> index 0368068e04d9..bc29badab890 100644 >> --- a/arch/arm/configs/sama7_defconfig >> +++ b/arch/arm/configs/sama7_defconfig >> @@ -138,6 +138,8 @@ CONFIG_SND_SOC_MIKROE_PROTO=m >> CONFIG_SND_MCHP_SOC_I2S_MCC=y >> CONFIG_SND_MCHP_SOC_SPDIFTX=y >> CONFIG_SND_MCHP_SOC_SPDIFRX=y >> +CONFIG_SND_MCHP_SOC_PDMC=y >> +CONFIG_SND_SOC_DMIC=y > > I'm fine with that, but I see that some Kconfig entries "select" this > SND_SOC_DMIC directly (amd, intel, mediatek, stm). > If it's absolutely needed for PDMC to work, what about doing the same as > it would prevent some broken configurations? The only way it makes sense to me to have this driver selected somewhere is in a sound card driver, used for a specific board, which we know it has PDM microphones. Since, for now, we use the simple sound card for our audio interfaces, we have no place to add this select. The reason I do not like to add this select under the controller driver, as some of the vendors did, is because, in the future, we might have different PDM microphones that might not work with SND_SOC_DMIC and might need a different driver. I don't have a strong opinion on this. If you think I am overthinking, please let me know and I will change this. Best regards, Codrin
On Thu, May 05, 2022 at 02:47:04PM +0000, Codrin.Ciubotariu@microchip.com wrote: > On 05.05.2022 16:58, Nicolas Ferre wrote: > > I'm fine with that, but I see that some Kconfig entries "select" this > > SND_SOC_DMIC directly (amd, intel, mediatek, stm). > > If it's absolutely needed for PDMC to work, what about doing the same as > > it would prevent some broken configurations? > The only way it makes sense to me to have this driver selected somewhere > is in a sound card driver, used for a specific board, which we know it > has PDM microphones. Since, for now, we use the simple sound card for > our audio interfaces, we have no place to add this select. > The reason I do not like to add this select under the controller driver, > as some of the vendors did, is because, in the future, we might have > different PDM microphones that might not work with SND_SOC_DMIC and > might need a different driver. > I don't have a strong opinion on this. If you think I am overthinking, > please let me know and I will change this. It's unlikely but possible that there could be some other device connected (eg, you could have a DSP or something that generates PDM output). If the driver doesn't directly instantiate the DMIC itself then it's probably reasonable for it to be user controllable if the DMIC driver is there.
On 05/05/2022 at 17:01, Mark Brown wrote: > On Thu, May 05, 2022 at 02:47:04PM +0000,Codrin.Ciubotariu@microchip.com wrote: >> On 05.05.2022 16:58, Nicolas Ferre wrote: >>> I'm fine with that, but I see that some Kconfig entries "select" this >>> SND_SOC_DMIC directly (amd, intel, mediatek, stm). >>> If it's absolutely needed for PDMC to work, what about doing the same as >>> it would prevent some broken configurations? >> The only way it makes sense to me to have this driver selected somewhere >> is in a sound card driver, used for a specific board, which we know it >> has PDM microphones. Since, for now, we use the simple sound card for >> our audio interfaces, we have no place to add this select. >> The reason I do not like to add this select under the controller driver, >> as some of the vendors did, is because, in the future, we might have >> different PDM microphones that might not work with SND_SOC_DMIC and >> might need a different driver. >> I don't have a strong opinion on this. If you think I am overthinking, >> please let me know and I will change this. > It's unlikely but possible that there could be some other device > connected (eg, you could have a DSP or something that generates PDM > output). If the driver doesn't directly instantiate the DMIC itself > then it's probably reasonable for it to be user controllable if the DMIC > driver is there. Fair enough, It makes perfect sense to me as it is then. Thanks for the feedback! Best regards, Nicolas