2022-03-07 15:08:00

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 0/6] Add driver for SAMA7G5's PDMC

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.

The patch series starts with a fix on the ASoC DMA engine support. Then
continues with the bindings and the driver of PDMC. It is followed by the
DT nodes for SAMA7G5 and SAMA7G5-EK. In the end, the drivers for PDMC
and PDM microphones are enabled in sama7_defconfig.

Changes in v3:
- addressed new DT bindings comments from Krzysztof Kozlowski

Changes in v2:
- addressed DT bindings comments from Krzysztof Kozlowski

Codrin Ciubotariu (6):
ASoC: dmaengine: do not use a NULL prepare_slave_config() callback
ASoC: dt-bindings: Document Microchip's PDMC
ASoC: atmel: mchp-pdmc: add PDMC driver
ARM: dts: at91: sama7g5: add nodes for PDMC
ARM: dts: at91: sama7g5ek: add node for PDMC0
ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

.../bindings/sound/microchip,pdmc.yaml | 100 ++
arch/arm/boot/dts/at91-sama7g5ek.dts | 21 +-
arch/arm/boot/dts/sama7g5.dtsi | 24 +
arch/arm/configs/sama7_defconfig | 2 +
include/dt-bindings/sound/microchip,pdmc.h | 13 +
sound/soc/atmel/Kconfig | 16 +
sound/soc/atmel/Makefile | 2 +
sound/soc/atmel/mchp-pdmc.c | 1084 +++++++++++++++++
sound/soc/soc-generic-dmaengine-pcm.c | 6 +-
9 files changed, 1264 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/microchip,pdmc.yaml
create mode 100644 include/dt-bindings/sound/microchip,pdmc.h
create mode 100644 sound/soc/atmel/mchp-pdmc.c

--
2.32.0


2022-03-07 20:36:36

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 2/6] ASoC: dt-bindings: Document Microchip's PDMC

Add DT bindings for the new Microchip PDMC embedded in sama7g5 SoCs.

Signed-off-by: Codrin Ciubotariu <[email protected]>
---

Changes in v3:
- set line length to 80 characters long
- set 'reg' as the second property

Changes in v2:
- renamed patch from 'ASoC: add DT bindings for Microchip PDMC' to
'ASoC: dt-bindings: Document Microchip's PDMC';
- renamed yaml file from 'mchp,pdmc.yaml' to 'microchip,pdmc.yaml';
- used imperative mode in commit description;
- renamed mchp,pdmc.h to microchip,pdmc.h;
- fixed 'title' to represent HW;
- made 'compatible' first property;
- s/microhpone/microphone
- none name in example set to 'sound'

.../bindings/sound/microchip,pdmc.yaml | 100 ++++++++++++++++++
include/dt-bindings/sound/microchip,pdmc.h | 13 +++
2 files changed, 113 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/microchip,pdmc.yaml
create mode 100644 include/dt-bindings/sound/microchip,pdmc.h

diff --git a/Documentation/devicetree/bindings/sound/microchip,pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,pdmc.yaml
new file mode 100644
index 000000000000..04414eb4ada9
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/microchip,pdmc.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/microchip,pdmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Pulse Density Microphone Controller
+
+maintainers:
+ - Codrin Ciubotariu <[email protected]>
+
+description:
+ The Microchip Pulse Density Microphone Controller (PDMC) interfaces up to 4
+ digital microphones having Pulse Density Modulated (PDM) outputs.
+
+properties:
+ compatible:
+ const: microchip,sama7g5-pdmc
+
+ reg:
+ maxItems: 1
+
+ "#sound-dai-cells":
+ const: 0
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: Peripheral Bus Clock
+ - description: Generic Clock
+
+ clock-names:
+ items:
+ - const: pclk
+ - const: gclk
+
+ dmas:
+ description: RX DMA Channel
+ maxItems: 1
+
+ dma-names:
+ const: rx
+
+ microchip,mic-pos:
+ description: |
+ Position of PDM microphones on the DS line and the sampling edge (rising
+ or falling) of the CLK line. A microphone is represented as a pair of DS
+ line and the sampling edge. The first microphone is mapped to channel 0,
+ the second to channel 1, etc.
+ $ref: /schemas/types.yaml#/definitions/uint32-matrix
+ items:
+ items:
+ - description: value for DS line
+ - description: value for sampling edge
+ anyOf:
+ - enum:
+ - [0, 0]
+ - [0, 1]
+ - [1, 0]
+ - [1, 1]
+ minItems: 1
+ maxItems: 4
+ uniqueItems: true
+
+required:
+ - compatible
+ - reg
+ - "#sound-dai-cells"
+ - interrupts
+ - clocks
+ - clock-names
+ - dmas
+ - dma-names
+ - microchip,mic-pos
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/at91.h>
+ #include <dt-bindings/dma/at91.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/sound/microchip,pdmc.h>
+
+ pdmc: sound@e1608000 {
+ compatible = "microchip,sama7g5-pdmc";
+ reg = <0xe1608000 0x4000>;
+ #sound-dai-cells = <0>;
+ interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
+ dmas = <&dma0 AT91_XDMAC_DT_PERID(37)>;
+ dma-names = "rx";
+ clocks = <&pmc PMC_TYPE_PERIPHERAL 68>, <&pmc PMC_TYPE_GCK 68>;
+ clock-names = "pclk", "gclk";
+ microchip,mic-pos = <MCHP_PDMC_DS0 MCHP_PDMC_CLK_POSITIVE>,
+ <MCHP_PDMC_DS0 MCHP_PDMC_CLK_NEGATIVE>,
+ <MCHP_PDMC_DS1 MCHP_PDMC_CLK_POSITIVE>,
+ <MCHP_PDMC_DS1 MCHP_PDMC_CLK_NEGATIVE>;
+ };
diff --git a/include/dt-bindings/sound/microchip,pdmc.h b/include/dt-bindings/sound/microchip,pdmc.h
new file mode 100644
index 000000000000..96cde94ce74f
--- /dev/null
+++ b/include/dt-bindings/sound/microchip,pdmc.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DT_BINDINGS_MICROCHIP_PDMC_H__
+#define __DT_BINDINGS_MICROCHIP_PDMC_H__
+
+/* PDM microphone's pin placement */
+#define MCHP_PDMC_DS0 0
+#define MCHP_PDMC_DS1 1
+
+/* PDM microphone clock edge sampling */
+#define MCHP_PDMC_CLK_POSITIVE 0
+#define MCHP_PDMC_CLK_NEGATIVE 1
+
+#endif /* __DT_BINDINGS_MICROCHIP_PDMC_H__ */
--
2.32.0

