Xlinx ML403 AC97 Controller Reference device driver

sound/ppc/ml403_ac97cr.c new file +0 -0

sound/ppc/ml403_ac97cr.c new file +11 -0

+static int
+snd_ml403_ac97cr_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_ml403_ac97cr *ml403_ac97cr;
+	int err = 0;
+
+	ml403_ac97cr = snd_pcm_substream_chip(substream);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		if (substream == ml403_ac97cr->playback_substream) {

+struct snd_pcm_ops snd_ml403_ac97cr_playback_ops = {

+irqreturn_t snd_ml403_ac97cr_irq(int irq, void *dev_id, struct pt_regs *regs)

sound/ppc/ml403_ac97cr.c new file +2 -0

+static unsigned short
+snd_ml403_ac97cr_codec_read(struct snd_ac97 *ac97, unsigned short reg)

sound/ppc/ml403_ac97cr.c new file +2 -0

+	/* if we are here, we _have_ to access the codec really, no faking */
+	spin_lock(&ml403_ac97cr->reg_lock);

sound/ppc/ml403_ac97cr.c new file +1 -0

+	do {

sound/ppc/pcm-indirect2.h new file +0 -0

(snip)

sound/ppc/ml403_ac97cr.c line 61

Quoted 3 lines

sound/ppc/ml403_ac97cr.c new file +3 -0

+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
+static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
+static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;

Can the driver really support multiple instances?
If not, better to avoid these arrays.

sound/ppc/ml403_ac97cr.c line 160

Quoted 4 lines

sound/ppc/ml403_ac97cr.c new file +4 -0

+struct lm4550_reg lm4550_regfile[64] = {
+	{.reg = 0x00,
+	 .flag = LM4550_REG_NOSAVE | LM4550_REG_FAKEREAD,
+	 .def = 0x0D50},

Better to use C99 style initialization here, e.g.

	[0x00] = { .... },
	[0x02] = { .... },
	[0x7e] = { .... },

so you can avoid writing empty items.
The index value could be also AC97_XXX, such as [AC97_MASTER] = {...}.

What is the purpose of reg field at all, BTW? I guess it's superfluous.

Quoted 1 line

+#define LM4550_RF_FLAG(reg) lm4550_regfile[reg / 2].flag

sound/ppc/ml403_ac97cr.c line 265

Quoted 6 lines

sound/ppc/ml403_ac97cr.c new file +6 -0

+static void lm4550_regfile_init(void)
+{
+	int i;
+	for (i = 0; i < 128; i = i + 2)
+		if (LM4550_RF_FLAG(i) & LM4550_REG_FAKEPROBE)
+			LM4550_RF_VAL(i) = LM4550_RF_DEF(i);

"MACRO(x) = XXX" looks a bit strange to me.

RATE_CONTINUOUS and RATE_KNOW are usually exclusive.

Hi Takashi,

before posting a corrected version, I would like to ask some unclear things (I think I understood the rest):

Quoted 7 lines · Takashi Iwai

(snip)

sound/ppc/ml403_ac97cr.c line 61

Quoted 3 lines

sound/ppc/ml403_ac97cr.c new file +3 -0

+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
+static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
+static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;

Can the driver really support multiple instances?
If not, better to avoid these arrays.

Well, while I wrote the driver I considered that there might be more than one instance - though I didn't test it and I won't be able to test it (no such hardware available - with more than one LM4550 chip). So, shall I remove it?

Quoted 15 lines

sound/ppc/ml403_ac97cr.c line 160

Quoted 4 lines

sound/ppc/ml403_ac97cr.c new file +4 -0

+struct lm4550_reg lm4550_regfile[64] = {
+	{.reg = 0x00,
+	 .flag = LM4550_REG_NOSAVE | LM4550_REG_FAKEREAD,
+	 .def = 0x0D50},

Better to use C99 style initialization here, e.g.

	[0x00] = { .... },
	[0x02] = { .... },
	[0x7e] = { .... },

so you can avoid writing empty items.
The index value could be also AC97_XXX, such as [AC97_MASTER] = {...}.

Quoted 2 lines

What is the purpose of reg field at all, BTW? I guess it's superfluous.

No, it's there to provide a shadow copy of the codec's (LM4550) registers.

It contains default values and (while driver is running) current values, which were written to the hardware. I had to introduce this, because Xilinx's AC97 Controller Reference has a very ugly bug: It provides a "register access finish" bit, so the driver is able to tell when a register read or write access is finished. Unfortunately this particular bit only works in the range of 0 to 100 times since board reset. After that many register access it just remains saying: I'm _not_ ready. But in fact, in most cases (98%) the correct value already is the read register (assume we have just said to the control that we want to read a register).
I thought, ok we have such RegAccessFinished bit, so use it, if we have to, until it doesn't work anymore.
So, through a shadow copy of most registers (some cannot be shadow or it makes no sence) I can provide the values without having to actually read from the controller/codec. The regfile also contains info which register might be shadowed, if values get saved at all (if written) ... Furthermore ALSA's AC97 layer does heavy initialization access series on the codec, which I tried to "mask out" completely (LM4550_REG_FAKEPROBE).

Quoted 1 line

Quoted 1 line

+#define LM4550_RF_FLAG(reg) lm4550_regfile[reg / 2].flag

<snip>

Quoted 8 lines

sound/ppc/ml403_ac97cr.c line 265

Quoted 6 lines

sound/ppc/ml403_ac97cr.c new file +6 -0

+static void lm4550_regfile_init(void)
+{
+	int i;
+	for (i = 0; i < 128; i = i + 2)
+		if (LM4550_RF_FLAG(i) & LM4550_REG_FAKEPROBE)
+			LM4550_RF_VAL(i) = LM4550_RF_DEF(i);

"MACRO(x) = XXX" looks a bit strange to me.

Hmmm, ok. I thought about that, too. I think, I'll spell them out?

Quoted 1 line

RATE_CONTINUOUS and RATE_KNOW are usually exclusive.

Ok, so what I want is RATE_CONTINUOUS, right? (because the LM4550 supports 4000 to 48000kHz in 1Hz steps)
BTW: What is RATE_KNOT good for?

sound/ppc/ml403_ac97cr.c line 1044

Quoted 3 lines

sound/ppc/ml403_ac97cr.c new file +3 -0

+static int __init
+snd_ml403_ac97cr_create(struct snd_card *card, struct platform_device *pfdev,
+			struct snd_ml403_ac97cr **rml403_ac97cr)

It's no longer __init as long as you use platform_device. It should be __devinit instead.

Quoted 2 lines

sound/ppc/pcm-indirect2.h new file +0 -0

(snip)

sound/ppc/pcm-indirect2.h line 117

Quoted 3 lines

sound/ppc/pcm-indirect2.h new file +3 -0

+#ifdef SND_PCM_INDIRECT2_STAT
+static inline void snd_pcm_indirect2_stat(struct snd_pcm_substream *substream,
+					  struct snd_pcm_indirect2 *rec)

Remove inline from the functions in this file. They are too lengthy.

sound/pcm-indirect.h contain inline functions becuase they are relatively small, and I didn't want to add them in the core module unconditionally.

Hi Takashi,

again some question came up (while correcting all the other issues):

Quoted 6 lines · Takashi Iwai

sound/ppc/ml403_ac97cr.c line 1044

Quoted 3 lines

sound/ppc/ml403_ac97cr.c new file +3 -0

+static int __init
+snd_ml403_ac97cr_create(struct snd_card *card, struct platform_device *pfdev,
+			struct snd_ml403_ac97cr **rml403_ac97cr)

It's no longer __init as long as you use platform_device. It should be __devinit instead.

Ok, I changed that (for create(), pcm(), mixer(), etc.). Do I have to change it for the module_init and module_exit functions, too? I guess, they are not in the "scope" of platform device, right? So, they will keep __init / __exit ?

Quoted 9 lines

Quoted 2 lines

sound/ppc/pcm-indirect2.h new file +0 -0

(snip)

sound/ppc/pcm-indirect2.h line 117

Quoted 3 lines

sound/ppc/pcm-indirect2.h new file +3 -0

+#ifdef SND_PCM_INDIRECT2_STAT
+static inline void snd_pcm_indirect2_stat(struct snd_pcm_substream *substream,
+					  struct snd_pcm_indirect2 *rec)

Remove inline from the functions in this file. They are too lengthy.

Quoted 3 lines

sound/pcm-indirect.h contain inline functions becuase they are relatively small, and I didn't want to add them in the core module unconditionally.

I looked at my functions again. And I think we could still go with inline for the *_interrupt() and *_pointer() functions since they have just a few lines.
With both *_transfer() functions - I don't know. In fact they shouldn't be much longer than yours in pcm-indirect.h . They seem long, because there are lot of comments and the #ifdef SND_PCM_INDIRECT2_STAT stuff, which is/was a debugging feature, which _won't_ be compiled in without CONFIG_SND_DEBUG.

[I removed inline from the _stat() and _min_periods() function - that's true, they are too long.]

The other thing I thought about, is: Is it ok to have non-inline functions in a header file? Don't we need a .c file + .h ? (Are there any CodingStyle rules about that?)

From: Joachim Foerster <[email protected]>

Add ALSA support for the opb_ac97_controller_ref_v1_00_a ip core found in Xilinx' ML403 reference design.

Known issue: Currently this driver hits a WARN_ON_ONCE(1) statement in kernel/irq/resend.c (line 70). According to Linus
(http://lkml.org/lkml/2007/8/5/5) this may be ignored, right? I haven't had a look into this "problem" yet.

(Patch for Linus' master branch, date 2007/08/08)

This patchset _will_ be published on
http://www.esic-solutions.com/support.html soon (like the first version of the driver (tar file), but this may take some days ...).

sound/ppc/Makefile +1 -0

 # Toplevel Module Dependency
 obj-$(CONFIG_SND_POWERMAC)     += snd-powermac.o
 obj-$(CONFIG_SND_PS3)          += snd_ps3.o
+obj-$(CONFIG_SND_ML403_AC97CR) += snd-ml403_ac97cr.o

sound/ppc/ml403_ac97cr.c new file +47 -0

new file mode 100644
index 0000000..99791d7
@@ -0,0 +1,1274 @@
+
+/*  ALSA driver for Xilinx ML403 AC97 Controller Reference
+ *    IP: opb_ac97_controller_ref_v1_00_a (EDK 8.1i)
+ *    IP: opb_ac97_controller_ref_v1_00_a (EDK 9.1i)
+ *
+ *  Copyright (c) by 2007  Joachim Foerster <[email protected]>
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#include <sound/driver.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
+
+#include <linux/platform_device.h>
+
+#include <linux/ioport.h>
+#include <asm/io.h>
+#include <linux/interrupt.h>
+
+/* HZ */
+#include <linux/param.h>
+/* jiffies, time_*() */
+#include <linux/jiffies.h>
+/* schedule_timeout*() */
+#include <linux/sched.h>
+/* spin_lock*() */
+#include <linux/spinlock.h>
+
+/* snd_printk(), snd_printd() */
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/initval.h>

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +0 -0

index eacee2d..827f2f5 100644

Couldn't this end up on MicroBlaze too? If so, sound/ppc is the wrong place.

I second grant here... this should eventually work on microblaze, too,
so putting it drivers/ and using XILINX_DRIVERS instead of XILINX_VIRTEX (based on the patch Wolfgang recently posted) would probably be preferable.

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +4 -0

+
+config SND_ML403_AC97CR
+       tristate "Xilinx ML403 AC97 Controller Reference"
+       depends on SND && XILINX_VIRTEX

Steve

Quoted 1 line

+snd-ml403_ac97cr-objs := ml403_ac97cr.o

This line is only needed if you're compiling multiple .c files into one .ko

I'll try adding your patches to my tree this afternoon.

Hi Grant,

Quoted 6 lines · Grant Likely

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +0 -0

index eacee2d..827f2f5 100644

Couldn't this end up on MicroBlaze too? If so, sound/ppc is the wrong place.

Well, I don't know. The only arch depended calls I use (am aware of) are in/out_be32(), so this _might_ work for mb, too. (I won't be able to test that in the next few months ...)

Quoted 10 lines · Stephen Neuendorffer

I second grant here... this should eventually work on microblaze, too,
so putting it drivers/ and using XILINX_DRIVERS instead of XILINX_VIRTEX (based on the patch Wolfgang recently posted) would probably be preferable.

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +4 -0

+
+config SND_ML403_AC97CR
+       tristate "Xilinx ML403 AC97 Controller Reference"
+       depends on SND && XILINX_VIRTEX

Steve

So, like Stephen Neuendorffer said, I should move it to sound/drivers, right? Takashi?

Quoted 3 lines · Grant Likely

Quoted 1 line

+snd-ml403_ac97cr-objs := ml403_ac97cr.o

This line is only needed if you're compiling multiple .c files into one .ko

Ah, well, then I have to rename the main .c file. But all the other ALSA drivers seem to _not_ include the "snd-" in front of it (ok, except the new PS3 driver and maybe other new ones?)

Quoted 1 line

I'll try adding your patches to my tree this afternoon.

I made patch against your tree, too (like a said before). Tell me, then I post it ...

Joachim

Hi Grant,

Quoted 6 lines · Grant Likely

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +0 -0

index eacee2d..827f2f5 100644

Couldn't this end up on MicroBlaze too? If so, sound/ppc is the wrong place.

Well, I don't know. The only arch depended calls I use (am aware of) are in/out_be32(), so this _might_ work for mb, too. (I won't be able to test that in the next few months ...)

Quoted 10 lines · Stephen Neuendorffer

I second grant here... this should eventually work on microblaze, too,
so putting it drivers/ and using XILINX_DRIVERS instead of XILINX_VIRTEX (based on the patch Wolfgang recently posted) would probably be preferable.

sound/ppc/Makefile

Quoted 4 lines

sound/ppc/Makefile +4 -0

+
+config SND_ML403_AC97CR
+       tristate "Xilinx ML403 AC97 Controller Reference"
+       depends on SND && XILINX_VIRTEX

Steve

So, like Stephen Neuendorffer said, I should move it to sound/drivers, right? Takashi?

Quoted 3 lines

Quoted 1 line

+snd-ml403_ac97cr-objs := ml403_ac97cr.o

This line is only needed if you're compiling multiple .c files into one .ko

Ah, well, then I have to rename the main .c file. But all the other ALSA drivers seem to _not_ include the "snd-" in front of it (ok, except the new PS3 driver and maybe other new ones?)