Re: [alsa-devel] [PATCH v2] Add M2Tech hiFace USB-SPDIF driver

28 Apr 2013

On Fri, 22 Feb 2013 17:12:36 +0100
Daniel Mack zonque@gmail.com wrote:
...
Hi Antonio,
a couple of more things that I stumbled over.
Thanks, some comments inlined below.
...
On 13.02.2013 18:11, Antonio Ospite wrote:
...
Add driver for M2Tech hiFace USB-SPDIF interface and compatible devices.
M2Tech hiFace and compatible devices offer a Hi-End S/PDIF Output
Interface, see http://www.m2tech.biz/hiface.html
The supported products are:

M2Tech Young
M2Tech hiFace
M2Tech North Star
M2Tech W4S Young
M2Tech Corrson
M2Tech AUDIA
M2Tech SL Audio
M2Tech Empirical
M2Tech Rockna
M2Tech Pathos
M2Tech Metronome
M2Tech CAD
M2Tech Audio Esclusive
M2Tech Rotel
M2Tech Eeaudio
The Chord Company CHORD
AVA Group A/S Vitus

Signed-off-by: Antonio Ospite ao2@amarulasolutions.com
Changes since v1:

Change the first sentence of the Kconfig entry into "Select this..."
Remove a useless sentence from the Kconfig entry
Don't set alsa_rt->hw.rates in hiface_pcm_open()
Style cleanup, no braces needed in single statement conditional
Remove the rate field from pcm_runtime
Use the hiFace name with the lowercase 'h' everywhere
List actually supported devices in MODULE_SUPPORTED_DEVICE()
Cosmetics, align values in the rates array
Use an explicit switch instead of the rate_value array in
hiface_pcm_set_rate()
Use usb_make_path() when building card->longname
Use again the implicit mechanism to allocate the card private data
Downgrade a pr_info to pr_debug in hiface_chip_probe()
Make device_table const
Rename PCM_MAX_PACKET_SIZE to PCM_PACKET_SIZE
Rename MAX_BUFSIZE to PCM_BUFFER_SIZE
Cosmetics, align symbolic constant values
Add SNDRV_PCM_RATE_KNOT only when needed
Declare memcpy_swahw32() as static
Protect snd_pcm_stop() with snd_pcm_stream_lock_irq()
Make hiface_pcm_playback() not returning anything
Move the period elapsed check into hiface_pcm_playback()
Handle the case of failing URBs in hiface_pcm_out_urb_handler()
Fix a couple of checkpatch.pl issues

The incremental changes can be seen individually at
https://github.com/panicking/snd-usb-asyncaudio/commits/master
Commits from Feb. 10th and later.
Thanks,
   Antonio
sound/usb/Kconfig         |   31 +++
 sound/usb/Makefile        |    2 +-
 sound/usb/hiface/Makefile |    2 +
 sound/usb/hiface/chip.h   |   34 +++
 sound/usb/hiface/chip.c   |  329 +++++++++++++++++++++++
 sound/usb/hiface/pcm.h    |   24 ++
 sound/usb/hiface/pcm.c    |  638 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 1059 insertions(+), 1 deletion(-)
 create mode 100644 sound/usb/hiface/Makefile
 create mode 100644 sound/usb/hiface/chip.h
 create mode 100644 sound/usb/hiface/chip.c
 create mode 100644 sound/usb/hiface/pcm.h
 create mode 100644 sound/usb/hiface/pcm.c

diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
index 225dfd7..de9408b 100644
--- a/sound/usb/Kconfig
+++ b/sound/usb/Kconfig
@@ -115,5 +115,36 @@ config SND_USB_6FIRE
           and further help can be found at
           http://sixfireusb.sourceforge.net
+config SND_USB_HIFACE

   tristate "M2Tech hiFace USB-SPDIF driver"


   select SND_PCM


   help


 Select this option to include support for M2Tech hiFace USB-SPDIF


 interface.



 This driver supports the original M2Tech hiFace and some other


 compatible devices. The supported products are:



   * M2Tech Young


   * M2Tech hiFace


   * M2Tech North Star


   * M2Tech W4S Young


   * M2Tech Corrson


   * M2Tech AUDIA


   * M2Tech SL Audio


   * M2Tech Empirical


   * M2Tech Rockna


   * M2Tech Pathos


   * M2Tech Metronome


   * M2Tech CAD


   * M2Tech Audio Esclusive


   * M2Tech Rotel


   * M2Tech Eeaudio


   * The Chord Company CHORD


   * AVA Group A/S Vitus



 To compile this driver as a module, choose M here: the module


 will be called snd-usb-hiface.




