At Sat, 16 Feb 2008 00:30:55 +0100, Cédric Brégardis wrote:
Hi,
We managed to write ALSA driver for Emagic Audiowerk 2 PCI sound card (based on Philips SAA7146 chip). You can find more information on https://gna.org/projects/aw2-alsa/.
We think it's time to integrate the sources into the alsa/kernel tree. So, here is the patch for this driver. We hope this patch is usable.
Regards,
Jean-Christian Hassler and Cedric Bregardis
Signed-off-by: Cedric Bregardis cedric.bregardis@free.fr Signed-off-by: Jean-Christian Hassler jhassler@free.fr
Thanks for the patch.
First of all, could you fix the issues reported by checkpatch.pl?
Below is a quick review.
--- /dev/null +++ b/sound/pci/aw2/aw2-alsa.c
(snip)
+#include <sound/driver.h>
sound/driver.h is deprecated now. Simply remove this line.
+struct aw2_pcm_device_t {
Usually *_t is used for typedef'ed types. Use names without _t for structs, i.e. "struct aw2_pcm_device".
+/* module parameters (see "Module Parameters") */
The comment (see "Module Parameters") isn't needed for the real codes. It's a comment that is valid only in my tutorial.
+/* pci_driver definition */ +static struct pci_driver driver = {
- .name = "My Own Chip",
Choose a better name :)
+/* component-destructor
- (see "Management of Cards and Components")
- */
Ditto about the comment.
+/* chip-specific constructor
- (see "Management of Cards and Components")
- */
Ditto.
+static int __devinit snd_aw2_create(struct snd_card *card,
struct pci_dev *pci, struct aw2_t **rchip)+{
(snip)
- chip = kcalloc(1, sizeof(*chip), GFP_KERNEL);
Use kzalloc.
- /* (1) PCI resource allocation */
- err = pci_request_regions(pci, "Audiowerk2");
- if (err < 0) {
kfree(chip);return err;- }
- chip->saa7146.iobase_phys = pci_resource_start(pci, 0);
- chip->saa7146.iobase_virt =
ioremap_nocache(chip->saa7146.iobase_phys,pci_resource_len(pci, 0));
Better to check whether ioremap succeeded or not.
+/* constructor -- see "Constructor" sub-section */
Fix the comment.
+/* destructor -- see "Destructor" sub-section */
Dtto.
+/* open callback */ +static int snd_aw2_pcm_playback_open(struct snd_pcm_substream *substream) +{
- struct snd_pcm_runtime *runtime = substream->runtime;
- printk(KERN_DEBUG "Playback_open \n");
Avoid unconditionally compiled debug messages. If you really want to keep such debug messages, use snd_printdd() or whatever that won't be compiled unless explicitly specified.
+/* create a pcm device */ +static int __devinit snd_aw2_new_pcm(struct aw2_t *chip) +{
- struct snd_pcm *pcm_playback_ana;
- struct snd_pcm *pcm_playback_num;
- struct snd_pcm *pcm_capture;
- int err = 0;
- /* Create new Alsa PCM device */
- err =
snd_pcm_new(chip->card, "Audiowerk2 analog playback", 0, 1, 0,&pcm_playback_ana);- if (err >= 0) {
It's easier to bail out when the error is returned.
- err = snd_pcm_new(chip->card, "Audiowerk2 capture", 2, 0, 1,
&pcm_capture);
Any reason to make all PCM streams non-duplex? Oh I see in the below...
+static int snd_aw2_control_switch_capture_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *uinfo)+{
- static char *texts[2] = {
"Analog", "Digital"- };
- uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
- uinfo->count = 1;
- uinfo->value.enumerated.items = 2;
- if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) {
uinfo->value.enumerated.item =uinfo->value.enumerated.items - 1;- }
- strcpy(uinfo->value.enumerated.name,
texts[uinfo->value.enumerated.item]);- return 0;
+}
So, you'd like to switch the input on the fly rather than selecting at open time? It's a question of the driver design...
--- /dev/null +++ b/sound/pci/aw2/aw2-saa7146.c
(snip)
+#define WRITEREG(value, addr) do { writel((value), chip->iobase_virt + (addr));\
mb(); } while (0)
Do you really need a barrier here?
+void snd_aw2_saa7146_dump_registers(struct snd_aw2_saa7146_t *chip) +{
- int i = 0;
- for (i = 0; i <= 0x148; i += 4) {
printk(KERN_DEBUG "###DUMP 0x%03x: 0x%08x\n", i, READREG(i));- }
+}
Better to use proc interface than printk for such a purpose.
+/* chip-specific destructor
- (see "PCI Resource Managements")
- */
+int snd_aw2_saa7146_free(struct snd_aw2_saa7146_t *chip) +{
- /* disable all irqs */
- WRITEREG(0, IER);
- /* reset saa7146 */
- WRITEREG((MRST_N << 16), MC1);
- /* release the irq */
- if (chip->irq >= 0) {
free_irq(chip->irq, (void *)chip);- }
- /* release the i/o ports & memory */
- if (chip->iobase_virt) {
iounmap(chip->iobase_virt);- }
- pci_release_regions(chip->pci);
- /* disable the PCI entry */
- pci_disable_device(chip->pci);
- /* release the data */
- kfree(chip);
- return 0;
+}
Put the destructor in the same place as the constructor. Don't put in separate files.
--- /dev/null +++ b/sound/pci/aw2/aw2-saa7146.h
(snip)
+#ifdef AW2_SAA7146_M +#define PUBLIC +#else +#define PUBLIC extern +#endif
Don't do this. It's too hackish.
thanks,
Takashi