Antonio Ospite wrote:
I also noticed that many drivers under sound/usb/ have text in the Kconfig entry stating "Say Y here to include support for ..." but I only see "[N/m/?]" as options when I run "make oldconfig" maybe because of some dependency, I am not sure, should such text be removed in order to avoid confusion?
It allows only "m" for "module" because some dependencies already are modules.
+++ b/sound/usb/Kconfig +...
M2Tech hiFace and compatible devices offer a Hi-End S/PDIF OutputInterface, see http://www.m2tech.biz/hiface.html
This sentence is not necessary for someone to decide whether to enable this Kconfig option, so it doesn't belong here.
+++ b/sound/usb/hiface/chip.c ... +MODULE_SUPPORTED_DEVICE("{{HiFace, Evo}}");
MODULE_SUPPORTED_DEVICE isn't actually used at the moment, but if you use it, you should include all supported devices. Or is this a name for the entire family?
(But I guess the name is "Evo", not " Evo".)
- if (quirk && quirk->device_name)
strcpy(card->shortname, quirk->device_name);- else
strcpy(card->shortname, "M2Tech generic audio");
Don't the devices have their name in the device descriptor?
- sprintf(card->longname, "%s at %d:%d", card->shortname,
device->bus->busnum, device->devnum);
It is common to use usb_make_path() to show where the device is connected, but this is more or less meaningless either way. :)
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
If you pass sizeof(*chip) as the fourth parameter to snd_card_create(), it will be allocated and freed automatically together with the card (as card->private_data) so that you don't need to create a separate device.
+static int hiface_chip_probe(struct usb_interface *intf,
const struct usb_device_id *usb_id)+...
- pr_info("Probe " DRIVER_NAME " driver.\n");
This doesn't belong into the finished driver.
+static struct usb_device_id device_table[] = {
This can be made const.
+++ b/sound/usb/hiface/pcm.c +#define PCM_MAX_PACKET_SIZE 4096
Why "MAX" when the packets are never smaller?
+static struct snd_pcm_hw_constraint_list constraints_rates = {
This should be const.
+static int hiface_pcm_set_rate(struct pcm_runtime *rt, unsigned int rate) +{
- u8 rate_value[] = { 0x43, 0x4b, 0x42, 0x4a, 0x40, 0x48, 0x58, 0x68 };
static const
- ret = usb_control_msg(device, usb_sndctrlpipe(device, 0),
0xb0,USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_OTHER,rate_value[rt->rate], 0, NULL, 0, 100);
You must not do DMA from either the stack or from static module data, so you have to copy this byte into kmalloc'ed memory (like snd_usb_ctl_msg() does).
+void memcpy_swahw32(u8 *dest, u8 *src, unsigned int n)
static
+static int hiface_pcm_playback(struct pcm_substream *sub,
struct pcm_urb *urb)+{
- /* XXX Can an invalid format make it to here?
No.
+static void hiface_pcm_out_urb_handler(struct urb *usb_urb) +{
- if (usb_urb->status || rt->panic || rt->stream_state == STREAM_STOPPING)
return;
In some error cases, you might consider stopping the stream.
ret = hiface_pcm_playback(sub, out_urb);if (ret < 0) {spin_unlock_irqrestore(&sub->lock, flags);goto out_fail;}+... +out_fail:
- usb_submit_urb(&out_urb->instance, GFP_ATOMIC);
Same here.
+static int hiface_pcm_open(struct snd_pcm_substream *alsa_sub) +{
/* XXX can we avoid setting hw.rates below?
You must set hw.rates to all the rates supported by this particular device.
The KNOT flag overrides all others flags and requires a separate constraints to describe the valid rates.
You could either
for 192 kHz devices: set the 44100...192000 flags and install no constraint, while for 384 kHz devices: set the KNOT flag and install the 44.1...384 rate constraint;
or, alternatively,
for 192 kHz devices: set the KNOT flag and install a 44.1...192 rate constraint, while for 384 kHz devices: set the KNOT flag and install a 44.1...384 rate constraint.
* Maybe the 6fire driver was setting alsa_rt->hw.rates in* order to have the playback stream and the capture stream* use the same rate?
Yes.
constraints_rates.count = ARRAY_SIZE(rates);
This does not work if contraints_rates is shared by two different devices. Use two snd_pcm_hw_constraint_list instances.
+void hiface_pcm_abort(struct hiface_chip *chip) +{
snd_pcm_stop(rt->playback.instance,SNDRV_PCM_STATE_XRUN);
This requres locking the stream with something like snd_pcm_stream_lock_irq().
Regards, Clemens