endif	# SND_USB
diff --git a/sound/usb/Makefile b/sound/usb/Makefile
index ac256dc..abe668f 100644
--- a/sound/usb/Makefile
+++ b/sound/usb/Makefile
@@ -23,4 +23,4 @@ obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o
 obj-$(CONFIG_SND_USB_USX2Y) += snd-usbmidi-lib.o
 obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/
+obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/
diff --git a/sound/usb/hiface/Makefile b/sound/usb/hiface/Makefile
new file mode 100644
index 0000000..463b136
--- /dev/null
+++ b/sound/usb/hiface/Makefile
@@ -0,0 +1,2 @@
+snd-usb-hiface-objs := chip.o pcm.o
+obj-$(CONFIG_SND_USB_HIFACE) += snd-usb-hiface.o
diff --git a/sound/usb/hiface/chip.h b/sound/usb/hiface/chip.h
new file mode 100644
index 0000000..cd615a0
--- /dev/null
+++ b/sound/usb/hiface/chip.h
@@ -0,0 +1,34 @@
+/*


Linux driver for M2Tech hiFace compatible devices







Copyright 2012-2013 (C) M2TECH S.r.l and Amarula Solutions B.V.







Authors:  Michael Trimarchi michael@amarulasolutions.com



      Antonio Ospite <ao2@amarulasolutions.com>









The driver is based on the work done in TerraTec DMX 6Fire USB







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.


*/


+#ifndef HIFACE_CHIP_H
+#define HIFACE_CHIP_H



+#include <linux/usb.h>
+#include <sound/core.h>



+struct pcm_runtime;



+struct hiface_chip {

struct usb_device *dev;
struct snd_card *card;
int intf_count; /* number of registered interfaces */
int index; /* index in module parameter arrays */
bool shutdown;

struct pcm_runtime *pcm;

+};
+#endif /* HIFACE_CHIP_H */
diff --git a/sound/usb/hiface/chip.c b/sound/usb/hiface/chip.c
new file mode 100644
index 0000000..f1293a5
--- /dev/null
+++ b/sound/usb/hiface/chip.c
@@ -0,0 +1,329 @@
+/*


Linux driver for M2Tech hiFace compatible devices







Copyright 2012-2013 (C) M2TECH S.r.l and Amarula Solutions B.V.







Authors:  Michael Trimarchi michael@amarulasolutions.com



      Antonio Ospite <ao2@amarulasolutions.com>









The driver is based on the work done in TerraTec DMX 6Fire USB







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.


*/


+#include <linux/module.h>
+#include <linux/slab.h>
+#include <sound/initval.h>



+#include "chip.h"
+#include "pcm.h"



+MODULE_AUTHOR("Michael Trimarchi michael@amarulasolutions.com");
+MODULE_AUTHOR("Antonio Ospite ao2@amarulasolutions.com");
+MODULE_DESCRIPTION("M2Tech hiFace USB-SPDIF audio driver");
+MODULE_LICENSE("GPL v2");
+MODULE_SUPPORTED_DEVICE("{{M2Tech,Young},"

	 "{M2Tech,hiFace},"


	 "{M2Tech,North Star},"


	 "{M2Tech,W4S Young},"


	 "{M2Tech,Corrson},"


	 "{M2Tech,AUDIA},"


	 "{M2Tech,SL Audio},"


	 "{M2Tech,Empirical},"


	 "{M2Tech,Rockna},"


	 "{M2Tech,Pathos},"


	 "{M2Tech,Metronome},"


	 "{M2Tech,CAD},"


	 "{M2Tech,Audio Esclusive},"


	 "{M2Tech,Rotel},"


	 "{M2Tech,Eeaudio},"


	 "{The Chord Company,CHORD},"


	 "{AVA Group A/S,Vitus}}");




+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; /* Index 0-max */
+static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; /* Id for card */
+static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP; /* Enable this card */



+#define DRIVER_NAME "snd-usb-hiface"
+#define CARD_NAME "hiFace"



