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

[Antonio Ospite]

On Tue, 12 Feb 2013 15:29:19 +0100
Clemens Ladisch <clemens at ladisch.de> wrote:

> Antonio Ospite wrote:
> > Clemens Ladisch <clemens at ladisch.de> wrote:
> >> Antonio Ospite wrote:

[...]
> >>> +	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?
> >
> > It looks like the iProduct field is not reliable enough, different
> > devices could have the same value there.
> 
> The iProduct field is the number of the string descriptor.
> That string might be useful at least in the non-quirk case
> (which shouldn't happen in the first case AFAICS).
> 

The original authors of the driver preferred to have all the device
names hardcoded in the driver, I guess they had a reason for that; for
now I'll leave it this way as I do not have all the supported devices to
verify the product strings for all of them. If Pietro can provide this
info I will update the driver to rely on info from the devices
themselves.

> >>> +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.
> >
> > You mean I should check explicitly for some values of usb_urb->status
> > and stop the stream if they happen? Like you do in
> > sound/usb/misc/ua101.c::capture_urb_complete()?
> 
> If only one URB doesn't get delivered for whatever reason, how should
> the driver react?  If you just exit from the completion handler, the
> driver will continue to fill and queue all the other URBs.
> And what happens when the device is unplugged?
>

I see, since —as Takashi said— killing URBs is not safe in the
complete callback I'll just set rt->panic to true when URBs fail:

https://github.com/panicking/snd-usb-asyncaudio/commit/0b7415e559e1a868240ac87a54ac805ca69fe4e7

> >>> +		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.
> >
> > If I remove the check for format in hiface_pcm_playback() then this
> > label would go away too and this won't be an error path anymore, but
> > maybe you meant that I need to check the return value of usb_submit_urb
> > () and stop the stream when the latter fails?
> 
> At the moment, you are trying to queue a URB with invalid samples.
> 

I am going to send a version 2 soon.

Thanks again,
   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?