2022-03-07 21:41:03

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback

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 <[email protected]>
---

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);
--
2.32.0

2022-03-07 22:34:08

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 6/6] ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

Enable drivers needed for Microchip's PDMC and PDM microphones.

Signed-off-by: Codrin Ciubotariu <[email protected]>
---

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
CONFIG_SND_SOC_PCM5102A=y
CONFIG_SND_SOC_SPDIF=y
CONFIG_SND_SIMPLE_CARD=y
--
2.32.0

2022-03-07 23:19:39

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 5/6] ARM: dts: at91: sama7g5ek: add node for PDMC0

SAMA7G5-EK has 4 PDM microphones connected to PDMC0. PDMC0 pinmux is in
conflict with gmac1, gmac1 being enabled by default.

Signed-off-by: Codrin Ciubotariu <[email protected]>
---

Changes in v3:
- none

Changes in v2:
- replaced included file dt-bindings/sound/mchp,pdmc.h wih
dt-bindings/sound/microchip,pdmc.h

arch/arm/boot/dts/at91-sama7g5ek.dts | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sama7g5ek.dts b/arch/arm/boot/dts/at91-sama7g5ek.dts
index ccf9e224da78..a1fb980d3fc1 100644
--- a/arch/arm/boot/dts/at91-sama7g5ek.dts
+++ b/arch/arm/boot/dts/at91-sama7g5ek.dts
@@ -14,6 +14,7 @@
#include <dt-bindings/mfd/atmel-flexcom.h>
#include <dt-bindings/input/input.h>
#include <dt-bindings/pinctrl/at91.h>
+#include <dt-bindings/sound/microchip,pdmc.h>

