[alsa-devel] [PATCH] alsa: lx6464es - driver for the digigram lx6464es interface

[Takashi Iwai]

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 at klingt.org>

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