+module_param_array(index, int, NULL, 0444);
+MODULE_PARM_DESC(index, "Index value for " CARD_NAME " soundcard.");
+module_param_array(id, charp, NULL, 0444);
+MODULE_PARM_DESC(id, "ID string for " CARD_NAME " soundcard.");
+module_param_array(enable, bool, NULL, 0444);
+MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");



+static struct hiface_chip *chips[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;



+static DEFINE_MUTEX(register_mutex);



+struct hiface_vendor_quirk {

const char *device_name;
u8 extra_freq;

+};



+static int hiface_chip_create(struct usb_device *device, int idx,

	      const struct hiface_vendor_quirk *quirk,


	      struct hiface_chip **rchip)



+{

struct snd_card *card = NULL;
struct hiface_chip *chip;
int ret;
int len;

*rchip = NULL;

/* if we are here, card can be registered in alsa. */
ret = snd_card_create(index[idx], id[idx], THIS_MODULE, sizeof(*chip), &card);
if (ret < 0) {
snd_printk(KERN_ERR "cannot create alsa card.\n");


return ret;


}

strcpy(card->driver, DRIVER_NAME);

if (quirk && quirk->device_name)
strcpy(card->shortname, quirk->device_name);


else
strcpy(card->shortname, "M2Tech generic audio");



Use strncpy() or strlcpy() please, even if you're sure.
Will do.
...
...


strlcat(card->longname, card->shortname, sizeof(card->longname));
len = strlcat(card->longname, " at ", sizeof(card->longname));
if (len < sizeof(card->longname))
usb_make_path(device, card->longname + len,


	      sizeof(card->longname) - len);



chip = card->private_data;
chip->dev = device;
chip->index = idx;
chip->card = card;

*rchip = chip;
return 0;

+}



+static int hiface_chip_probe(struct usb_interface *intf,

	     const struct usb_device_id *usb_id)



+{

const struct hiface_vendor_quirk *quirk = (struct hiface_vendor_quirk *)usb_id->driver_info;
int ret;
int i;
struct hiface_chip *chip;
struct usb_device *device = interface_to_usbdev(intf);

pr_debug("Probe " DRIVER_NAME " driver.\n");

As Clemens said, you should drop this.
OK, the USB enumeration messages are enough, I see.
...
...


ret = usb_set_interface(device, 0, 0);
if (ret != 0) {
snd_printk(KERN_ERR "can't set first interface for " CARD_NAME " device.\n");


return -EIO;


}

/* check whether the card is already registered */
chip = NULL;
mutex_lock(&register_mutex);
for (i = 0; i < SNDRV_CARDS; i++) {
if (chips[i] && chips[i]->dev == device) {


	if (chips[i]->shutdown) {


		snd_printk(KERN_ERR CARD_NAME " device is in the shutdown state, cannot create a card instance\n");


		ret = -ENODEV;


		goto err;


	}


	chip = chips[i];


	break;


}


}
if (!chip) {
/* it's a fresh one.


 * now look for an empty slot and create a new card instance


 */


for (i = 0; i < SNDRV_CARDS; i++)


	if (enable[i] && !chips[i]) {


		ret = hiface_chip_create(device, i, quirk,


					 &chip);


		if (ret < 0)


			goto err;



		snd_card_set_dev(chip->card, &intf->dev);


		break;


	}


if (!chip) {


	snd_printk(KERN_ERR "no available " CARD_NAME " audio device\n");


	ret = -ENODEV;


	goto err;


}


}

The generic driver does the above in order to group multiple interfaces
on one device into one snd_card. It's needed because it is probed on
interface level.
Your driver seems to operate on device-level only, so you can drop this.
For the first loop I agree, it's not strictly necessary to check if the
device was registered already if there is only one "sound card" per
device, but the second loop is still needed in some form in order to
respect the "enable" option. I'll simplify that part, thanks.
BTW looking at how caiaq/device.c does it, I noticed that the
snd_card_used[] array is only read but never written, unless I am
missing anything :)
...
...

ret = hiface_pcm_init(chip, quirk ? quirk->extra_freq : 0);
if (ret < 0)
goto err_chip_destroy;



ret = snd_card_register(chip->card);
if (ret < 0) {
snd_printk(KERN_ERR "cannot register " CARD_NAME " card\n");



Not sure, but I think it might be better to use dev_{err,info,dgb} here,
so the logging can be grouped to individual devices. Takashi, any
opinion on that?
Will do, thanks.
Regards,
   Antonio
-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

    