LinuxLists.cc - [PATCH v2 0/5] Sound support for at91sam9x5-wm8731 based boards

2013-07-08 14:36:53

by Mark Brown

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH v2 3/5] ARM: AT91: DTS: sam9x5ek: add WM8731 codec

On Mon, Jul 08, 2013 at 03:29:51PM +0200, Richard Genoud wrote:
> The WM8731 codec on sam9x5ek board is on i2c, address 1A
>
> Signed-off-by: Richard Genoud <[email protected]>

Acked-by: Mark Brown <[email protected]>

This should go via arm-soc.

Attachments:

(No filename) (257.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-08 14:38:44

by Lars-Peter Clausen

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH v2 1/5] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards

On 07/08/2013 03:29 PM, Richard Genoud wrote:
[...]
> +/*
> + * Logic for a wm8731 as connected on a at91sam9x5 based board.
> + */
> +static int at91sam9x5ek_wm8731_init(struct snd_soc_pcm_runtime *rtd)
> +{
[...]
> + codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
> + SNDRV_PCM_RATE_32000 |
> + SNDRV_PCM_RATE_48000 |
> + SNDRV_PCM_RATE_96000;
> + codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
> + SNDRV_PCM_RATE_32000 |
> + SNDRV_PCM_RATE_48000 |
> + SNDRV_PCM_RATE_96000;

That's not right. The driver structure is shared between all instances of
the codec, a single instance should not modify it. If you need to constrain
the list of supported rates use snd_pcm_hw_constraint_list().

> +
> + /* set the codec system clock for DAC and ADC */
> + ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,
> + MCLK_RATE, SND_SOC_CLOCK_IN);
> + if (ret < 0) {
> + dev_err(dev, "ASoC: Failed to set WM8731 SYSCLK: %d\n", ret);
> + return ret;
> + }
> +
> + /* signal a DAPM event */
> + snd_soc_dapm_sync(dapm);

This should not be necessary.

> + return 0;
> +}

2013-07-08 16:28:47

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] ARM: AT91: DTS: sam9x5: add SSC DMA parameters

On Mon, Jul 08, 2013 at 03:29:50PM +0200, Richard Genoud wrote:
> Signed-off-by: Richard Genoud <[email protected]>
> ---
> arch/arm/boot/dts/at91sam9x5.dtsi | 3 +++

There was no binding document in your first patch - there should be one
for any new DT bindings.

Attachments:

