[alsa-devel] [PATCH 1/2] Support internal I2S clock sources on MPC5200
Support internal I2S clock sources on MPC5200
Signed-off-by: Jon Smirl jonsmirl@gmail.com ---
sound/soc/fsl/mpc5200_psc_i2s.c | 58 ++++++++++++++++++++++++++++++++++----- sound/soc/fsl/mpc5200_psc_i2s.h | 13 +++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 sound/soc/fsl/mpc5200_psc_i2s.h
diff --git a/sound/soc/fsl/mpc5200_psc_i2s.c b/sound/soc/fsl/mpc5200_psc_i2s.c index 8692329..f028f61 100644 --- a/sound/soc/fsl/mpc5200_psc_i2s.c +++ b/sound/soc/fsl/mpc5200_psc_i2s.c @@ -23,8 +23,12 @@
#include <sysdev/bestcomm/bestcomm.h> #include <sysdev/bestcomm/gen_bd.h> +#include <asm/time.h> +#include <asm/mpc52xx.h> #include <asm/mpc52xx_psc.h>
+#include "mpc5200_psc_i2s.h" + MODULE_AUTHOR("Grant Likely grant.likely@secretlab.ca"); MODULE_DESCRIPTION("Freescale MPC5200 PSC in I2S mode ASoC Driver"); MODULE_LICENSE("GPL"); @@ -93,6 +97,7 @@ struct psc_i2s { struct snd_soc_dai dai; spinlock_t lock; u32 sicr; + uint sysclk;
/* per-stream data */ struct psc_i2s_stream playback; @@ -224,6 +229,7 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_i2s *psc_i2s = rtd->dai->cpu_dai->private_data; + uint bits, framesync, bitclk, value; u32 mode;
dev_dbg(psc_i2s->dev, "%s(substream=%p) p_size=%i p_bytes=%i" @@ -235,15 +241,19 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S8: mode = MPC52xx_PSC_SICR_SIM_CODEC_8; + bits = 8; break; case SNDRV_PCM_FORMAT_S16_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_16; + bits = 16; break; case SNDRV_PCM_FORMAT_S24_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_24; + bits = 24; break; case SNDRV_PCM_FORMAT_S32_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_32; + bits = 32; break; default: dev_dbg(psc_i2s->dev, "invalid format\n"); @@ -251,7 +261,24 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, } out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | mode);
- snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); + if (psc_i2s->sysclk) { + framesync = bits * 2; + bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync); + + /* bitclk field is byte swapped due to mpc5200/b compatibility */ + value = ((framesync - 1) << 24) | + (((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00); + + dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i" + " framesync=%i bitclk=%i reg=%X\n", + __FUNCTION__, substream, params_rate(params), psc_i2s->sysclk, + framesync, bitclk, value); + + out_be32(&psc_i2s->psc_regs->ccr, value); + out_8(&psc_i2s->psc_regs->ctur, bits - 1); + } + + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
return 0; } @@ -429,9 +456,29 @@ static int psc_i2s_set_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { struct psc_i2s *psc_i2s = cpu_dai->private_data; - dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, dir=%i)\n", - cpu_dai, dir); - return (dir == SND_SOC_CLOCK_IN) ? 0 : -EINVAL; + int clkdiv, err; + dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, freq=%u, dir=%i)\n", + cpu_dai, freq, dir); + if (dir == SND_SOC_CLOCK_OUT) { + psc_i2s->sysclk = freq; + if (clk_id == MPC52xx_CLK_CELLSLAVE) { + psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK; + } else { /* MPC52xx_CLK_INTERNAL */ + psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE; + psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK; + + clkdiv = ppc_proc_freq / freq; + err = ppc_proc_freq % freq; + if (err > freq / 2) + clkdiv++; + + dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n", + clkdiv, (ppc_proc_freq / clkdiv - freq)); + + return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv); + } + } + return 0; }
/** @@ -784,9 +831,6 @@ static int __devinit psc_i2s_of_probe(struct of_device *op, /* Configure the serial interface mode; defaulting to CODEC8 mode */ psc_i2s->sicr = MPC52xx_PSC_SICR_DTS1 | MPC52xx_PSC_SICR_I2S | MPC52xx_PSC_SICR_CLKPOL; - if (of_get_property(op->node, "fsl,cellslave", NULL)) - psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | - MPC52xx_PSC_SICR_GENCLK; out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | MPC52xx_PSC_SICR_SIM_CODEC_8);
diff --git a/sound/soc/fsl/mpc5200_psc_i2s.h b/sound/soc/fsl/mpc5200_psc_i2s.h new file mode 100644 index 0000000..0e0a84e --- /dev/null +++ b/sound/soc/fsl/mpc5200_psc_i2s.h @@ -0,0 +1,13 @@ +/* + * Freescale MPC5200 PSC in I2S mode + * ALSA SoC Digital Audio Interface (DAI) driver + * + */ + +#ifndef __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ +#define __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ + +#define MPC52xx_CLK_INTERNAL 0 +#define MPC52xx_CLK_CELLSLAVE 1 + +#endif /* __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ */
Allow a custom ASOC machine driver with soc-of-simple
Signed-off-by: Jon Smirl jonsmirl@gmail.com ---
include/sound/soc-of-simple.h | 2 ++ sound/soc/fsl/soc-of-simple.c | 26 +++++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc-of-simple.h b/include/sound/soc-of-simple.h index 696fc51..1e83f2f 100644 --- a/include/sound/soc-of-simple.h +++ b/include/sound/soc-of-simple.h @@ -18,4 +18,6 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, struct device_node *node, struct snd_soc_dai *cpu_dai);
+void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops); + #endif /* _INCLUDE_SOC_OF_H_ */ diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c index 0382fda..dd2fa23 100644 --- a/sound/soc/fsl/soc-of-simple.c +++ b/sound/soc/fsl/soc-of-simple.c @@ -38,8 +38,8 @@ struct of_snd_soc_device { struct device_node *codec_node; };
-static struct snd_soc_ops of_snd_soc_ops = { -}; +static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
static struct of_snd_soc_device * of_snd_soc_get_device(struct device_node *codec_node) @@ -61,7 +61,7 @@ of_snd_soc_get_device(struct device_node *codec_node) of_soc->machine.dai_link = &of_soc->dai_link; of_soc->machine.num_links = 1; of_soc->device.machine = &of_soc->machine; - of_soc->dai_link.ops = &of_snd_soc_ops; + of_soc->dai_link.ops = machine_ops; list_add(&of_soc->list, &of_snd_soc_device_list);
return of_soc; @@ -74,7 +74,7 @@ static void of_snd_soc_register_device(struct of_snd_soc_device *of_soc)
/* Only register the device if both the codec and platform have * been registered */ - if ((!of_soc->device.codec_data) || (!of_soc->platform_node)) + if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name) return;
pr_info("platform<-->codec match achieved; registering machine\n"); @@ -159,7 +159,7 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, of_soc->platform_node = node; of_soc->dai_link.cpu_dai = cpu_dai; of_soc->device.platform = platform; - of_soc->machine.name = of_soc->dai_link.cpu_dai->name; + of_soc->machine.name = machine_name;
/* Now try to register the SoC device */ of_snd_soc_register_device(of_soc); @@ -169,3 +169,19 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, return rc; } EXPORT_SYMBOL_GPL(of_snd_soc_register_platform); + +void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops) +{ + struct of_snd_soc_device *of_soc; + + machine_name = name; + machine_ops = ops; + + list_for_each_entry(of_soc, &of_snd_soc_device_list, list) { + of_soc->dai_link.ops = machine_ops; + of_soc->machine.name = machine_name; + of_snd_soc_register_device(of_soc); + } + +} +EXPORT_SYMBOL_GPL(of_snd_soc_register_machine);
Hi John,
On Tue, 22 Jul 2008 19:53:51 -0400 Jon Smirl jonsmirl@gmail.com wrote:
+static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
You don't need these initialisations.
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
As with the clocking configuration: this looks fine from an ASoC point of view but please fix the checkpatch warnings. However...
/* Only register the device if both the codec and platform have * been registered */
- if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
- if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name) return;
...this doesn't just allow a custom machine driver, it requires that something configures at least the machine name. That's not a problem from the ASoC point of view but possibly from the PowerPC side?
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
As with the clocking configuration: this looks fine from an ASoC point of view but please fix the checkpatch warnings. However...
/* Only register the device if both the codec and platform have * been registered */
if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name) return;...this doesn't just allow a custom machine driver, it requires that something configures at least the machine name. That's not a problem from the ASoC point of view but possibly from the PowerPC side?
You have to configure at least the name. Otherwise if the machine driver is the last to register, how do you know to hold off the final registration and wait for the machine driver to appear?
Or is it ok for me to change these after the platform device has been created? of_soc->dai_link.ops = machine_ops; of_soc->machine.name = machine_name;
I have to have the machine driver in order to control my external clock chips.
On Wed, Jul 23, 2008 at 10:09:01AM -0400, Jon Smirl wrote:
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
...this doesn't just allow a custom machine driver, it requires that something configures at least the machine name. That's not a problem from the ASoC point of view but possibly from the PowerPC side?
You have to configure at least the name. Otherwise if the machine driver is the last to register, how do you know to hold off the final registration and wait for the machine driver to appear?
I understand why you have made this change but it's a substantial change which should at least be documented in the changelog (I'd expect to see some mention of how this is supposed to be configured, for example). I'd also expect something to handle the existing user.
Like I said, I'm not entirely sure that you're supposed to be using this driver if you want a machine driver: this is a machine driver and I'm not sure if it's supposed to cover all cases or not. Grant?
Or is it ok for me to change these after the platform device has been created? of_soc->dai_link.ops = machine_ops; of_soc->machine.name = machine_name;
No.
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Jul 23, 2008 at 10:09:01AM -0400, Jon Smirl wrote:
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
...this doesn't just allow a custom machine driver, it requires that something configures at least the machine name. That's not a problem from the ASoC point of view but possibly from the PowerPC side?
You have to configure at least the name. Otherwise if the machine driver is the last to register, how do you know to hold off the final registration and wait for the machine driver to appear?
I understand why you have made this change but it's a substantial change which should at least be documented in the changelog (I'd expect to see some mention of how this is supposed to be configured, for example). I'd also expect something to handle the existing user.
This is a modification to Grant's new driver. Grant is the only other user.
Like I said, I'm not entirely sure that you're supposed to be using this driver if you want a machine driver: this is a machine driver and I'm not sure if it's supposed to cover all cases or not. Grant?
Or is it ok for me to change these after the platform device has been created? of_soc->dai_link.ops = machine_ops; of_soc->machine.name = machine_name;
No.
On Wed, Jul 23, 2008 at 11:16:17AM -0400, Jon Smirl wrote:
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
I'd also expect something to handle the existing user.
This is a modification to Grant's new driver. Grant is the only other user.
Right, hence my use of the singular : ) . Since you are adding this new of_snd_soc_register_machine() Grant's existing platform won't be calling it yet and will therefore stop instantiating the ASoC subsystem until it is changed to do so.
On Wed, Jul 23, 2008 at 04:14:32PM +0100, Mark Brown wrote:
On Wed, Jul 23, 2008 at 10:09:01AM -0400, Jon Smirl wrote:
On 7/23/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
...this doesn't just allow a custom machine driver, it requires that something configures at least the machine name. That's not a problem from the ASoC point of view but possibly from the PowerPC side?
You have to configure at least the name. Otherwise if the machine driver is the last to register, how do you know to hold off the final registration and wait for the machine driver to appear?
I understand why you have made this change but it's a substantial change which should at least be documented in the changelog (I'd expect to see some mention of how this is supposed to be configured, for example). I'd also expect something to handle the existing user.
Like I said, I'm not entirely sure that you're supposed to be using this driver if you want a machine driver: this is a machine driver and I'm not sure if it's supposed to cover all cases or not. Grant?
As I replied to the patch itself, I'm not thrilled with it, but I don't have an immediate solution. I do want this behaviour to at least trigger on something in the device tree; not to become a hard requirement.
g.
Hi i want to fill dummy "struct snd_dma_buffer" in which i want to pass base address of allocated BUFFER DESCRIPTOR(BD). help me.
Regards, Dinesh dua
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
Signed-off-by: Jon Smirl jonsmirl@gmail.com
Sorry I didn't respond earlier. OLS kept me pretty distracted.
Need a more detailed comment block about how your changing things and why.
include/sound/soc-of-simple.h | 2 ++ sound/soc/fsl/soc-of-simple.c | 26 +++++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc-of-simple.h b/include/sound/soc-of-simple.h index 696fc51..1e83f2f 100644 --- a/include/sound/soc-of-simple.h +++ b/include/sound/soc-of-simple.h @@ -18,4 +18,6 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, struct device_node *node, struct snd_soc_dai *cpu_dai);
+void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops);
#endif /* _INCLUDE_SOC_OF_H_ */ diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c index 0382fda..dd2fa23 100644 --- a/sound/soc/fsl/soc-of-simple.c +++ b/sound/soc/fsl/soc-of-simple.c @@ -38,8 +38,8 @@ struct of_snd_soc_device { struct device_node *codec_node; };
-static struct snd_soc_ops of_snd_soc_ops = { -}; +static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
Doing this prevents multiple instances of this machine driver (which is exactly what I have on my board). To register a machine driver it creates a 3-way bind condition instead of the existing 2-way one. Right now it is easy to match up codec and platform drivers because a common key is available in the device tree.
Alternately, it might be okay to only allow for a single machine driver that is able to create multiple sound card instances, but this current code just uses the same name and ops for each registered device.
static struct of_snd_soc_device * of_snd_soc_get_device(struct device_node *codec_node) @@ -61,7 +61,7 @@ of_snd_soc_get_device(struct device_node *codec_node) of_soc->machine.dai_link = &of_soc->dai_link; of_soc->machine.num_links = 1; of_soc->device.machine = &of_soc->machine;
- of_soc->dai_link.ops = &of_snd_soc_ops;
of_soc->dai_link.ops = machine_ops; list_add(&of_soc->list, &of_snd_soc_device_list);
return of_soc;
@@ -74,7 +74,7 @@ static void of_snd_soc_register_device(struct of_snd_soc_device *of_soc)
/* Only register the device if both the codec and platform have * been registered */
- if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
- if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name)
I'm not thrilled with the hard requirement for a machine driver, but I see what you're trying to do. I want to find a clean way to trigger this behaviour in the device tree without resorting to encoding linux internal details into the data. Need to think about this more.
return;pr_info("platform<-->codec match achieved; registering machine\n"); @@ -159,7 +159,7 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, of_soc->platform_node = node; of_soc->dai_link.cpu_dai = cpu_dai; of_soc->device.platform = platform;
- of_soc->machine.name = of_soc->dai_link.cpu_dai->name;
- of_soc->machine.name = machine_name;
As mentioned above, either there needs to be multiple machine drivers or the ability to change the name for each platform--codec pair.
/* Now try to register the SoC device */ of_snd_soc_register_device(of_soc); @@ -169,3 +169,19 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, return rc; } EXPORT_SYMBOL_GPL(of_snd_soc_register_platform);
+void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops) +{
- struct of_snd_soc_device *of_soc;
- machine_name = name;
- machine_ops = ops;
- list_for_each_entry(of_soc, &of_snd_soc_device_list, list) {
of_soc->dai_link.ops = machine_ops;of_soc->machine.name = machine_name;of_snd_soc_register_device(of_soc);- }
+}
You need to hold the mutex when manipulating the list.
On 7/26/08, Grant Likely grant.likely@secretlab.ca wrote:
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
Signed-off-by: Jon Smirl jonsmirl@gmail.com
Sorry I didn't respond earlier. OLS kept me pretty distracted.
Need a more detailed comment block about how your changing things and why.
include/sound/soc-of-simple.h | 2 ++ sound/soc/fsl/soc-of-simple.c | 26 +++++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc-of-simple.h b/include/sound/soc-of-simple.h index 696fc51..1e83f2f 100644 --- a/include/sound/soc-of-simple.h +++ b/include/sound/soc-of-simple.h @@ -18,4 +18,6 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, struct device_node *node, struct snd_soc_dai *cpu_dai);
+void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops);
#endif /* _INCLUDE_SOC_OF_H_ */ diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c index 0382fda..dd2fa23 100644 --- a/sound/soc/fsl/soc-of-simple.c +++ b/sound/soc/fsl/soc-of-simple.c @@ -38,8 +38,8 @@ struct of_snd_soc_device { struct device_node *codec_node; };
-static struct snd_soc_ops of_snd_soc_ops = { -}; +static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
Doing this prevents multiple instances of this machine driver (which is exactly what I have on my board). To register a machine driver it creates a 3-way bind condition instead of the existing 2-way one. Right now it is easy to match up codec and platform drivers because a common key is available in the device tree.
Alternately, it might be okay to only allow for a single machine driver that is able to create multiple sound card instances, but this current code just uses the same name and ops for each registered device.
We need to call them fabric drivers since we already have machine drivers in arch/.... It is too confusing.
My fabric driver was getting probed after the rest of ASOC bound, that's why I had to add the third condition.
An important feature for me is the ability to compile in multiple fabric drivers and then get the right one selected based on the machine name in the device tree. I'm doing via my machine driver.
Could we dynamically build an OF fabric device entry with a compatible string like this: compatible="machinename-fabric,generic-fabric" Now a hard requirement for a fabric driver is ok since one of those two will load for sure. generic-fabric is just a do nothing driver that is always built in.
/* Trigger the platform specific ASOC driver to load */ static struct platform_device platform = { .name = "dspeak01-fabric", .id = -1, };
static struct platform_device *devices[] = { &platform, };
static void __init digispeaker_declare_platform_devices(void) { mpc52xx_declare_of_platform_devices(); platform_add_devices(&devices[0], ARRAY_SIZE(devices)); }
define_machine(digispeaker_platform) { .name = "dspeak01", .probe = digispeaker_probe, .setup_arch = digispeaker_setup_arch, .init = digispeaker_declare_platform_devices, .init_IRQ = mpc52xx_init_irq, .get_irq = mpc52xx_get_irq, .restart = mpc52xx_restart, .calibrate_decr = generic_calibrate_decr, };
static struct of_snd_soc_device * of_snd_soc_get_device(struct device_node *codec_node) @@ -61,7 +61,7 @@ of_snd_soc_get_device(struct device_node *codec_node) of_soc->machine.dai_link = &of_soc->dai_link; of_soc->machine.num_links = 1; of_soc->device.machine = &of_soc->machine;
of_soc->dai_link.ops = &of_snd_soc_ops;
of_soc->dai_link.ops = machine_ops; list_add(&of_soc->list, &of_snd_soc_device_list); return of_soc;@@ -74,7 +74,7 @@ static void of_snd_soc_register_device(struct of_snd_soc_device *of_soc)
/* Only register the device if both the codec and platform have * been registered */
if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name)I'm not thrilled with the hard requirement for a machine driver, but I see what you're trying to do. I want to find a clean way to trigger this behaviour in the device tree without resorting to encoding linux internal details into the data. Need to think about this more.
return; pr_info("platform<-->codec match achieved; registering machine\n");@@ -159,7 +159,7 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, of_soc->platform_node = node; of_soc->dai_link.cpu_dai = cpu_dai; of_soc->device.platform = platform;
of_soc->machine.name = of_soc->dai_link.cpu_dai->name;
of_soc->machine.name = machine_name;As mentioned above, either there needs to be multiple machine drivers or the ability to change the name for each platform--codec pair.
/* Now try to register the SoC device */ of_snd_soc_register_device(of_soc);@@ -169,3 +169,19 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform, return rc; } EXPORT_SYMBOL_GPL(of_snd_soc_register_platform);
+void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops) +{
struct of_snd_soc_device *of_soc;machine_name = name;machine_ops = ops;list_for_each_entry(of_soc, &of_snd_soc_device_list, list) {of_soc->dai_link.ops = machine_ops;of_soc->machine.name = machine_name;of_snd_soc_register_device(of_soc);}+}
You need to hold the mutex when manipulating the list.
On Sun, Jul 27, 2008 at 12:44 AM, Jon Smirl jonsmirl@gmail.com wrote:
On 7/26/08, Grant Likely grant.likely@secretlab.ca wrote:
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
Signed-off-by: Jon Smirl jonsmirl@gmail.com diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c index 0382fda..dd2fa23 100644 --- a/sound/soc/fsl/soc-of-simple.c +++ b/sound/soc/fsl/soc-of-simple.c @@ -38,8 +38,8 @@ struct of_snd_soc_device { struct device_node *codec_node; };
-static struct snd_soc_ops of_snd_soc_ops = { -}; +static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
Doing this prevents multiple instances of this machine driver (which is exactly what I have on my board). To register a machine driver it creates a 3-way bind condition instead of the existing 2-way one. Right now it is easy to match up codec and platform drivers because a common key is available in the device tree.
Alternately, it might be okay to only allow for a single machine driver that is able to create multiple sound card instances, but this current code just uses the same name and ops for each registered device.
We need to call them fabric drivers since we already have machine drivers in arch/.... It is too confusing.
heh, even worse; we've already got platform drivers under drivers/. :-P I disagree. We need to be consistent with ALSA terminology, but I will be more diligent to call them ASoC Machine drivers.
My fabric driver was getting probed after the rest of ASOC bound, that's why I had to add the third condition.
An important feature for me is the ability to compile in multiple fabric drivers and then get the right one selected based on the machine name in the device tree.
Absolutely; this is a hard requirement as far as I'm concerned.
It is also a hard requirement for either multiple *active* ASoC machine drivers, or support ASoC machine drivers that control multiple ASoC device instances.
I'm doing via my machine driver.
Could we dynamically build an OF fabric device entry with a compatible string like this: compatible="machinename-fabric,generic-fabric" Now a hard requirement for a fabric driver is ok since one of those two will load for sure. generic-fabric is just a do nothing driver that is always built in.
Ugh; unfortunately that results in Linux internal details getting leaked out to the device tree. Not a good idea; especially when we *know* this driver is a stop gap measure until v2 changes things on us.
But that still doesn't help the question of how to trigger loading of the board specific machine drivers. grumble. But I do think that the sanest approach is still to trigger on the top level model property in the tree.
Oh well, if I need to add a dummy machine driver that matches on my board, then that's what I'll do. It certainly sidesteps all the ugly heuristics of figuring out when custom code is needed.
/* Trigger the platform specific ASOC driver to load */ static struct platform_device platform = { .name = "dspeak01-fabric", .id = -1, };
static struct platform_device *devices[] = { &platform, };
static void __init digispeaker_declare_platform_devices(void) { mpc52xx_declare_of_platform_devices(); platform_add_devices(&devices[0], ARRAY_SIZE(devices)); }
I'm not convinced that all this is necessary. I'm partial to just having the ASoC machine module probe routine check the top level board property and registering the ASoC machine driver if it is a match. That also eliminates needing to build knowledge of the sound circuit into the PPC platform driver which helps stem the tide of proliferating platform files.
g.
On 7/27/08, Grant Likely grant.likely@secretlab.ca wrote:
On Sun, Jul 27, 2008 at 12:44 AM, Jon Smirl jonsmirl@gmail.com wrote:
On 7/26/08, Grant Likely grant.likely@secretlab.ca wrote:
On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
Allow a custom ASOC machine driver with soc-of-simple
Signed-off-by: Jon Smirl jonsmirl@gmail.com
diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c index 0382fda..dd2fa23 100644 --- a/sound/soc/fsl/soc-of-simple.c +++ b/sound/soc/fsl/soc-of-simple.c @@ -38,8 +38,8 @@ struct of_snd_soc_device { struct device_node *codec_node; };
-static struct snd_soc_ops of_snd_soc_ops = { -}; +static struct snd_soc_ops *machine_ops = NULL; +static char *machine_name = NULL;
Doing this prevents multiple instances of this machine driver (which is exactly what I have on my board). To register a machine driver it creates a 3-way bind condition instead of the existing 2-way one. Right now it is easy to match up codec and platform drivers because a common key is available in the device tree.
Alternately, it might be okay to only allow for a single machine driver that is able to create multiple sound card instances, but this current code just uses the same name and ops for each registered device.
We need to call them fabric drivers since we already have machine drivers in arch/.... It is too confusing.
heh, even worse; we've already got platform drivers under drivers/. :-P I disagree. We need to be consistent with ALSA terminology, but I will be more diligent to call them ASoC Machine drivers.
My fabric driver was getting probed after the rest of ASOC bound, that's why I had to add the third condition.
An important feature for me is the ability to compile in multiple fabric drivers and then get the right one selected based on the machine name in the device tree.
Absolutely; this is a hard requirement as far as I'm concerned.
It is also a hard requirement for either multiple *active* ASoC machine drivers, or support ASoC machine drivers that control multiple ASoC device instances.
I'm doing via my machine driver.
Could we dynamically build an OF fabric device entry with a compatible string like this: compatible="machinename-fabric,generic-fabric" Now a hard requirement for a fabric driver is ok since one of those two will load for sure. generic-fabric is just a do nothing driver that is always built in.
Ugh; unfortunately that results in Linux internal details getting leaked out to the device tree. Not a good idea; especially when we *know* this driver is a stop gap measure until v2 changes things on us.
It has been pointed out several times that the sound fabric (wires, external chips) really is a device so why can't it have a device tree entry?
ASOCv2 still has this same problem of triggering the load of the fabric driver.
The Linuxism being exposed here is the split between the machine driver and the fabric driver. Machine drivers are dynamically selected by the device tree. The device tree model is assuming these are combined into a single unit.
Simulating the merge of these two two into a single unit is why I added the platform device creation code for my fabric driver to my machine driver. But then we run in to the problem that Linux doesn't support device drivers that override each other. There is no way to have a dspeak01-fabric override a generic-fabric.
One solution to the override problem would be to make an overridable callout in the machine driver that always creates a generic-fabric platform device. I would then set a bit in my machine driver which would cause the name to change from generic-fabric to dspeak01-fabric.
Another Linuxism involved here is creating devices without knowing when or if a driver is ever going to show up. You hit this when making the ASOC devices, say we only created a dspeak01-fabric platform device and it hasn't been probed yet. How do you know if it is safe to assume a generic-fabric? Is dspeak01-fabric coming and it just hasn't been probed yet, or is it never coming at all?
But that still doesn't help the question of how to trigger loading of the board specific machine drivers. grumble. But I do think that the sanest approach is still to trigger on the top level model property in the tree.
Oh well, if I need to add a dummy machine driver that matches on my board, then that's what I'll do. It certainly sidesteps all the ugly heuristics of figuring out when custom code is needed.
/* Trigger the platform specific ASOC driver to load */ static struct platform_device platform = { .name = "dspeak01-fabric", .id = -1, };
static struct platform_device *devices[] = { &platform, };
static void __init digispeaker_declare_platform_devices(void) { mpc52xx_declare_of_platform_devices(); platform_add_devices(&devices[0], ARRAY_SIZE(devices)); }
I'm not convinced that all this is necessary. I'm partial to just having the ASoC machine module probe routine check the top level board property and registering the ASoC machine driver if it is a match. That also eliminates needing to build knowledge of the sound circuit into the PPC platform driver which helps stem the tide of proliferating platform files.
g.
-- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
On Tue, Jul 22, 2008 at 07:53:49PM -0400, Jon Smirl wrote:
Support internal I2S clock sources on MPC5200
Looks good from an ASoC point of view. There's quite a few checkpatch warnings that should be fixed (all fairly straightforward, though) but other than that I'm happy to apply it with Grant's ack.
On Tue, Jul 22, 2008 at 07:53:49PM -0400, Jon Smirl wrote:
Support internal I2S clock sources on MPC5200
Signed-off-by: Jon Smirl jonsmirl@gmail.com
I'll play with this when I get home. In the mean time I've got some comments below. Overall, it looks right to me.
g.
sound/soc/fsl/mpc5200_psc_i2s.c | 58 ++++++++++++++++++++++++++++++++++----- sound/soc/fsl/mpc5200_psc_i2s.h | 13 +++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 sound/soc/fsl/mpc5200_psc_i2s.h
diff --git a/sound/soc/fsl/mpc5200_psc_i2s.c b/sound/soc/fsl/mpc5200_psc_i2s.c index 8692329..f028f61 100644 --- a/sound/soc/fsl/mpc5200_psc_i2s.c +++ b/sound/soc/fsl/mpc5200_psc_i2s.c @@ -23,8 +23,12 @@
#include <sysdev/bestcomm/bestcomm.h> #include <sysdev/bestcomm/gen_bd.h> +#include <asm/time.h> +#include <asm/mpc52xx.h> #include <asm/mpc52xx_psc.h>
+#include "mpc5200_psc_i2s.h"
MODULE_AUTHOR("Grant Likely grant.likely@secretlab.ca"); MODULE_DESCRIPTION("Freescale MPC5200 PSC in I2S mode ASoC Driver"); MODULE_LICENSE("GPL"); @@ -93,6 +97,7 @@ struct psc_i2s { struct snd_soc_dai dai; spinlock_t lock; u32 sicr;
uint sysclk;
/* per-stream data */ struct psc_i2s_stream playback;
@@ -224,6 +229,7 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_i2s *psc_i2s = rtd->dai->cpu_dai->private_data;
uint bits, framesync, bitclk, value; u32 mode;
dev_dbg(psc_i2s->dev, "%s(substream=%p) p_size=%i p_bytes=%i"
@@ -235,15 +241,19 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S8: mode = MPC52xx_PSC_SICR_SIM_CODEC_8;
bits = 8;bits = 16;bits = 24;bits = 32;@@ -251,7 +261,24 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, } out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | mode);
- snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- if (psc_i2s->sysclk) {
framesync = bits * 2;bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync);/* bitclk field is byte swapped due to mpc5200/b compatibility */value = ((framesync - 1) << 24) |(((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00);dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i"" framesync=%i bitclk=%i reg=%X\n",__FUNCTION__, substream, params_rate(params), psc_i2s->sysclk,framesync, bitclk, value);out_be32(&psc_i2s->psc_regs->ccr, value);out_8(&psc_i2s->psc_regs->ctur, bits - 1);- }
This should probably only be executed if the psc is the clock master; just like you've done in psc_i2s_set_sysclk()
snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
return 0;
} @@ -429,9 +456,29 @@ static int psc_i2s_set_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { struct psc_i2s *psc_i2s = cpu_dai->private_data;
- dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, dir=%i)\n",
cpu_dai, dir);- return (dir == SND_SOC_CLOCK_IN) ? 0 : -EINVAL;
- int clkdiv, err;
- dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, freq=%u, dir=%i)\n",
cpu_dai, freq, dir);- if (dir == SND_SOC_CLOCK_OUT) {
psc_i2s->sysclk = freq;if (clk_id == MPC52xx_CLK_CELLSLAVE) {psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK;} else { /* MPC52xx_CLK_INTERNAL */psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE;psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK;clkdiv = ppc_proc_freq / freq;err = ppc_proc_freq % freq;if (err > freq / 2)clkdiv++;dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n",clkdiv, (ppc_proc_freq / clkdiv - freq));return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv);}- }
- return 0;
}
/** @@ -784,9 +831,6 @@ static int __devinit psc_i2s_of_probe(struct of_device *op, /* Configure the serial interface mode; defaulting to CODEC8 mode */ psc_i2s->sicr = MPC52xx_PSC_SICR_DTS1 | MPC52xx_PSC_SICR_I2S | MPC52xx_PSC_SICR_CLKPOL;
- if (of_get_property(op->node, "fsl,cellslave", NULL))
psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE |MPC52xx_PSC_SICR_GENCLK;diff --git a/sound/soc/fsl/mpc5200_psc_i2s.h b/sound/soc/fsl/mpc5200_psc_i2s.h new file mode 100644 index 0000000..0e0a84e --- /dev/null +++ b/sound/soc/fsl/mpc5200_psc_i2s.h @@ -0,0 +1,13 @@ +/*
- Freescale MPC5200 PSC in I2S mode
- ALSA SoC Digital Audio Interface (DAI) driver
- */
+#ifndef __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ +#define __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
+#define MPC52xx_CLK_INTERNAL 0 +#define MPC52xx_CLK_CELLSLAVE 1
+#endif /* __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ */
Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
On 7/26/08, Grant Likely grant.likely@secretlab.ca wrote:
On Tue, Jul 22, 2008 at 07:53:49PM -0400, Jon Smirl wrote:
Support internal I2S clock sources on MPC5200
Signed-off-by: Jon Smirl jonsmirl@gmail.com
I'll play with this when I get home. In the mean time I've got some comments below. Overall, it looks right to me.
g.
sound/soc/fsl/mpc5200_psc_i2s.c | 58 ++++++++++++++++++++++++++++++++++----- sound/soc/fsl/mpc5200_psc_i2s.h | 13 +++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 sound/soc/fsl/mpc5200_psc_i2s.h
diff --git a/sound/soc/fsl/mpc5200_psc_i2s.c b/sound/soc/fsl/mpc5200_psc_i2s.c index 8692329..f028f61 100644 --- a/sound/soc/fsl/mpc5200_psc_i2s.c +++ b/sound/soc/fsl/mpc5200_psc_i2s.c @@ -23,8 +23,12 @@
#include <sysdev/bestcomm/bestcomm.h> #include <sysdev/bestcomm/gen_bd.h> +#include <asm/time.h> +#include <asm/mpc52xx.h> #include <asm/mpc52xx_psc.h>
+#include "mpc5200_psc_i2s.h"
MODULE_AUTHOR("Grant Likely grant.likely@secretlab.ca"); MODULE_DESCRIPTION("Freescale MPC5200 PSC in I2S mode ASoC Driver"); MODULE_LICENSE("GPL"); @@ -93,6 +97,7 @@ struct psc_i2s { struct snd_soc_dai dai; spinlock_t lock; u32 sicr;
uint sysclk; /* per-stream data */ struct psc_i2s_stream playback;@@ -224,6 +229,7 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_i2s *psc_i2s = rtd->dai->cpu_dai->private_data;
uint bits, framesync, bitclk, value; u32 mode; dev_dbg(psc_i2s->dev, "%s(substream=%p) p_size=%i p_bytes=%i"@@ -235,15 +241,19 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S8: mode = MPC52xx_PSC_SICR_SIM_CODEC_8;
bits = 8; break; case SNDRV_PCM_FORMAT_S16_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_16;bits = 16; break; case SNDRV_PCM_FORMAT_S24_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_24;bits = 24; break; case SNDRV_PCM_FORMAT_S32_BE: mode = MPC52xx_PSC_SICR_SIM_CODEC_32;bits = 32; break; default: dev_dbg(psc_i2s->dev, "invalid format\n");@@ -251,7 +261,24 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream, } out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | mode);
snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
if (psc_i2s->sysclk) {framesync = bits * 2;bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync);/* bitclk field is byte swapped due to mpc5200/b compatibility */value = ((framesync - 1) << 24) |(((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00);dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i"" framesync=%i bitclk=%i reg=%X\n",__FUNCTION__, substream, params_rate(params), psc_i2s->sysclk,framesync, bitclk, value);out_be32(&psc_i2s->psc_regs->ccr, value);out_8(&psc_i2s->psc_regs->ctur, bits - 1);}This should probably only be executed if the psc is the clock master; just like you've done in psc_i2s_set_sysclk()
psc_i2s->sysclk can only get set if there is a clock master. Note that there are two ways to be a clock master, internal and cellslave.
i2s_set_sysclk() is dynamically called from my fabric driver to adjust the timebase to match the music.
snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); return 0;} @@ -429,9 +456,29 @@ static int psc_i2s_set_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { struct psc_i2s *psc_i2s = cpu_dai->private_data;
dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, dir=%i)\n",cpu_dai, dir);return (dir == SND_SOC_CLOCK_IN) ? 0 : -EINVAL;
int clkdiv, err;dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, freq=%u, dir=%i)\n",cpu_dai, freq, dir);if (dir == SND_SOC_CLOCK_OUT) {psc_i2s->sysclk = freq;if (clk_id == MPC52xx_CLK_CELLSLAVE) {psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK;} else { /* MPC52xx_CLK_INTERNAL */psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE;psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK;clkdiv = ppc_proc_freq / freq;err = ppc_proc_freq % freq;if (err > freq / 2)clkdiv++;dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n",clkdiv, (ppc_proc_freq / clkdiv - freq));return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv);}}return 0;}
/** @@ -784,9 +831,6 @@ static int __devinit psc_i2s_of_probe(struct of_device *op, /* Configure the serial interface mode; defaulting to CODEC8 mode */ psc_i2s->sicr = MPC52xx_PSC_SICR_DTS1 | MPC52xx_PSC_SICR_I2S | MPC52xx_PSC_SICR_CLKPOL;
if (of_get_property(op->node, "fsl,cellslave", NULL))psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE |MPC52xx_PSC_SICR_GENCLK; out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | MPC52xx_PSC_SICR_SIM_CODEC_8);diff --git a/sound/soc/fsl/mpc5200_psc_i2s.h b/sound/soc/fsl/mpc5200_psc_i2s.h new file mode 100644 index 0000000..0e0a84e --- /dev/null +++ b/sound/soc/fsl/mpc5200_psc_i2s.h @@ -0,0 +1,13 @@ +/*
- Freescale MPC5200 PSC in I2S mode
- ALSA SoC Digital Audio Interface (DAI) driver
- */
+#ifndef __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ +#define __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
+#define MPC52xx_CLK_INTERNAL 0 +#define MPC52xx_CLK_CELLSLAVE 1
+#endif /* __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ */
Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
participants (5)
-
dinesh -
Grant Likely -
Jon Smirl -
Mark Brown -
Stephen Rothwell