/ {
model = "Microchip SAMA7G5-EK";
@@ -439,7 +440,7 @@ &gmac1 {
&pinctrl_gmac1_mdio_default
&pinctrl_gmac1_phy_irq>;
phy-mode = "rmii";
- status = "okay";
+ status = "okay"; /* Conflict with pdmc0. */

ethernet-phy@0 {
reg = <0x0>;
@@ -453,6 +454,17 @@ &i2s0 {
pinctrl-0 = <&pinctrl_i2s0_default>;
};

+&pdmc0 {
+ #sound-dai-cells = <0>;
+ microchip,mic-pos = <MCHP_PDMC_DS0 MCHP_PDMC_CLK_NEGATIVE>, /* MIC 1 */
+ <MCHP_PDMC_DS1 MCHP_PDMC_CLK_NEGATIVE>, /* MIC 2 */
+ <MCHP_PDMC_DS0 MCHP_PDMC_CLK_POSITIVE>, /* MIC 3 */
+ <MCHP_PDMC_DS1 MCHP_PDMC_CLK_POSITIVE>; /* MIC 4 */
+ status = "disabled"; /* Conflict with gmac1. */
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pdmc0_default>;
+};
+
&pioA {
pinctrl_flx0_default: flx0_default {
pinmux = <PIN_PE3__FLEXCOM0_IO0>,
@@ -609,6 +621,13 @@ pinctrl_mikrobus1_spi: mikrobus1_spi {
bias-disable;
};

+ pinctrl_pdmc0_default: pdmc0_default {
+ pinmux = <PIN_PD23__PDMC0_DS0>,
+ <PIN_PD24__PDMC0_DS1>,
+ <PIN_PD22__PDMC0_CLK>;
+ bias_disable;
+ };
+
pinctrl_qspi: qspi {
pinmux = <PIN_PB12__QSPI0_IO0>,
<PIN_PB11__QSPI0_IO1>,
--
2.32.0

2022-03-08 09:59:17

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 4/6] ARM: dts: at91: sama7g5: add nodes for PDMC

Microchip's SAMA7G5 embeds two PDMCs. The PDMCs can be used to connect 2x4
PDM microphones.

Signed-off-by: Codrin Ciubotariu <[email protected]>
---

Chnages in v3:
- none

Changes in v2:
- set 'sound' as nodes' name

arch/arm/boot/dts/sama7g5.dtsi | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
index eddcfbf4d223..70e86444194f 100644
--- a/arch/arm/boot/dts/sama7g5.dtsi
+++ b/arch/arm/boot/dts/sama7g5.dtsi
@@ -275,6 +275,30 @@ pwm: pwm@e1604000 {
status = "disabled";
};

+ pdmc0: sound@e1608000 {
+ compatible = "microchip,sama7g5-pdmc";
+ reg = <0xe1608000 0x1000>;
+ interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
+ #sound-dai-cells = <0>;
+ dmas = <&dma0 AT91_XDMAC_DT_PERID(37)>;
+ dma-names = "rx";
+ clocks = <&pmc PMC_TYPE_PERIPHERAL 68>, <&pmc PMC_TYPE_GCK 68>;
+ clock-names = "pclk", "gclk";
+ status = "disabled";
+ };
+
+ pdmc1: sound@e160c000 {
+ compatible = "microchip,sama7g5-pdmc";
+ reg = <0xe160c000 0x1000>;
+ interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+ #sound-dai-cells = <0>;
+ dmas = <&dma0 AT91_XDMAC_DT_PERID(38)>;
+ dma-names = "rx";
+ clocks = <&pmc PMC_TYPE_PERIPHERAL 69>, <&pmc PMC_TYPE_GCK 69>;
+ clock-names = "pclk", "gclk";
+ status = "disabled";
+ };
+
spdifrx: spdifrx@e1614000 {
#sound-dai-cells = <0>;
compatible = "microchip,sama7g5-spdifrx";
--
2.32.0

2022-03-09 00:59:10

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 2/6] ASoC: dt-bindings: Document Microchip's PDMC

On 07/03/2022 13:21, Codrin Ciubotariu wrote:
> Add DT bindings for the new Microchip PDMC embedded in sama7g5 SoCs.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> ---
>
> Changes in v3:
> - set line length to 80 characters long
> - set 'reg' as the second property
>
> Changes in v2:
> - renamed patch from 'ASoC: add DT bindings for Microchip PDMC' to
> 'ASoC: dt-bindings: Document Microchip's PDMC';
> - renamed yaml file from 'mchp,pdmc.yaml' to 'microchip,pdmc.yaml';
> - used imperative mode in commit description;
> - renamed mchp,pdmc.h to microchip,pdmc.h;
> - fixed 'title' to represent HW;
> - made 'compatible' first property;
> - s/microhpone/microphone
> - none name in example set to 'sound'
>


Reviewed-by: Krzysztof Kozlowski <[email protected]>


Best regards,
Krzysztof

2022-03-09 01:41:19

by Mark Brown

[permalink] [raw]
Subject: Re: (subset) [PATCH v3 0/6] Add driver for SAMA7G5's PDMC

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

2022-04-21 20:09:47

by Sascha Hauer

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback

On Wed, Apr 20, 2022 at 09:58:06AM +0000, [email protected] 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 <[email protected]>
> >> ---
> >>
> >> 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/[email protected]/
> > 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


--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-04-22 13:10:51

by Codrin Ciubotariu

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback

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, [email protected] 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 <[email protected]>
>>>> ---
>>>>
>>>> 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/[email protected]/
>>> 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

2022-04-22 21:06:25

by Sascha Hauer

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback

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 <[email protected]>
> ---
>
> 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/[email protected]/
on current master.

Sascha

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-04-22 22:05:15

by Codrin Ciubotariu

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] ASoC: dmaengine: do not use a NULL prepare_slave_config() callback

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 <[email protected]>
>> ---
>>
>> 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/[email protected]/
> 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

2022-05-07 06:43:09

by Nicolas Ferre

[permalink] [raw]
Subject: Re: [PATCH v3 6/6] ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

On 05/05/2022 at 17:01, Mark Brown wrote:
> On Thu, May 05, 2022 at 02:47:04PM +0000,[email protected] 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

--
Nicolas Ferre

2022-05-09 02:43:06

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v3 6/6] ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

On Thu, May 05, 2022 at 02:47:04PM +0000, [email protected] 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.


Attachments:
(No filename) (1.32 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-09 03:14:21

by Codrin Ciubotariu

[permalink] [raw]
Subject: Re: [PATCH v3 6/6] ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

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 <[email protected]>
>> ---
>>
>> 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

2022-05-09 06:37:34

by Nicolas Ferre

[permalink] [raw]
Subject: Re: [PATCH v3 6/6] ARM: configs: at91: sama7_defconfig: add MCHP PDMC and DMIC drivers

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 <[email protected]>
> ---
>
> 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


--
Nicolas Ferre