(No filename) (275.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-08 19:40:03

On Tue, Jul 09, 2013 at 10:19:45AM +0200, Richard Genoud wrote:
> 2013/7/8 Mark Brown <[email protected]>:

> > The obvious question here is of course if we can use the same driver for
> > both of them.

> I haven't got a g20 to test that, but that's the goal.

I think I do somewhere.

> For now, g20 is still non-DT, so I think it's best to have a DT-only
> driver like this one for the 9x5 family (9g15, 9g25, 9x25, 9g35,
> 9x25).
> When the g20 will move to DT completely, we can drop sam9g20_wm8731.c
> and adjust sam9x5_wm8731.c (mainly master clock and widgets it seems)
> By the way, maybe g45 could use that also (and SAMA5 ?)

If this is the goal then this driver needs a more generic name.

Attachments:

(No filename) (702.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-09 09:33:26

2013-07-09 14:26:05

by Richard Genoud

[permalink] [raw]

Subject: [PATCH v4 2/7] Documentation: DT: add sam9x5ek-wm8731 machine driver

2013-07-09 14:27:30

by Richard Genoud

[permalink] [raw]

Subject: [PATCH v4 4/7] ARM: AT91: DTS: sam9x5: add SSC DMA parameters

2013-07-09 14:27:51

by Richard Genoud

[permalink] [raw]

Subject: [PATCH v4 3/7] Documentation: DT: update atmel SSC with DMA binding

As atmel-ssc can be used with DMA, the documentation should be updated.
Also, a configuration DMA example is given.

Signed-off-by: Richard Genoud <[email protected]>
---
.../devicetree/bindings/misc/atmel-ssc.txt | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/misc/atmel-ssc.txt b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
index 38e51ad..7c320da 100644
--- a/Documentation/devicetree/bindings/misc/atmel-ssc.txt
+++ b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
@@ -6,10 +6,28 @@ Required properties:
- atmel,at91sam9g45-ssc: support dma transfer
- reg: Should contain SSC registers location and length
- interrupts: Should contain SSC interrupt
+For dma transfer:
+- dmas: DMA specifier, consisting of a phandle to DMA controller node,
+ the memory interface and SSC DMA channel ID (for tx and rx).
+ See Documentation/devicetree/bindings/dma/atmel-dma.txt for details.
+- dma-names: Must be "tx", "rx".

-Example:
+Examples:
ssc0: ssc@fffbc000 {
compatible = "atmel,at91rm9200-ssc";
reg = <0xfffbc000 0x4000>;
interrupts = <14 4 5>;
};
+
+
+ssc0: ssc@f0010000 {
+ compatible = "atmel,at91sam9g45-ssc";
+ reg = <0xf0010000 0x4000>;
+ interrupts = <28 4 5>;
+ dmas = <&dma0 1 13>,
+ <&dma0 1 14>;
+ dma-names = "tx", "rx";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ssc0_tx &pinctrl_ssc0_rx>;
+ status = "disabled";
+};
--
1.7.10.4

2013-07-09 14:42:49

On Tue, Jul 09, 2013 at 04:25:27PM +0200, Richard Genoud wrote:

> +/*
> + * Authorized rates are:
> + * Rate = MCLK_RATE / (n * 2)
> + * Where n is in [1..4095]
> + * (cf register SSC_CMR)
> + */
> +static unsigned int rates[] = {
> + 8000,
> + 16000,
> + 32000,
> + 48000,
> + 64000,
> + 96000,
> +};

Shouldn't the SSC driver be enforcing this constraint if it comes from
the SSC hardware? If the clock is reprogrammable the usual convention
for drivers is to not constrain if the clock is set to zero so a machine
driver could remove the constraint.

> + ret = atmel_ssc_set_audio(0);
> + if (ret != 0) {
> + dev_err(&pdev->dev,
> + "ASoC: Failed to set SSC 0 for audio: %d\n", ret);
> + return ret;
> + }

Shouldn't this be a parameter in the DT too?

> + cpu_np = of_parse_phandle(np, "atmel,ssc-controller", 0);
> + if (!cpu_np) {
> + dev_err(&pdev->dev, "ssc controller node missing\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + at91sam9x5ek_dai.cpu_of_node = cpu_np;
> + at91sam9x5ek_dai.platform_of_node = cpu_np;

After all we're looking things up in the DT...

> + at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,");

Is this really something that machines would want to reconfigure? If so
why?

Attachments:

(No filename) (1.20 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-09 15:01:20

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v4 2/7] Documentation: DT: add sam9x5ek-wm8731 machine driver

On Tue, Jul 09, 2013 at 04:25:28PM +0200, Richard Genoud wrote:
> This add the sound DT binding for sam9x5ek-wm8731 machine driver
>
> Signed-off-by: Richard Genoud <[email protected]>
> ---
> .../bindings/sound/atmel-sam9x5-wm8731-audio.txt | 30 ++++++++++++++++++++

Put new binding documents in the same patch that reads them, this makes
review easier.

> + - atmel,audio-routing: A list of the connections between audio components.

This needs to be more specific and list the available board specific
nodes for routing. For the CODEC you can just refer to the CODEC
binding documentation.

> + - atmel,format: DAI format. Must be "i2s"

So why not just omit this then?

> + - atmel,bitclock-master: DAI clock master
> + - atmel,frame-master: DAI frame master

The driver isn't handling these and there's no information on how to set
them.

Attachments:

(No filename) (863.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-09 15:06:14

On Wed, Jul 10, 2013 at 11:25:37AM +0200, Richard Genoud wrote:
> 2013/7/9 Mark Brown <[email protected]>:

> > Shouldn't the SSC driver be enforcing this constraint if it comes from
> > the SSC hardware? If the clock is reprogrammable the usual convention
> > for drivers is to not constrain if the clock is set to zero so a machine
> > driver could remove the constraint.

> Actually, my comment is buggy here (or at least, unrelated to the
> authorized rates).
> The "MCLK_RATE" is the master clock of the wm8731 codec (a 12.288MHz crystal).
> According to the datasheet of wm8731, when a 12.288 crystal is used,
> the authorized rates are 8, 32, 48 and 96kHz (I have to remove 16 and
> 64kHz).

> So, is this the right place for the rates ?

No, the CODEC driver should be enforcing the restrictions if that's
where they come from.

Attachments:

(No filename) (838.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-10 10:01:58

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v4 3/7] Documentation: DT: update atmel SSC with DMA binding

On Wed, Jul 10, 2013 at 11:48:27AM +0200, Richard Genoud wrote:
> 2013/7/9 Mark Brown <[email protected]>:

> > ...this first example is now invalid and should probably just be being
> > extended with the new required properties.

> Well, I have to rewrite that to make it clearer.
> The thing is:
> with "atmel,at91rm9200-ssc" the SSC doesn't work with DMA.
> with "atmel,at91sam9g45-ssc", the SSC work ONLY with DMA.

> So the dmas/dma-names properties are only required for g45-ssc, and
> useless for rm9200-ssc

> Maybe the best will be to write a paragraph for g45-ssc and another
> for rm9200-ssc, even if there's some identical lines between them.

OK, or just write a section "Required for devices with compatible X".

Attachments:

(No filename) (727.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments