ASoC drivers for the Freescale MPC8610 SoC

This patch adds ALSA SoC device drivers for the Freescale MPC8610 SoC and the MPC8610-HPCD reference board.

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +0 -0

index 6390895..6e1bde3 100644

sound/soc/fsl/Kconfig new file +0 -0

new file mode 100644
index 0000000..4a5bbd2

sound/soc/fsl/fsl_dma.c new file +57 -0

new file mode 100644
index 0000000..6b86be0
@@ -0,0 +1,819 @@
+/*
+ * Freescale DMA ALSA SoC PCM driver
+ *
+ * Author: Timur Tabi <[email protected]>
+ *
+ * Copyright 2007 Freescale Semiconductor, Inc.  This file is licensed under
+ * the terms of the GNU General Public License version 2.  This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ *
+ * This driver implements ASoC support for the Elo DMA controller, which is
+ * the DMA controller on Freescale 83xx, 85xx, and 86xx SOCs. In ALSA terms,
+ * the PCM driver is what handles the DMA buffer.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+
+#include <sound/driver.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include <asm/io.h>
+
+#include "fsl_dma.h"
+
+/*
+ * The formats that the DMA controller supports, which is anything
+ * that is 8, 16, or 32 bits.
+ */
+#define FSLDMA_PCM_FORMATS (SNDRV_PCM_FMTBIT_S8 	| \
+			    SNDRV_PCM_FMTBIT_U8 	| \
+			    SNDRV_PCM_FMTBIT_S16_LE     | \
+			    SNDRV_PCM_FMTBIT_S16_BE     | \
+			    SNDRV_PCM_FMTBIT_U16_LE     | \
+			    SNDRV_PCM_FMTBIT_U16_BE     | \
+			    SNDRV_PCM_FMTBIT_S24_LE     | \
+			    SNDRV_PCM_FMTBIT_S24_BE     | \
+			    SNDRV_PCM_FMTBIT_U24_LE     | \
+			    SNDRV_PCM_FMTBIT_U24_BE     | \
+			    SNDRV_PCM_FMTBIT_S32_LE     | \
+			    SNDRV_PCM_FMTBIT_S32_BE     | \
+			    SNDRV_PCM_FMTBIT_U32_LE     | \
+			    SNDRV_PCM_FMTBIT_U32_BE)
+
+#define FSLDMA_PCM_RATES (SNDRV_PCM_RATE_5512 | SNDRV_PCM_RATE_8000_192000 | \
+			  SNDRV_PCM_RATE_CONTINUOUS)
+
+/* DMA global data.  This structure is used by fsl_dma_open() to determine
+ * which DMA channels to assign to a substream.  Unfortunately, ASoC V1 does
+ * not allow the machine driver to provide this information to the PCM

sound/soc/fsl/fsl_dma.c new file +5 -0

+/*
+ * Initialize this PCM driver.
+ *
+ * This function is called when the codec driver calls snd_soc_new_pcms(),
+ * once for each .dai_link in the machine driver's snd_soc_machine

arch/powerpc/platforms/86xx/mpc8610_hpcd.c line 43

Quoted 3 lines

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +3 -0

+static struct of_device_id mpc8610_ids[] = {
+	{ .type = "soc", },
+	{}

Please scan based on compatible instead of device_type.

sound/soc/fsl/Kconfig line 9

Quoted 7 lines

sound/soc/fsl/Kconfig new file +7 -0

+config SND_SOC_MPC8610
+	bool "ALSA SoC support for the MPC8610 SOC"
+	depends on SND_SOC #&& MPC8610_HPCD
+	default y #if MPC8610
+	help
+	  Say Y if you want to add support for codecs attached to the SSI
+          device on an MPC8610.

Don't default configs to 'y'. Also, what's with the commented-out dependencies and if?

Quoted 6 lines

+ * ssi_stx_phys: bus address of SSI STX register
+ * ssi_srx_phys: bus address of SSI SRX register
+ * dma_channel: pointer to the DMA channel's registers
+ * irq: IRQ for this DMA channel
+ * assigned: set to 1 if that DMA channel is assigned to a substream
+ */

This goes for the whole patch: You've got good documentation, but it's not in docbook format. Please reformat it since it should be a pretty simple thing to do.

sound/soc/fsl/fsl_dma.c line 284

Quoted 8 lines

sound/soc/fsl/fsl_dma.c new file +8 -0

+static int fsl_dma_new(struct snd_card *card, struct snd_soc_codec_dai *dai,
+	struct snd_pcm *pcm)
+{
+	static u64 fsl_dma_dmamask = 0xffffffff;
+	int ret;
+
+	if (!card->dev->dma_mask)
+		card->dev->dma_mask = &fsl_dma_dmamask;

I haven't read how your channel allocation works, but providing a pointer to a local static variable is a bit fishy no matter what.

Do you ever anticipate having other dma users on the system, such as memcpy offload? You'll probably need allocation support for channels when that day comes (I ended up writing a simple library for dma channel management for that very reason on my platform).

Quoted 12 lines

sound/soc/fsl/fsl_dma.c line 284

Quoted 9 lines

sound/soc/fsl/fsl_dma.c new file +8 -0

+static int fsl_dma_new(struct snd_card *card, struct snd_soc_codec_dai *dai,
+	struct snd_pcm *pcm)
+{
+	static u64 fsl_dma_dmamask = 0xffffffff;
+	int ret;
+
+	if (!card->dev->dma_mask)
+		card->dev->dma_mask = &fsl_dma_dmamask;

I haven't read how your channel allocation works, but providing a pointer to a local static variable is a bit fishy no matter what.

I just copied this code from another module. All the ALSA drivers do this,

All? No, only a few...
For PCI, usually pci_set_dma_mask() and pci_set_consistent_dma_mask() are used, of course.

At Thu, 20 Dec 2007 08:24:35 -0600,

Quoted 13 lines · Timur Tabi

Quoted 12 lines

sound/soc/fsl/fsl_dma.c line 284

Quoted 9 lines

sound/soc/fsl/fsl_dma.c new file +8 -0

+static int fsl_dma_new(struct snd_card *card, struct snd_soc_codec_dai *dai,
+  struct snd_pcm *pcm)
+{
+  static u64 fsl_dma_dmamask = 0xffffffff;
+  int ret;
+
+  if (!card->dev->dma_mask)
+          card->dev->dma_mask = &fsl_dma_dmamask;

I haven't read how your channel allocation works, but providing a pointer to a local static variable is a bit fishy no matter what.

Quoted 1 line

I just copied this code from another module. All the ALSA drivers do this,

All? No, only a few...
For PCI, usually pci_set_dma_mask() and pci_set_consistent_dma_mask() are used, of course.

Please use DMA_32BIT_MASK (see include/linux/dma-mapping.h) instead of 0xffffffff.

I've personally fixed a heisenbug in an ALSA driver caused by incorrectly typed DMA mask...

Quoted 5 lines · Olof Johansson

arch/powerpc/platforms/86xx/mpc8610_hpcd.c line 43

Quoted 3 lines

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +3 -0

+static struct of_device_id mpc8610_ids[] = {
+	{ .type = "soc", },
+	{}

Please scan based on compatible instead of device_type.

I was just following the example from another board file. However, the 'soc' node in the device tree does not have a compatible property, so I don't how to change this.

Quoted 4 lines

Do you ever anticipate having other dma users on the system, such as memcpy offload? You'll probably need allocation support for channels when that day comes (I ended up writing a simple library for dma channel management for that very reason on my platform).

Yes, I plan on updating this driver to work with the standard Freescale "Elo" device driver, but that will have to wait until that code is in the kernel and stabilized. The SSI is limited in which DMA channels it can use, anyway.

Quoted 3 lines

I was just following the example from another board file. However, the 'soc' node in the device tree does not have a compatible property, so I don't how to change this.

Then add an appropriate compatible entry to it, please.

Quoted 2 lines · Olof Johansson

Quoted 1 line

I was just following the example from another board file. However, the 'soc' node in the device tree does not have a compatible property, so I don't how to change this.

Then add an appropriate compatible entry to it, please.

None of the SOC nodes in any DTS have a "compatible" entry.

Quoted 1 line

None of the SOC nodes in any DTS have a "compatible" entry.

Not quite true; ep88xc, mpc8272ads, and pq2fads have them.

Quoted 3 lines · Scott Wood

Quoted 1 line

None of the SOC nodes in any DTS have a "compatible" entry.

Not quite true; ep88xc, mpc8272ads, and pq2fads have them.

Ah ok. So what should the compatible entry for 8641 be?

	compatible = "fsl,mpc8641"

That looks a lot like what a compatible entry for the CPU should be.

sound/soc/fsl/mpc8610_hpcd.c new file +9 -0

new file mode 100644
index 0000000..5421a5c
@@ -0,0 +1,621 @@
+/**
+ * Freescale MPC8610HPCD ALSA SoC Fabric driver
+ *
+ * Author: Timur Tabi <[email protected]>
+ *
+ * Copyright 2007 Freescale Semiconductor, Inc.  This file is licensed under
+ * the terms of the GNU General Public License version 2.  This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +6 -0

+static struct of_device_id mpc8610_ids[] = {
+       { .type = "soc", },
+       {}
+};
+
+static int __init mpc8610_declare_of_platform_devices(void)

How is of_platform_bus_probe() supposed to be called? mpc5200/virtex call it with three NULLs. Is it necessary to name all of the buses in a of_device_id? If it's not necessary to list the buses the of_platform_bus_probe() call could be moved to common code.

Are these buses?

        { .compatible = "ibm,plb4", },
        { .compatible = "ibm,opb", },
        { .compatible = "ibm,ebc", },

Could of_platform_bus_probe() be simplified? No one uses the first and third parameters.

Quoted 4 lines · Jon Smirl

How is of_platform_bus_probe() supposed to be called? mpc5200/virtex call it with three NULLs. Is it necessary to name all of the buses in a of_device_id? If it's not necessary to list the buses the of_platform_bus_probe() call could be moved to common code.

I added the above code because it is the only way I could get my SSI nodes to be probed. If there's a better way to do it, I'm all ears. I just copied that code from the mpc836x_mds.c platform file.

Quoted 5 lines

Are these buses?

        { .compatible = "ibm,plb4", },
        { .compatible = "ibm,opb", },
        { .compatible = "ibm,ebc", },

I have no idea.

Quoted 2 lines

Could of_platform_bus_probe() be simplified? No one uses the first and third parameters.

Maybe, but that's not a discussion for this thread!

--
Timur Tabi
Linux kernel developer at Freescale

mpc5200 does it like this:
of_platform_bus_probe(NULL, NULL, NULL);

Quoted 2 lines · Jon Smirl

mpc5200 does it like this:
of_platform_bus_probe(NULL, NULL, NULL);

I think that tells the OF base code to probe everything in the device tree, which is probably overkill. I think fsl_soc.c covers most of the device tree, but the SSI is not defined in fsl_soc.c.

Quoted 5 lines · Timur Tabi

Quoted 2 lines · Jon Smirl

mpc5200 does it like this:
of_platform_bus_probe(NULL, NULL, NULL);

Quoted 3 lines

I think that tells the OF base code to probe everything in the device tree, which is probably overkill. I think fsl_soc.c covers most of the device tree, but the SSI is not defined in fsl_soc.c.

Not quite; it tells it to use a built-in list of bus matches. Most of which are device_type-based, FWIW.

-Scott

Quoted 6 lines · Scott Wood

Quoted 5 lines · Timur Tabi

Quoted 2 lines · Jon Smirl

mpc5200 does it like this:
of_platform_bus_probe(NULL, NULL, NULL);

Quoted 6 lines

Quoted 3 lines

I think that tells the OF base code to probe everything in the device tree, which is probably overkill. I think fsl_soc.c covers most of the device tree, but the SSI is not defined in fsl_soc.c.

Not quite; it tells it to use a built-in list of bus matches. Most of which are device_type-based, FWIW.

Here's the default. Using NULL would work.

arch/powerpc/boot/dts/mpc8610_hpcd.dts line 119

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

Quoted 17 lines · Jon Smirl

arch/powerpc/boot/dts/mpc8610_hpcd.dts line 119

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

Quoted 2 lines

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

Looking at the cs4270 codec driver it is controlled by i2c (supports SPI too). What happened to the conversation about putting codecs on the controlling bus and then linking them to the data bus?

If that's the case the cs4270 should be in the i2c bus node (missing currently) and then a link from the SSI bus would point to it.

Quoted 18 lines · Jon Smirl

Quoted 17 lines · Jon Smirl

arch/powerpc/boot/dts/mpc8610_hpcd.dts line 119

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

Quoted 6 lines

Quoted 2 lines

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

Looking at the cs4270 codec driver it is controlled by i2c (supports SPI too). What happened to the conversation about putting codecs on the controlling bus and then linking them to the data bus?

The current CS4270 driver doesn't support device trees. When I wrote it, the idea of putting I2C info in the device tree was not finalized, and since the driver is supposed to be cross-platform, I decided to do it the old-fashioned way. Before I update the code, however, I'm waiting for:

1) The current code to be accepted into the tree
2) ASoC is updated to V2
3) The current drivers are updated to support ASoC V2.

I think ASoC V2 will make it easier to support device trees, but I'm not ready yet for that.

Quoted 3 lines

If that's the case the cs4270 should be in the i2c bus node (missing currently) and then a link from the SSI bus would point to it.

The CS4270 is a child of both the I2C bus *and* the SSI bus. It needs to have two nodes, one under each. Your're right in that there needs to be a link, but until the code is updated to ASoC V2, I think it's premature to add that support.

Quoted 30 lines · Timur Tabi

Quoted 15 lines · Jon Smirl

Quoted 14 lines · Jon Smirl

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Quoted 8 lines

Quoted 5 lines

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

Looking at the cs4270 codec driver it is controlled by i2c (supports SPI too). What happened to the conversation about putting codecs on the controlling bus and then linking them to the data bus?

The current CS4270 driver doesn't support device trees. When I wrote it, the idea of putting I2C info in the device tree was not finalized, and since the driver is supposed to be cross-platform, I decided to do it the old-fashioned way. Before I update the code, however, I'm waiting for:

Quoted 3 lines

1) The current code to be accepted into the tree
2) ASoC is updated to V2
3) The current drivers are updated to support ASoC V2.

I've been trying to get the i2c code in for two months. Hopefully it will go in soon, no one had made any comments on it recently. Have you tried your code with it?

There is nothing stopping your from putting a node for the CS4270 on the i2c bus today. It just won't trigger the loading of the driver.

Don't we want to follow the device tree policy of putting the device on the controlling bus and then link it to the data bus?

It makes it a little easier but it doesn't fix everything. We need to start looking at it since none of the example driver for it are device tree based.

It still has problems with wanting 'struct
platform_device' when we have 'struct of_platform_device' pointers. It also doesn't know how to dynamically load codecs based on device trees.

Liam messed up all of my code when he refactored it in late December.

I've switched over to the current SOC code for the moment. The big thing that v2 fixes is that SOC is changed to being a subsystem instead of platform driver. Being a subsystem is the correct model.

It would be good if more pieces of v2 get push forward. Then we can sort out the device tree issues in it.

Adding the second device tree node doesn't have anything to do with ASOC v2. It's specific to powerpc and device trees.

Quoted 2 lines · Jon Smirl

Don't we want to follow the device tree policy of putting the device on the controlling bus and then link it to the data bus?

Normally, that sounds like a good idea, but the cs4270 is an I2S device first, and an I2C device second. I need to be able to find that codec from the I2S node. My I2S driver would not know to scan all I2C devices to find the codec.

The device tree is a description of the hardware; not software. It's not a good idea to break with convention due to current driver architecture.

Quoted 3 lines · Grant Likely

The device tree is a description of the hardware; not software. It's not a good idea to break with convention due to current driver architecture.

I believe that with ASoC V1, I'm stuck between a rock and a hard place, and so the only way to make this code work is to bend some rules. Right now, the CS4270 driver does not support platform drivers or the device tree, so there's no point in putting a child I2C node for it. As I mentioned in other posts, I will be more than happy to update the CS4270 driver to support this new paradigm (which was invented after the CS4270 driver was written) *after* this current patchset is applied.

Quoted 4 lines · Jon Smirl

Quoted 3 lines · Timur Tabi

Quoted 2 lines · Jon Smirl

Quoted 1 line · Jon Smirl

On 12/19/07, Timur Tabi <[email protected]> wrote:

Quoted 2 lines

Don't we want to follow the device tree policy of putting the device on the controlling bus and then link it to the data bus?

Normally, that sounds like a good idea, but the cs4270 is an I2S device first, and an I2C device second. I need to be able to find that codec from the I2S node. My I2S driver would not know to scan all I2C devices to find the codec.

And what distinction are you drawing between "first" and "second" here?

Why would the I2S need to scan for codecs? Wouldn't it be up to the codec driver to register with I2S?

Quoted 2 lines · David Gibson

And what distinction are you drawing between "first" and "second" here?

Oh, that's an easy one: The CS4270 can work without an I2C or SPI connection, but it will never work without an I2S connection.

Quoted 2 lines

Why would the I2S need to scan for codecs? Wouldn't it be up to the codec driver to register with I2S?

Not in ASoC V1. The codec driver registers with ASoC, but the actual connection to other devices (e.g. the I2S driver) is done either in the I2S driver or in the fabric driver, depending on your mood. And that connection is done via a pointer to a structure in the codec driver.

Ok, but couldn't you strucutre your I2S or fabric driver so that it only becomes fully operational once the codec driver has registered with it?

Ok, but couldn't you strucutre your I2S or fabric driver so that it only becomes fully operational once the codec driver has registered with it?

Quoted 3 lines · David Gibson

Ok, but couldn't you strucutre your I2S or fabric driver so that it only becomes fully operational once the codec driver has registered with it?

Not in ASoC V1. You have to understand, ASoC V1 was designed without any consideration for runtime-bindings and other OF goodies. All connections between the drivers are static, literally. In fact, I wouldn't be surprised if some ASoC drivers cannot be compiled as modules.

So all I'm trying to do now is get my driver, with warts and all, into the tree so that I can focus with Liam et al to get a real ASoC V2 up and running.

Quoted 10 lines · Timur Tabi

Quoted 3 lines · David Gibson

Ok, but couldn't you strucutre your I2S or fabric driver so that it only becomes fully operational once the codec driver has registered with it?

Not in ASoC V1. You have to understand, ASoC V1 was designed without any consideration for runtime-bindings and other OF goodies. All connections between the drivers are static, literally. In fact, I wouldn't be surprised if some ASoC drivers cannot be compiled as modules.

I'd just like to emphasise this point - ASoC v1 really doesn't understand the idea that the components of the sound subsystem might be probed separately. It's set up to handle bare hardware with everything being probed from code in the machine/fabric driver. This makes life very messy for platforms with something like the device tree.

As has been said, handling this properly is one of the major motivations behind ASoC v2.

Quoted 12 lines · Mark Brown

Quoted 10 lines · Timur Tabi

Quoted 3 lines · David Gibson

Ok, but couldn't you strucutre your I2S or fabric driver so that it only becomes fully operational once the codec driver has registered with it?

Not in ASoC V1. You have to understand, ASoC V1 was designed without any consideration for runtime-bindings and other OF goodies. All connections between the drivers are static, literally. In fact, I wouldn't be surprised if some ASoC drivers cannot be compiled as modules.

Quoted 8 lines

I'd just like to emphasise this point - ASoC v1 really doesn't understand the idea that the components of the sound subsystem might be probed separately. It's set up to handle bare hardware with everything being probed from code in the machine/fabric driver. This makes life very messy for platforms with something like the device tree.

As has been said, handling this properly is one of the major motivations behind ASoC v2.

Ick. Ok.

Nonetheless, messing up the device tree to workaround ASoC V1's silly limitations is not a good idea. The device tree must represent the hardware as much as possible. If that means we have to have a bunch of platform-specific hacks to instatiate the drivers in the correct order / combination, that's unfortunate, but there you go.

--

David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!

http://www.ozlabs.org/~dgibson

ASOC v2 is sitting on a Wolfson server out of tree. I have been using it for several months without problem. The pace of it being merged could probably be sped up.

I think we are probably looking at submission in the next 8 - 10 weeks. Currently most of the core code is complete, however some platforms and codecs still need porting.

Quoted 3 lines · Liam Girdwood

I think we are probably looking at submission in the next 8 - 10 weeks. Currently most of the core code is complete, however some platforms and codecs still need porting.

With that in mind, can I get some kind of consensus from the PPC side as to whether this ASoC V1 driver is okay? I want to get it into 2.6.25.

Keep in mind:

1) ASoC V1 is not PowerPC-friendly, so it's impossible to make an ASoC V1 PowerPC driver "100% correct".

The driver doesn't need to be 100% correct. Drivers are easy to change if they aren't quite right. There are no long term consequences.

However, the device tree issues must be addressed before it is merged and deployed. Otherwise we end up having to support poorly designed trees over the long term.

Quoted 4 lines · Grant Likely

The driver doesn't need to be 100% correct. Drivers are easy to change if they aren't quite right. There are no long term consequences.

Quoted 3 lines

However, the device tree issues must be addressed before it is merged and deployed. Otherwise we end up having to support poorly designed trees over the long term.

I agree. Correct me if I'm wrong, but I think the only device tree "issue" is the definition of the 'codec' node under the SSI node. If so, I'm not sure what other changes need to be mode.

--
Timur Tabi
Linux kernel developer at Freescale

Isn't your codec is i2c controlled? Where does the main node for the code live, i2c bus or ssi bus? What does the link between the buses look like for pointing to the codec entry?

What about fsl,ssi being too generic for a compatible type?

Quoted 3 lines · Jon Smirl

Isn't your codec is i2c controlled? Where does the main node for the code live, i2c bus or ssi bus? What does the link between the buses look like for pointing to the codec entry?

The CS4270 driver is "old style", which means it probes all possible I2C addresses until it finds a hit, and then stops. This has all the obvious drawbacks, but I'm stuck with that design for now.

Quoted 1 line

What about fsl,ssi being too generic for a compatible type?

Already fixed:

                ssi@16000 {
                        compatible = "fsl,mpc8610-ssi";

Nit: node name should be either i2s@16000 (the mode that it is in), or sound@16000 (if you feel that this node encapsulates the concept of a sound device enough).

Quoted 3 lines · Jon Smirl

Isn't your codec is i2c controlled? Where does the main node for the code live, i2c bus or ssi bus? What does the link between the buses look like for pointing to the codec entry?

The CS4270 driver is "old style", which means it probes all possible I2C addresses until it finds a hit, and then stops. This has all the obvious drawbacks, but I'm stuck with that design for now.

Quoted 2 lines

What about fsl,ssi being too generic for a compatible type?

Already fixed:

                ssi@16000 {
                        compatible = "fsl,mpc8610-ssi";

--
Timur Tabi
Linux kernel developer at Freescale

So the codec is controlled from the i2c bus and SSI is used to supply it with data. Based on what has been said on this list, the device tree node for the codec should be on the i2c bus with a link from the ssi bus to it.

So all I'm trying to do now is get my driver, with warts and all, into the tree so that I can focus with Liam et al to get a real ASoC V2 up and running.

Quoted 3 lines · Timur Tabi

So all I'm trying to do now is get my driver, with warts and all, into the tree so that I can focus with Liam et al to get a real ASoC V2 up and running.

I'll commit the MPC8610 into the ASoC (v1) dev tree soon (hopefully tonight). This will allow folks to use it in the meantime whilst we sort out any changes.

I'll then port (what I can) to V2, although I may need some assistance with some of the PPC sections.

Quoted 2 lines · Jon Smirl

If that's the case the cs4270 should be in the i2c bus node (missing currently) and then a link from the SSI bus would point to it.

The CS4270 is a child of both the I2C bus *and* the SSI bus. It needs to have two nodes, one under each. Your're right in that there needs to be a link, but until the code is updated to ASoC V2, I think it's premature to add that support.

Why not be a child of the i2c bus with a phandle to the ssi bus?

Quoted 1 line · Grant Likely

Why not be a child of the i2c bus with a phandle to the ssi bus?

Because when I probe the SSI node, I want to know what the attached codec is.
So if anything, I would need a pointer from the SSI bus *to* the respective
child on the I2C bus.

Quoted 6 lines · Timur Tabi

Quoted 1 line · Grant Likely

Why not be a child of the i2c bus with a phandle to the ssi bus?

Because when I probe the SSI node, I want to know what the attached codec is.
So if anything, I would need a pointer from the SSI bus *to* the respective
child on the I2C bus.

That's fine too (it's what is done with Ethernet PHYs). My preference is the other way around, but it's not a big issue in this case.

Quoted 8 lines · Grant Likely

Quoted 6 lines · Timur Tabi

Quoted 1 line · Grant Likely

Why not be a child of the i2c bus with a phandle to the ssi bus?

Because when I probe the SSI node, I want to know what the attached codec is.
So if anything, I would need a pointer from the SSI bus *to* the respective
child on the I2C bus.

Quoted 2 lines

That's fine too (it's what is done with Ethernet PHYs). My preference is the other way around, but it's not a big issue in this case.

I'd just link in both directions, and let software follow it in whichever direction it prefers.

Quoted 2 lines · Scott Wood

I'd just link in both directions, and let software follow it in whichever direction it prefers.

Gah! Don't do that! Then you need to maintain both directions in the dts file. Software is good at generating reverse mappings.

Quoted 2 lines · Grant Likely

Gah! Don't do that! Then you need to maintain both directions in the dts file. Software is good at generating reverse mappings.

Software is, however, lousy at correctly wading through poorly-structured data (which device trees are full of) to figure out how to locate the link it wants to follow backwards.

Quoted 8 lines · Scott Wood

Quoted 7 lines · Grant Likely

Quoted 6 lines · Timur Tabi

Quoted 1 line · Grant Likely

Why not be a child of the i2c bus with a phandle to the ssi bus?

Because when I probe the SSI node, I want to know what the attached codec is.
So if anything, I would need a pointer from the SSI bus *to* the respective
child on the I2C bus.

Quoted 4 lines

Quoted 2 lines

That's fine too (it's what is done with Ethernet PHYs). My preference is the other way around, but it's not a big issue in this case.

I'd just link in both directions, and let software follow it in whichever direction it prefers.

Gah! Don't do that! Then you need to maintain both directions in the dts file.

Software is good at generating reverse mappings.

Don't put
that burden on the dts author.

Quoted 2 lines · Grant Likely

Don't put
that burden on the dts author.

As the DTS author in question, I hereby declare that such a requirement is not a burden in the slightest. Thank you.

arch/powerpc/boot/dts/mpc8610_hpcd.dts line 119

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

arch/powerpc/boot/dts/mpc8610_hpcd.dts line 119

Quoted 13 lines · Timur Tabi

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+               ssi@16000 {
+                       compatible = "fsl,ssi";
+                       cell-index = <0>;
+                       reg = <16000 100>;
+                       interrupt-parent = <&mpic>;
+                       interrupts = <3e 2>;
+                       fsl,mode = "i2s-slave";
+                       codec {
+                               compatible = "cirrus,cs4270";
+                               /* MCLK source is a stand-alone oscillator */
+                               bus-frequency = <bb8000>;
+                       };
+               };

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

Quoted 2 lines · Jon Smirl

Does this need to be bus-frequency? It's always called MCLK in all of the literature.

I'm trying to make this node as generic as possible. The fabric driver is the one that will parse this node and pass the data to the codec driver, so I can't use any codec-specific terms.

The API from the fabric driver for passing clock information includes a clock ID, a direction, and a frequency. I can do something like this:

clock1 = <0, bb8000>

Would that be better?

Quoted 2 lines

In my case the MCLK comes from a chip on the i2c bus that is programmable How would that be encoded?.

I'm going under the assumption that MCLK does not change once the board is up and running. In your case, you'd need to do something quite different, because you're not reading the clock info from the device tree and passing it to the codec at initialization once. If you want to define an extension to the 'codec' child node that handles that, I'll add it to the documentation.

Quoted 3 lines

clock1 = <0, bb8000>

Would that be better?

To cover everything you'd need to be able to specify all the clocking parameters, especially a PLL configuration, and also specify more than one of each item. Even then you'd still have problems like...

According to the documentation in your patch the bus frequency should already be optional

Quoted 3 lines · Mark Brown

To cover everything you'd need to be able to specify all the clocking parameters, especially a PLL configuration, and also specify more than one of each item. Even then you'd still have problems like...

The ASoC V1 API for communicating clock data from the fabric driver to the codec driver only allows for three parameters.

Quoted 2 lines

According to the documentation in your patch the bus frequency should already be optional

My code does not currently support that configuration, and I don't have any hardware that works that way, so I don't know what it would look like. I'm just trying to make the driver as flexible as possible, given ASoC V1 constraints.

Each individual call to set_sysclk() only takes three parameters but it can be called repeatedly and some configurations are going to require this.

and of course the ordering matters.

Indeed. Providing the device tree stuff doesn't get set in stone I'm not sure we need to nail this down perfectly for ASoC v1 when we're running into trouble working around it.

Mark Brown wrote:

In other words, ...

clock1 = <0, bb8000>
clock2 = <1, 653230>
clock23 = <0, ab2372>

Quoted 1 line

and of course the ordering matters.

Ok, you got me there. But then, isn't this just another example where the device tree is incapable of describing a complex configuration, and so we need a platform driver?

Quoted 3 lines

Indeed. Providing the device tree stuff doesn't get set in stone I'm not sure we need to nail this down perfectly for ASoC v1 when we're running into trouble working around it.

I definitely agree with that. I'll be the first to admit that this driver, much like ASoC V1, is a prototype.

 sound/soc/fsl/fsl_ssi.c                      |  614 +++++++++++++++++++
 sound/soc/fsl/fsl_ssi.h                      |  224 +++++++

Quoted 2 lines · Timur Tabi

 sound/soc/fsl/fsl_ssi.c                      |  614 +++++++++++++++++++
 sound/soc/fsl/fsl_ssi.h                      |  224 +++++++

I'm confused about this part. You built a driver for the mpc8610 ssi port. This port has a device tree entry.

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+		ssi@16000 {
+			compatible = "fsl,ssi";
+			cell-index = <0>;
+			reg = <16000 100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <3e 2>;
+			fsl,mode = "i2s-slave";
+			codec {
+				compatible = "cirrus,cs4270";
+				/* MCLK source is a stand-alone oscillator */
+				bus-frequency = <bb8000>;
+			};
+		};

But then you don't create an of_platform_driver for this device. Instead you create one for the fabric driver, struct of_platform_driver mpc8610_hpcd_of_driver, and directly link the SSI driver into it.

sound/soc/fsl/mpc8610_hpcd.c new file +15 -0

+static struct of_device_id mpc8610_hpcd_match[] = {
+	{
+		.compatible = "fsl,ssi",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpc8610_hpcd_match);
+
+static struct of_platform_driver mpc8610_hpcd_of_driver = {
+	.owner  	= THIS_MODULE,
+	.name   	= "mpc8610_hpcd",
+	.match_table    = mpc8610_hpcd_match,
+	.probe  	= mpc8610_hpcd_probe,
+	.remove 	= mpc8610_hpcd_remove,
+};

static int mpc8610_hpcd_probe(struct of_device *ofdev,
const struct of_device_id *match)
{
.....

	machine_data->dai.cpu_dai = fsl_ssi_create_dai(&ssi_info);

Isn't this two separate drivers that have been combined into one driver? Or does the fsl_ssi channel only work on the mpc8610_hpcd?

This is the problem of knowing how to load the fabric driver that I was talking about in the other threads.

A device that can occur on
more than one chip ".compatible = "fsl,ssi"," is being used to pull in a platform specific fabric driver, "mpc8610_hpcd". You can use the kernel config system to select the right driver for ".compatible = "fsl,ssi"," that matches you hardware and compile it in.

But that doesn't work in my environment. My generic channel is "fsl,i2s". I have four different systems booting off from a shared network drive. Each of these systems needs the common "fsl,i2s" driver but they all four need different fabric drivers.

Are you saying that you want to use the same kernel on four different systems? If so, then you need to find a way to compile all fabric drivers together, and at boot time each fabric driver will decide whether it will do anything.

On 1/2/08, Timur Tabi <[email protected]> wrote:
mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

Quoted 12 lines · Jon Smirl

On 1/2/08, Timur Tabi <[email protected]> wrote:
mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

Quoted 2 lines

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

The driver can always get the root node. But better yet, instantiate the correct fabric device (probably as a platform_device) from the platform code. Then the correct fabric driver can probe against it.

Quoted 16 lines · Grant Likely

Quoted 14 lines · Jon Smirl

On 1/2/08, Timur Tabi <[email protected]> wrote:
mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

Quoted 3 lines

The driver can always get the root node. But better yet, instantiate the correct fabric device (probably as a platform_device) from the platform code. Then the correct fabric driver can probe against it.

The meaning of this has finally sunk into my consciousness. The platform code can create a device that isn't bound to a driver. So why not make this an of_platform_device? This is basically a pseudo device that isn't in the device tree.

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

Quoted 6 lines

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

Yes, but it requires the core ASoC code to not be a module. Then you can use machine_device_initcall() to register the device at a later time.

Quoted 10 lines · Grant Likely

Quoted 6 lines

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

Yes, but it requires the core ASoC code to not be a module. Then you can use machine_device_initcall() to register the device at a later time.

How about this for a simpler solution? My mpc5200-psc-ac97 and mpc5200-psc-i2c drivers can create a device on the ASOC bus named after the first entry in the compatible field of the root node. That will cause the correct driver to get activated. I'm in the process of making ASOC drivers dynamically loadable like the i2c ones.

Quoted 12 lines · Jon Smirl

Quoted 10 lines · Grant Likely

Quoted 6 lines

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

Yes, but it requires the core ASoC code to not be a module. Then you can use machine_device_initcall() to register the device at a later time.

Quoted 5 lines

How about this for a simpler solution? My mpc5200-psc-ac97 and mpc5200-psc-i2c drivers can create a device on the ASOC bus named after the first entry in the compatible field of the root node. That will cause the correct driver to get activated. I'm in the process of making ASOC drivers dynamically loadable like the i2c ones.

I little icky, but it doesn't sound dangerous (as in shouldn't cause any name conflicts). That may be the best we can do for the time being. But I don't think it is a good idea for the long term.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

Cheers,
g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195

Quoted 13 lines · Grant Likely

Quoted 12 lines · Jon Smirl

On 1/2/08, Timur Tabi <[email protected]> wrote:
mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

Quoted 6 lines

Quoted 2 lines

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

The driver can always get the root node. But better yet, instantiate the correct fabric device (probably as a platform_device) from the platform code. Then the correct fabric driver can probe against it.

The meaning of this has finally sunk into my consciousness. The platform code can create a device that isn't bound to a driver. So why not make this an of_platform_device? This is basically a pseudo device that isn't in the device tree.

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

In the longer term I'd like to kill platform_bus on powerpc and only use of_platform_bus. Platform_bus seems to be functioning like a catch-all and collecting junk from lots of different platforms.

Quoted 33 lines · Jon Smirl

Quoted 13 lines · Grant Likely

Quoted 12 lines · Jon Smirl

On 1/2/08, Timur Tabi <[email protected]> wrote:
mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
        {
                .compatible     = "fsl,MPC8610HPCD",
        },
        {},
};

Quoted 6 lines

Quoted 2 lines

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

The driver can always get the root node. But better yet, instantiate the correct fabric device (probably as a platform_device) from the platform code. Then the correct fabric driver can probe against it.

The meaning of this has finally sunk into my consciousness. The platform code can create a device that isn't bound to a driver. So why not make this an of_platform_device? This is basically a pseudo device that isn't in the device tree.

Alternatively, the best place for this device would be on the ASOC bus, but the ASOC bus hasn't been created when the platform code runs. Maybe I can figure out a place in the platform code to create this device after the ASOC driver has loaded and created the bus. Does the platform code get control back after loading all of the device drivers?

Quoted 3 lines

In the longer term I'd like to kill platform_bus on powerpc and only use of_platform_bus. Platform_bus seems to be functioning like a catch-all and collecting junk from lots of different platforms.

Not going to happen. of_platform_bus is not the right solution, and in fact we're looking at moving (gradually) away from using of_platform_bus, and instead using platform devices (along with the device node being available for *any* struct device via the arch_sysdata).

--

David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!

http://www.ozlabs.org/~dgibson

For this model to work you need to split your driver. fsl-ssi and mpc8610_hpcd need to be in two separate drivers.

fsl-ssi is easy
enough to load since it has a device tree entry.

mpc8610_hpcd is the harder one to load since it doesn't have a device tree entry. What you want to do it match on the compatible field of the root node.

static struct of_device_id fabric_of_match[] = {
	{
		.compatible	= "fsl,MPC8610HPCD",
	},
	{},
};

But this doesn't work since the root is the device tree isn't passed down into the device probe code. (Could this be fixed?)

Instead we could make the separated mpc8610_hpcd fabric driver attach to fsl,ssi.

static struct of_device_id fabric_of_match[] = {
	{
		.compatible	= "fsl,ssi",
	},
	{},
};

Then in it's probe code check for the right platform.

That's the best plan I came up with. This is apparently fixed in ASoC V2. From ASoC V1's perspective, the fabric driver must be the master. However, it doesn't make sense to have a node in the device tree for the fabric driver, because there is no such "device". The fabric driver is an abstraction. So I need to chose some other node to probe the fabric driver with. I chose the SSI, since each SSI can have only one codec.

Quoted 4 lines

But that doesn't work in my environment. My generic channel is "fsl,i2s". I have four different systems booting off from a shared network drive. Each of these systems needs the common "fsl,i2s" driver but they all four need different fabric drivers.

That, I don't understand. fsl,ssi is pretty much the same thing as fsl,i2s, since the SSI *is* an I2S device. It's also an AC97 device, which is why I added the fsl,mode property.

The fabric driver is specific to the board. So you should be using Kconfig to select the fabric driver. There is no node in the device tree for fabric drivers. I thought that was the consensus.

Are you saying that you want to use the same kernel on four different systems? If so, then you need to find a way to compile all fabric drivers together, and at boot time each fabric driver will decide whether it will do anything.

Quoted 4 lines · Timur Tabi

However, it doesn't make sense to have a node in the device tree for the fabric driver, because there is no such "device". The fabric driver is an abstraction. So I need to chose some other node to probe the fabric driver with. I chose the SSI, since each SSI can have only one codec.

Does that mean with ASoC V2 you can instantiate it with the board specific platform code instead? I think that is the consensus we were leaning towards in the last discussion about this issue.

The machine support code (fabric driver in PowerPC terms, I think?) tells the core how everything is connected together by registering devices representing the links (eg, I2S) between the codecs, CPU and other devices. The ASoC core is then responsible for ensuring that all the required components are present before it registers with the ALSA core.

Quoted 6 lines · Mark Brown

The machine support code (fabric driver in PowerPC terms, I think?) tells the core how everything is connected together by registering devices representing the links (eg, I2S) between the codecs, CPU and other devices. The ASoC core is then responsible for ensuring that all the required components are present before it registers with the ALSA core.

I'm no expert on this, but I think from the PowerPC point-of-view, the *ideal* situation would be if the ASoC fabric driver were generic, maybe even part of ASoC itself, and everything it needed could be obtained from the device tree.

Quoted 4 lines · Timur Tabi

I'm no expert on this, but I think from the PowerPC point-of-view, the *ideal* situation would be if the ASoC fabric driver were generic, maybe even part of ASoC itself, and everything it needed could be obtained from the device tree.

Nice idea in principle, and may be the way to go ultimately, but very tricky in practice. The whole reason the fabric driver concept exists (from other archs) is that there are an awful lot of variants on how to wire the sound components together. Devising a way of expressing those connections in the device tree that's sufficient will be very curly. Then we'd have to build the fabric driver that can parse and process them all.

And then, people will no doubt produce device trees
with errors in the connection information, so we'll still need platform-specific workarounds.

If we want sound working any time soon, we'll want to stick with the platform based fabric drivers for the time being.

Does that mean with ASoC V2 you can instantiate it with the board specific platform code instead?

This is one of the examples of where the compatible properties are trying to be far to generic about what they are. How do you define what "fsl,ssi" is?

What happens when Freescale produces another peripheral that can do ssi but isn't register level compatible?

In my opinion, it is far better to be specific in the device tree and teach the driver about what versions it is able to bind against. In this case, I would use "fsl,mpc8610-ssi" or maybe better yet:
"fsl,mpc8610-ssi,i2s" (MPC8610 SSI device in I2S mode).

I don't like the idea of a separate fsl,mode property to describe the behaviour of multifunction peripherals. It makes probing more difficult when there is a different driver for each mode.

Quoted 3 lines

The fabric driver is specific to the board. So you should be using Kconfig to select the fabric driver. There is no node in the device tree for fabric drivers. I thought that was the consensus.

No, the desire is to go multiplatform in ppc. That means you cannot use Kconfig to select the correct fabric driver.

Quoted 2 lines · Grant Likely

Does that mean with ASoC V2 you can instantiate it with the board specific platform code instead?

I don't know. I haven't really looked at V2 yet. You'll have to ask Liam Girdwood.

Quoted 3 lines

This is one of the examples of where the compatible properties are trying to be far to generic about what they are. How do you define what "fsl,ssi" is?

The SSI is a specific Freescale device, so I think it's pretty well defined.

Quoted 2 lines

What happens when Freescale produces another peripheral that can do ssi but isn't register level compatible?

It won't be called the SSI. It will be called something else.

Quoted 4 lines

In my opinion, it is far better to be specific in the device tree and teach the driver about what versions it is able to bind against. In this case, I would use "fsl,mpc8610-ssi" or maybe better yet:
"fsl,mpc8610-ssi,i2s" (MPC8610 SSI device in I2S mode).

I can work with that, but the SSI could be placed into any future 83xx, 85xx, or 86xx SOC, and the driver will still work with it as-is.

Quoted 4 lines

I don't like the idea of a separate fsl,mode property to describe the behaviour of multifunction peripherals. It makes probing more difficult when there is a different driver for each mode.

Can you propose an alternative? The driver needs to know what mode to use when communicating with its codec. How am I supposed to know if I have an I2S codec or an AC97 codec?

Make the compatible property tell you! :-) If it's connected to an I2S codec, then it could be compatible = "fsl,mpc8610-ssi,i2s". Or for AC7, compatible = "fsl,mpc8610-ssi,ac97"

Quoted 3 lines · Grant Likely

Make the compatible property tell you! :-) If it's connected to an I2S codec, then it could be compatible = "fsl,mpc8610-ssi,i2s". Or for AC7, compatible = "fsl,mpc8610-ssi,ac97"

