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@xxxxxxx> > Signed-off-by: Jean-Christian Hassler <jhassler@xxxxxxx> 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
