At Mon, 23 Mar 2009 11:12:22 +0100,
Tim Blechmann wrote:
>
> prototype of a driver for the digigram lx6464es 64 channel ethersound
> interface.
>
> Signed-off-by: Tim Blechmann <tim@xxxxxxxxxx>
Thanks.
First off, the current code looks a bit different from the standard
kernel coding style. Please run $LINUX/scripts/checkpatch.pl and
fix the issues reported there.
> diff --git a/sound/pci/lx6464es/LXES_registers.h b/sound/pci/lx6464es/LXES_registers.h
> new file mode 100644
> index 0000000..f665084
> --- /dev/null
> +++ b/sound/pci/lx6464es/LXES_registers.h
> @@ -0,0 +1,122 @@
> +/**
The comments with "/**" in the kernel codes are used for kernel-doc
in general. Unless you want to process it via kernel-doc, avoid it.
> +* Copyright (C) 2006 DIGIGRAM S.A.
Hrm, is it the copy from Digigram's source? Then we'd need their
sign-off as well...
> +//*********************************************************************************
> +// $history:$
> +//
Better to remove unneeded parts like this.
> diff --git a/sound/pci/lx6464es/Makefile b/sound/pci/lx6464es/Makefile
> new file mode 100644
> index 0000000..a23257c
> --- /dev/null
> +++ b/sound/pci/lx6464es/Makefile
> @@ -0,0 +1,13 @@
> +ifeq ($(KERNELRELEASE),)
> +snd-lx6464es-objs := lx6464es.o lx_core.o
> +obj-m += snd-lx6464es.o
> +all:
> + make -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules
> +
> +clean:
> + make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean
> +
> +else
> +snd-lx6464es-objs := lx6464es.o lx_core.o
> +obj-$(CONFIG_SND_LX6464ES) += snd-lx6464es.o
> +endif
Don't put it the stuff for custom builds.
> diff --git a/sound/pci/lx6464es/PcxErr_e.h b/sound/pci/lx6464es/PcxErr_e.h
> new file mode 100644
> index 0000000..dd48c46
> --- /dev/null
> +++ b/sound/pci/lx6464es/PcxErr_e.h
> +// Return value when OK
> +// ********************
> +
> +#define SUCCESS 0
Isn't it defined anywhere?
If it's an internal value, I'd suggest to put a prefix to avoid
confusion. No big issue, though.
> diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> new file mode 100644
> index 0000000..e1df693
> --- /dev/null
> +++ b/sound/pci/lx6464es/lx6464es.c
...
> +#ifndef PCI_DEVICE_ID_PLX_9056 /* LATER: this should go to pci_ids.h */
> +#define PCI_DEVICE_ID_PLX_9056 0x9056
> +#endif
You patch already includes the change in pci_ids.h :)
Remove this.
> +#define PCI_DEVICE_ID_PLX_LX6464ES PCI_DEVICE_ID_PLX_9056
> +
> +#ifndef PCI_VENDOR_ID_DIGIGRAM
> +#define PCI_VENDOR_ID_DIGIGRAM 0x1369
> +#endif
> +
> +#ifndef PCI_SUBDEVICE_ID_DIGIGRAM_LX6464ES_SERIAL_SUBSYSTEM
> +#define PCI_SUBDEVICE_ID_DIGIGRAM_LX6464ES_SERIAL_SUBSYSTEM 0xc001
> +#define PCI_SUBDEVICE_ID_DIGIGRAM_LX6464ES_CAE_SERIAL_SUBSYSTEM 0xc002
> +#endif
Ditto.
> +static int lx_proc_create(struct snd_card *card, struct lx6464es *chip)
Can be __devinit.
> +static int __devinit snd_lx6464es_create(struct snd_card *card,
> + struct pci_dev *pci,
> + struct lx6464es **rchip)
...
> + chip->port_mem = pci_resource_start(pci, 0);
> +
> + /* plx port */
> + chip->port_plx = pci_resource_start(pci, 1);
> + chip->port_plx_remapped = ioport_map(chip->port_plx,
> + pci_resource_len(pci, 1));
Better to use pci_ioremap_bar().
> + /* dsp port */
> + chip->port_dsp = pci_resource_start(pci, 2);
> + chip->port_dsp_resource = request_mem_region(chip->port_dsp,
> + pci_resource_len(pci, 2),
> + card_name);
Hm, isn't it already requested via pci_request_regions()?
> + chip->port_dsp_remapped = ioremap_nocache(chip->port_dsp,
> + pci_resource_len(pci, 2));
Use pci_ioremap_bar().
> + err = request_irq(pci->irq, lx_interrupt, IRQF_SHARED,
> + card_name, chip);
> + if (err) {
> + snd_printk(KERN_ERR LXP "unable to grab IRQ %d\n", pci->irq);
> + goto request_irq_failed;
> + }
> + chip->irq = pci->irq;
> +
> + err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> + if (err < 0)
> + return err;
This error path can be a leak.
> diff --git a/sound/pci/lx6464es/lx_core.h b/sound/pci/lx6464es/lx_core.h
> new file mode 100644
> index 0000000..901d443
> --- /dev/null
> +++ b/sound/pci/lx6464es/lx_core.h
> +static inline void unpack_pointer(dma_addr_t ptr, u32 *r_low, u32 *r_high)
> +{
> + *r_low = (u32)(ptr & 0xffffffff);
> +#if BITS_PER_LONG == 32
> + *r_high = 0;
> +#else
> + *r_high = (u32)((u64)ptr>>32);
> +#endif
Better to use upper_32_bits() macro.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