That won't work. There are too many variations. I think a separate property just makes more sense. Frankly, I don't see what's wrong with it.

Quoted 21 lines · Jon Smirl

Quoted 2 lines · Timur Tabi

 sound/soc/fsl/fsl_ssi.c                      |  614 +++++++++++++++++++
 sound/soc/fsl/fsl_ssi.h                      |  224 +++++++

I'm confused about this part. You built a driver for the mpc8610 ssi port. This port has a device tree entry.

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+		ssi@16000 {
+			compatible = "fsl,ssi";
+			cell-index = <0>;
+			reg = <16000 100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <3e 2>;
+			fsl,mode = "i2s-slave";
+			codec {
+				compatible = "cirrus,cs4270";
+				/* MCLK source is a stand-alone oscillator */
+				bus-frequency = <bb8000>;
+			};
+		};

Quoted 4 lines

But then you don't create an of_platform_driver for this device. Instead you create one for the fabric driver, struct of_platform_driver mpc8610_hpcd_of_driver, and directly link the SSI driver into it.

That's the best plan I came up with. This is apparently fixed in ASoC V2. From ASoC V1's perspective, the fabric driver must be the master. However, it doesn't make sense to have a node in the device tree for the fabric driver, because there is no such "device". The fabric driver is an abstraction. So I need to chose some other node to probe the fabric driver with. I chose the SSI, since each SSI can have only one codec.

