2013/7/9 Mark Brown [off-list ref]:

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

quoted

+/*
+ * 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.

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 ?

quoted

+     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?

Yes, I'll add that to the DT.

quoted

+     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...

quoted

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

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

That's right. There's no point reconfiguring that because I2S is the
only possible interface.


Thanks for your comments !


Richard.