Quoted 27 lines · Timur Tabi

Quoted 21 lines · Jon Smirl

Quoted 2 lines · Timur Tabi

 sound/soc/fsl/fsl_ssi.c                      |  614 +++++++++++++++++++
 sound/soc/fsl/fsl_ssi.h                      |  224 +++++++

I'm confused about this part. You built a driver for the mpc8610 ssi port. This port has a device tree entry.

arch/powerpc/boot/dts/mpc8610_hpcd.dts +13 -0

+           ssi@16000 {
+                   compatible = "fsl,ssi";
+                   cell-index = <0>;
+                   reg = <16000 100>;
+                   interrupt-parent = <&mpic>;
+                   interrupts = <3e 2>;
+                   fsl,mode = "i2s-slave";
+                   codec {
+                           compatible = "cirrus,cs4270";
+                           /* MCLK source is a stand-alone oscillator */
+                           bus-frequency = <bb8000>;
+                   };
+           };

Quoted 4 lines

But then you don't create an of_platform_driver for this device. Instead you create one for the fabric driver, struct of_platform_driver mpc8610_hpcd_of_driver, and directly link the SSI driver into it.

Quoted 7 lines

That's the best plan I came up with. This is apparently fixed in ASoC V2. From ASoC V1's perspective, the fabric driver must be the master. However, it doesn't make sense to have a node in the device tree for the fabric driver, because there is no such "device". The fabric driver is an abstraction. So I need to chose some other node to probe the fabric driver with. I chose the SSI, since each SSI can have only one codec.

Instantiating the fabric driver off any node is wrong, precisely because it is an abstraction. The fabric driver should be instantiated by the platform code.

--

David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!

http://www.ozlabs.org/~dgibson

On 1/2/08, David Gibson <[email protected]> wrote:

Quoted 3 lines

Instantiating the fabric driver off any node is wrong, precisely because it is an abstraction. The fabric driver should be instantiated by the platform code.

Instantiating it from the platform code forces me to put it either the of_platform_bus or the platform_bus since there aren't any other buses around when the platform code runs. Platform bus doesn't implement dynamic module loading. So that means it has to go onto the of_platform_bus. That implies that is it a pseudo-device without a pseudo-device entry in the device tree which is fine with me. I'll need to poke around in the of_bus code and see if the driver will load without a device tree entry.

A simple fix to this would be to let me instantiate the driver off from the root node of the tree. That's the conceptually correct place for instantiating a driver that extends the platform code. Should I try adjusting the of probing code to pass the node in, or are there major objections?

Also, as others have pointed out, this driver is not an abstraction. It represents the mess of wires hooking the codec up to the jacks on the back panel and possibly GPIO pins that control the wiring. You need this because the pins on HD audio codecs are completely reconfigurable and the same chip can be wired in a thousand different ways. It lets you have a generic codec driver and the move the platform specific code out of the driver.

Instantiating the fabric driver off any node is wrong, precisely because it is an abstraction. The fabric driver should be instantiated by the platform code.