linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
@ 2019-07-02  0:43 Nicola Lunghi
  2019-07-02 14:37 ` Takashi Iwai
  0 siblings, 1 reply; 7+ messages in thread
From: Nicola Lunghi @ 2019-07-02  0:43 UTC (permalink / raw)
  To: alsa-devel
  Cc: info, Nicola Lunghi, Jaroslav Kysela, Takashi Iwai,
	Allison Randal, Greg Kroah-Hartman, Richard Fontana,
	Thomas Gleixner, Jussi Laako, linux-kernel

Line6 Helix and HX stomp don't support retrieving
the number of clock sample rate.

Add a quirk to return the default value of 48Khz.

Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
---
 sound/usb/format.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index c02b51a82775..05442f6ada62 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct snd_usb_audio *chip,
 			      tmp, sizeof(tmp));
 
 	if (ret < 0) {
-		dev_err(&dev->dev,
-			"%s(): unable to retrieve number of sample rates (clock %d)\n",
+		switch (chip->usb_id) {
+		/* LINE 6 HX pedals don't support getting the clock sample rate.
+		 * Set the framerate to 48khz by default
+		 */
+		case USB_ID(0x0E41, 0x4244): /* HELIX */
+		case USB_ID(0x0E41, 0x4246): /* HX STOMP */
+			dev_warn(&dev->dev,
+				"%s(): line6 helix: unable to retrieve number of sample rates. Set it to default value (clock %d).\n",
 				__func__, clock);
-		goto err;
+			fp->nr_rates = 1;
+			fp->rate_min = 48000;
+			fp->rate_max = 48000;
+			fp->rates = SNDRV_PCM_RATE_48000;
+			fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
+			if (!fp->rate_table) {
+				ret = -ENOMEM;
+				goto err_free;
+			}
+			fp->rate_table[0] = 48000;
+			return 0;
+		default:
+			dev_err(&dev->dev,
+				"%s(): unable to retrieve number of sample rates (clock %d)\n",
+					__func__, clock);
+			goto err;
+		}
 	}
 
 	nr_triplets = (tmp[1] << 8) | tmp[0];
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
  2019-07-02  0:43 [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates Nicola Lunghi
@ 2019-07-02 14:37 ` Takashi Iwai
  2019-07-02 15:52   ` [alsa-devel] " Wasko, Michal
  0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2019-07-02 14:37 UTC (permalink / raw)
  To: Nicola Lunghi
  Cc: alsa-devel, info, Thomas Gleixner, Greg Kroah-Hartman,
	Allison Randal, Jaroslav Kysela, Richard Fontana, Jussi Laako,
	linux-kernel

On Tue, 02 Jul 2019 02:43:14 +0200,
Nicola Lunghi wrote:
> 
> Line6 Helix and HX stomp don't support retrieving
> the number of clock sample rate.
> 
> Add a quirk to return the default value of 48Khz.
> 
> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>

It's not particularly good place to put a quirk, but there seems no
other better place, unfortunately.  Is this specific to certain unit
or all I/Os on the device suffer from this problem?

In anyway, if the behavior is expected, we don't need to use
dev_warn() to annoy users unnecessarily.  Replace it with dev_info().

Also, the code that creates a single 48k entry would be better to be
put into a function for readability.

Could you resubmit with that change?


Thanks!

Takashi

> ---
>  sound/usb/format.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index c02b51a82775..05442f6ada62 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct snd_usb_audio *chip,
>  			      tmp, sizeof(tmp));
>  
>  	if (ret < 0) {
> -		dev_err(&dev->dev,
> -			"%s(): unable to retrieve number of sample rates (clock %d)\n",
> +		switch (chip->usb_id) {
> +		/* LINE 6 HX pedals don't support getting the clock sample rate.
> +		 * Set the framerate to 48khz by default
> +		 */
> +		case USB_ID(0x0E41, 0x4244): /* HELIX */
> +		case USB_ID(0x0E41, 0x4246): /* HX STOMP */
> +			dev_warn(&dev->dev,
> +				"%s(): line6 helix: unable to retrieve number of sample rates. Set it to default value (clock %d).\n",
>  				__func__, clock);
> -		goto err;
> +			fp->nr_rates = 1;
> +			fp->rate_min = 48000;
> +			fp->rate_max = 48000;
> +			fp->rates = SNDRV_PCM_RATE_48000;
> +			fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
> +			if (!fp->rate_table) {
> +				ret = -ENOMEM;
> +				goto err_free;
> +			}
> +			fp->rate_table[0] = 48000;
> +			return 0;
> +		default:
> +			dev_err(&dev->dev,
> +				"%s(): unable to retrieve number of sample rates (clock %d)\n",
> +					__func__, clock);
> +			goto err;
> +		}
>  	}
>  
>  	nr_triplets = (tmp[1] << 8) | tmp[0];
> -- 
> 2.19.1
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
  2019-07-02 14:37 ` Takashi Iwai
@ 2019-07-02 15:52   ` Wasko, Michal
  2019-07-02 15:57     ` Takashi Iwai
  0 siblings, 1 reply; 7+ messages in thread
From: Wasko, Michal @ 2019-07-02 15:52 UTC (permalink / raw)
  To: Takashi Iwai, Nicola Lunghi
  Cc: alsa-devel, info, Greg Kroah-Hartman, linux-kernel, Jussi Laako,
	Richard Fontana, Thomas Gleixner, Allison Randal

On 7/2/2019 4:37 PM, Takashi Iwai wrote:
> On Tue, 02 Jul 2019 02:43:14 +0200,
> Nicola Lunghi wrote:
>> Line6 Helix and HX stomp don't support retrieving
>> the number of clock sample rate.
>>
>> Add a quirk to return the default value of 48Khz.
>>
>> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
> It's not particularly good place to put a quirk, but there seems no
> other better place, unfortunately.  Is this specific to certain unit
> or all I/Os on the device suffer from this problem?
>
> In anyway, if the behavior is expected, we don't need to use
> dev_warn() to annoy users unnecessarily.  Replace it with dev_info().
>
> Also, the code that creates a single 48k entry would be better to be
> put into a function for readability.
>
> Could you resubmit with that change?
>
>
> Thanks!
>
> Takashi

If the listed USB devices do not support sample rate format retrieval
then maybe it would be a better idea to perform below check before
sending message?

Have you also considered new function or macro that check device
support? This would separate formatfunctionality code from routine
that identifies applicable devices- in case if in future more devices
will require quirk.

Michal W.

>> ---
>>   sound/usb/format.c | 28 +++++++++++++++++++++++++---
>>   1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/usb/format.c b/sound/usb/format.c
>> index c02b51a82775..05442f6ada62 100644
>> --- a/sound/usb/format.c
>> +++ b/sound/usb/format.c
>> @@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct snd_usb_audio *chip,
>>   			      tmp, sizeof(tmp));
>>   
>>   	if (ret < 0) {
>> -		dev_err(&dev->dev,
>> -			"%s(): unable to retrieve number of sample rates (clock %d)\n",
>> +		switch (chip->usb_id) {
>> +		/* LINE 6 HX pedals don't support getting the clock sample rate.
>> +		 * Set the framerate to 48khz by default
>> +		 */
>> +		case USB_ID(0x0E41, 0x4244): /* HELIX */
>> +		case USB_ID(0x0E41, 0x4246): /* HX STOMP */
>> +			dev_warn(&dev->dev,
>> +				"%s(): line6 helix: unable to retrieve number of sample rates. Set it to default value (clock %d).\n",
>>   				__func__, clock);
>> -		goto err;
>> +			fp->nr_rates = 1;
>> +			fp->rate_min = 48000;
>> +			fp->rate_max = 48000;
>> +			fp->rates = SNDRV_PCM_RATE_48000;
>> +			fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
>> +			if (!fp->rate_table) {
>> +				ret = -ENOMEM;
>> +				goto err_free;
>> +			}
>> +			fp->rate_table[0] = 48000;
>> +			return 0;
>> +		default:
>> +			dev_err(&dev->dev,
>> +				"%s(): unable to retrieve number of sample rates (clock %d)\n",
>> +					__func__, clock);
>> +			goto err;
>> +		}
>>   	}
>>   
>>   	nr_triplets = (tmp[1] << 8) | tmp[0];
>> -- 
>> 2.19.1
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
  2019-07-02 15:52   ` [alsa-devel] " Wasko, Michal
@ 2019-07-02 15:57     ` Takashi Iwai
       [not found]       ` <CABPh3UMttE1s7oNt0-JLNm2N3wxb-JXBTLzMYZM42ENiy9NC9Q@mail.gmail.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2019-07-02 15:57 UTC (permalink / raw)
  To: Wasko, Michal
  Cc: Nicola Lunghi, alsa-devel, info, Greg Kroah-Hartman,
	linux-kernel, Jussi Laako, Richard Fontana, Thomas Gleixner,
	Allison Randal

On Tue, 02 Jul 2019 17:52:01 +0200,
Wasko, Michal wrote:
> 
> On 7/2/2019 4:37 PM, Takashi Iwai wrote:
> > On Tue, 02 Jul 2019 02:43:14 +0200,
> > Nicola Lunghi wrote:
> >> Line6 Helix and HX stomp don't support retrieving
> >> the number of clock sample rate.
> >>
> >> Add a quirk to return the default value of 48Khz.
> >>
> >> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
> > It's not particularly good place to put a quirk, but there seems no
> > other better place, unfortunately.  Is this specific to certain unit
> > or all I/Os on the device suffer from this problem?
> >
> > In anyway, if the behavior is expected, we don't need to use
> > dev_warn() to annoy users unnecessarily.  Replace it with dev_info().
> >
> > Also, the code that creates a single 48k entry would be better to be
> > put into a function for readability.
> >
> > Could you resubmit with that change?
> >
> >
> > Thanks!
> >
> > Takashi
> 
> If the listed USB devices do not support sample rate format retrieval
> then maybe it would be a better idea to perform below check before
> sending message?

Yes, if we know that it always fails, we don't need to query.

> Have you also considered new function or macro that check device
> support? This would separate formatfunctionality code from routine
> that identifies applicable devices- in case if in future more devices
> will require quirk.

The split can be done later.  It's always hard to know what kind of
quirk would be needed in future.  If any more devices show the same
problem, we can reorganize the quirk in a saner way.


thanks,

Takashi

> 
> Michal W.
> 
> >> ---
> >>   sound/usb/format.c | 28 +++++++++++++++++++++++++---
> >>   1 file changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/sound/usb/format.c b/sound/usb/format.c
> >> index c02b51a82775..05442f6ada62 100644
> >> --- a/sound/usb/format.c
> >> +++ b/sound/usb/format.c
> >> @@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct snd_usb_audio *chip,
> >>   			      tmp, sizeof(tmp));
> >>     	if (ret < 0) {
> >> -		dev_err(&dev->dev,
> >> -			"%s(): unable to retrieve number of sample rates (clock %d)\n",
> >> +		switch (chip->usb_id) {
> >> +		/* LINE 6 HX pedals don't support getting the clock sample rate.
> >> +		 * Set the framerate to 48khz by default
> >> +		 */
> >> +		case USB_ID(0x0E41, 0x4244): /* HELIX */
> >> +		case USB_ID(0x0E41, 0x4246): /* HX STOMP */
> >> +			dev_warn(&dev->dev,
> >> +				"%s(): line6 helix: unable to retrieve number of sample rates. Set it to default value (clock %d).\n",
> >>   				__func__, clock);
> >> -		goto err;
> >> +			fp->nr_rates = 1;
> >> +			fp->rate_min = 48000;
> >> +			fp->rate_max = 48000;
> >> +			fp->rates = SNDRV_PCM_RATE_48000;
> >> +			fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
> >> +			if (!fp->rate_table) {
> >> +				ret = -ENOMEM;
> >> +				goto err_free;
> >> +			}
> >> +			fp->rate_table[0] = 48000;
> >> +			return 0;
> >> +		default:
> >> +			dev_err(&dev->dev,
> >> +				"%s(): unable to retrieve number of sample rates (clock %d)\n",
> >> +					__func__, clock);
> >> +			goto err;
> >> +		}
> >>   	}
> >>     	nr_triplets = (tmp[1] << 8) | tmp[0];
> >> -- 
> >> 2.19.1
> >>
> >>
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
       [not found]       ` <CABPh3UMttE1s7oNt0-JLNm2N3wxb-JXBTLzMYZM42ENiy9NC9Q@mail.gmail.com>
@ 2019-07-03  7:49         ` Takashi Iwai
  2019-07-03  8:26           ` Wasko, Michal
  0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2019-07-03  7:49 UTC (permalink / raw)
  To: nick83ola
  Cc: Wasko, Michal, alsa-devel, info, Greg Kroah-Hartman,
	linux-kernel, Jussi Laako, Richard Fontana, Thomas Gleixner,
	Allison Randal

On Wed, 03 Jul 2019 09:41:00 +0200,
nick83ola wrote:
> 
> On Tue 2 Jul 2019, 16:57 Takashi Iwai, <tiwai@suse.de> wrote:
> 
>     On Tue, 02 Jul 2019 17:52:01 +0200,
>     Wasko, Michal wrote:
>     >
>     > On 7/2/2019 4:37 PM, Takashi Iwai wrote:
>     > > On Tue, 02 Jul 2019 02:43:14 +0200,
>     > > Nicola Lunghi wrote:
>     > >> Line6 Helix and HX stomp don't support retrieving
>     > >> the number of clock sample rate.
>     > >>
>     > >> Add a quirk to return the default value of 48Khz.
>     > >>
>     > >> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
>     > > It's not particularly good place to put a quirk, but there seems no
>     > > other better place, unfortunately. 
> 
> If you prefer I can add a function to quirk.c

It's OK to place in format.c as a start.

>     Is this specific to certain unit
>     > > or all I/Os on the device suffer from this problem?
> 
> This is specific to the helix line of line6
> There are currently 4 devices in that line that I think shared their firmware
> more or less.
> Unfortunately I have only one device to test here (but maybe someone else can
> add the others)

Ah sorry I wasn't clear enough.  What I meant as "unit" is the USB
descriptor unit (aka widget), a feature unit, a terminal, whatever.
Does this error appear only once at parsing, or does it hit multiple
times per device?

>     > >
>     > > In anyway, if the behavior is expected, we don't need to use
>     > > dev_warn() to annoy users unnecessarily.  Replace it with dev_info().
>     > >
> 
> Ok
> 
>     > > Also, the code that creates a single 48k entry would be better to be
>     > > put into a function for readability.
>     > >
>     > > Could you resubmit with that change?
>     > >
> 
> Yes (in quirks.c or here?)
> 
> Also I suspect that there' s a way to get the clock rates because windows
> driver supports getting the rates and switching (and in Mac the device has the
> same problem without a custom driver only supports 48khz)

It might be some fixed list without inquiries?


>     > >
>     > > Thanks!
>     > >
>     > > Takashi
>     >
> 
> Thank you
> 
>     > If the listed USB devices do not support sample rate format retrieval
>     > then maybe it would be a better idea to perform below check before
>     > sending message?
>    
>     Yes, if we know that it always fails, we don't need to query.
> 
> Sorry I don't understand this :-)

Well, the error happens always on your device in the very same way,
right?  Then we don't have to send the specific command that leads to
an error, but takes the fixed 48k from the beginning.


> My idea is that if line6 in the future fixes their code (they are quite active
> on the helix line) the call will not fail and we get a proper device without
> quirks.
> But If the driver fail to get the clock this settings works as a "failsafe"
> and get the device working.
> I also tried to contact their support but they don't care a lot about Linux
> for now :-(

The fallback might be good, but I'm not sure whether there are many
devices that hit the same problem.  If we encounter more, let's try to
make it as a fallback.


thanks,

Takashi

> 
> I will clean this better and resubmit
> Thank you
> Nick
> 
>     > Have you also considered new function or macro that check device
>     > support? This would separate formatfunctionality code from routine
>     > that identifies applicable devices- in case if in future more devices
>     > will require quirk.
>    
>     The split can be done later.  It's always hard to know what kind of
>     quirk would be needed in future.  If any more devices show the same
>     problem, we can reorganize the quirk in a saner way.
> 
>     thanks,
>    
>     Takashi
> 
>     >
>     > Michal W.
>     >
>     > >> ---
>     > >>   sound/usb/format.c | 28 +++++++++++++++++++++++++---
>     > >>   1 file changed, 25 insertions(+), 3 deletions(-)
>     > >>
>     > >> diff --git a/sound/usb/format.c b/sound/usb/format.c
>     > >> index c02b51a82775..05442f6ada62 100644
>     > >> --- a/sound/usb/format.c
>     > >> +++ b/sound/usb/format.c
>     > >> @@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct
>     snd_usb_audio *chip,
>     > >>                          tmp, sizeof(tmp));
>     > >>            if (ret < 0) {
>     > >> -          dev_err(&dev->dev,
>     > >> -                  "%s(): unable to retrieve number of sample rates
>     (clock %d)\n",
>     > >> +          switch (chip->usb_id) {
>     > >> +          /* LINE 6 HX pedals don't support getting the clock sample
>     rate.
>     > >> +           * Set the framerate to 48khz by default
>     > >> +           */
>     > >> +          case USB_ID(0x0E41, 0x4244): /* HELIX */
>     > >> +          case USB_ID(0x0E41, 0x4246): /* HX STOMP */
>     > >> +                  dev_warn(&dev->dev,
>     > >> +                          "%s(): line6 helix: unable to retrieve
>     number of sample rates. Set it to default value (clock %d).\n",
>     > >>                            __func__, clock);
>     > >> -          goto err;
>     > >> +                  fp->nr_rates = 1;
>     > >> +                  fp->rate_min = 48000;
>     > >> +                  fp->rate_max = 48000;
>     > >> +                  fp->rates = SNDRV_PCM_RATE_48000;
>     > >> +                  fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
>     > >> +                  if (!fp->rate_table) {
>     > >> +                          ret = -ENOMEM;
>     > >> +                          goto err_free;
>     > >> +                  }
>     > >> +                  fp->rate_table[0] = 48000;
>     > >> +                  return 0;
>     > >> +          default:
>     > >> +                  dev_err(&dev->dev,
>     > >> +                          "%s(): unable to retrieve number of sample
>     rates (clock %d)\n",
>     > >> +                                  __func__, clock);
>     > >> +                  goto err;
>     > >> +          }
>     > >>    }
>     > >>            nr_triplets = (tmp[1] << 8) | tmp[0];
>     > >> --
>     > >> 2.19.1
>     > >>
>     > >>
>     > > _______________________________________________
>     > > Alsa-devel mailing list
>     > > Alsa-devel@alsa-project.org
>     > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>     > >
>     >
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
  2019-07-03  7:49         ` Takashi Iwai
@ 2019-07-03  8:26           ` Wasko, Michal
  2019-07-03  8:32             ` Takashi Iwai
  0 siblings, 1 reply; 7+ messages in thread
From: Wasko, Michal @ 2019-07-03  8:26 UTC (permalink / raw)
  To: Takashi Iwai, nick83ola
  Cc: alsa-devel, info, Greg Kroah-Hartman, linux-kernel,
	Richard Fontana, Jussi Laako, Thomas Gleixner, Allison Randal

On 7/3/2019 9:49 AM, Takashi Iwai wrote:

> On Wed, 03 Jul 2019 09:41:00 +0200,
> nick83ola wrote:
>> On Tue 2 Jul 2019, 16:57 Takashi Iwai, <tiwai@suse.de> wrote:
>>
>>      On Tue, 02 Jul 2019 17:52:01 +0200,
>>      Wasko, Michal wrote:
>>      >
>>      > On 7/2/2019 4:37 PM, Takashi Iwai wrote:
>>      > > On Tue, 02 Jul 2019 02:43:14 +0200,
>>      > > Nicola Lunghi wrote:
>>      > >> Line6 Helix and HX stomp don't support retrieving
>>      > >> the number of clock sample rate.
>>      > >>
>>      > >> Add a quirk to return the default value of 48Khz.
>>      > >>
>>      > >> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
>>      > > It's not particularly good place to put a quirk, but there seems no
>>      > > other better place, unfortunately.
>>
>> If you prefer I can add a function to quirk.c
> It's OK to place in format.c as a start.
>
>>      Is this specific to certain unit
>>      > > or all I/Os on the device suffer from this problem?
>>
>> This is specific to the helix line of line6
>> There are currently 4 devices in that line that I think shared their firmware
>> more or less.
>> Unfortunately I have only one device to test here (but maybe someone else can
>> add the others)
> Ah sorry I wasn't clear enough.  What I meant as "unit" is the USB
> descriptor unit (aka widget), a feature unit, a terminal, whatever.
> Does this error appear only once at parsing, or does it hit multiple
> times per device?
>
>>      > >
>>      > > In anyway, if the behavior is expected, we don't need to use
>>      > > dev_warn() to annoy users unnecessarily.  Replace it with dev_info().
>>      > >
>>
>> Ok
>>
>>      > > Also, the code that creates a single 48k entry would be better to be
>>      > > put into a function for readability.
>>      > >
>>      > > Could you resubmit with that change?
>>      > >
>>
>> Yes (in quirks.c or here?)
>>
>> Also I suspect that there' s a way to get the clock rates because windows
>> driver supports getting the rates and switching (and in Mac the device has the
>> same problem without a custom driver only supports 48khz)
> It might be some fixed list without inquiries?
>
>
>>      > >
>>      > > Thanks!
>>      > >
>>      > > Takashi
>>      >
>>
>> Thank you
>>
>>      > If the listed USB devices do not support sample rate format retrieval
>>      > then maybe it would be a better idea to perform below check before
>>      > sending message?
>>     
>>      Yes, if we know that it always fails, we don't need to query.
>>
>> Sorry I don't understand this :-)
> Well, the error happens always on your device in the very same way,
> right?  Then we don't have to send the specific command that leads to
> an error, but takes the fixed 48k from the beginning.
>
>
>> My idea is that if line6 in the future fixes their code (they are quite active
>> on the helix line) the call will not fail and we get a proper device without
>> quirks.
>> But If the driver fail to get the clock this settings works as a "failsafe"
>> and get the device working.
>> I also tried to contact their support but they don't care a lot about Linux
>> for now :-(
> The fallback might be good, but I'm not sure whether there are many
> devices that hit the same problem.  If we encounter more, let's try to
> make it as a fallback.
> thanks,
>
> Takashi
Since there is a chance that the issue will be addressed in USB device FW
thanthe fallback mechanism will make sense. It would cover eventual future
scenario withdevices that will have the issue fixed.

Michal W.
>> I will clean this better and resubmit
>> Thank you
>> Nick
>>
>>      > Have you also considered new function or macro that check device
>>      > support? This would separate formatfunctionality code from routine
>>      > that identifies applicable devices- in case if in future more devices
>>      > will require quirk.
>>     
>>      The split can be done later.  It's always hard to know what kind of
>>      quirk would be needed in future.  If any more devices show the same
>>      problem, we can reorganize the quirk in a saner way.
>>
>>      thanks,
>>     
>>      Takashi
>>
>>      >
>>      > Michal W.
>>      >
>>      > >> ---
>>      > >>   sound/usb/format.c | 28 +++++++++++++++++++++++++---
>>      > >>   1 file changed, 25 insertions(+), 3 deletions(-)
>>      > >>
>>      > >> diff --git a/sound/usb/format.c b/sound/usb/format.c
>>      > >> index c02b51a82775..05442f6ada62 100644
>>      > >> --- a/sound/usb/format.c
>>      > >> +++ b/sound/usb/format.c
>>      > >> @@ -313,10 +313,32 @@ static int parse_audio_format_rates_v2v3(struct
>>      snd_usb_audio *chip,
>>      > >>                          tmp, sizeof(tmp));
>>      > >>            if (ret < 0) {
>>      > >> -          dev_err(&dev->dev,
>>      > >> -                  "%s(): unable to retrieve number of sample rates
>>      (clock %d)\n",
>>      > >> +          switch (chip->usb_id) {
>>      > >> +          /* LINE 6 HX pedals don't support getting the clock sample
>>      rate.
>>      > >> +           * Set the framerate to 48khz by default
>>      > >> +           */
>>      > >> +          case USB_ID(0x0E41, 0x4244): /* HELIX */
>>      > >> +          case USB_ID(0x0E41, 0x4246): /* HX STOMP */
>>      > >> +                  dev_warn(&dev->dev,
>>      > >> +                          "%s(): line6 helix: unable to retrieve
>>      number of sample rates. Set it to default value (clock %d).\n",
>>      > >>                            __func__, clock);
>>      > >> -          goto err;
>>      > >> +                  fp->nr_rates = 1;
>>      > >> +                  fp->rate_min = 48000;
>>      > >> +                  fp->rate_max = 48000;
>>      > >> +                  fp->rates = SNDRV_PCM_RATE_48000;
>>      > >> +                  fp->rate_table = kmalloc(sizeof(int), GFP_KERNEL);
>>      > >> +                  if (!fp->rate_table) {
>>      > >> +                          ret = -ENOMEM;
>>      > >> +                          goto err_free;
>>      > >> +                  }
>>      > >> +                  fp->rate_table[0] = 48000;
>>      > >> +                  return 0;
>>      > >> +          default:
>>      > >> +                  dev_err(&dev->dev,
>>      > >> +                          "%s(): unable to retrieve number of sample
>>      rates (clock %d)\n",
>>      > >> +                                  __func__, clock);
>>      > >> +                  goto err;
>>      > >> +          }
>>      > >>    }
>>      > >>            nr_triplets = (tmp[1] << 8) | tmp[0];
>>      > >> --
>>      > >> 2.19.1
>>      > >>
>>      > >>
>>      > > _______________________________________________
>>      > > Alsa-devel mailing list
>>      > > Alsa-devel@alsa-project.org
>>      > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>      > >
>>      >
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [alsa-devel] [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates
  2019-07-03  8:26           ` Wasko, Michal
@ 2019-07-03  8:32             ` Takashi Iwai
  0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2019-07-03  8:32 UTC (permalink / raw)
  To: Wasko, Michal
  Cc: nick83ola, alsa-devel, info, Greg Kroah-Hartman, linux-kernel,
	Richard Fontana, Jussi Laako, Thomas Gleixner, Allison Randal

On Wed, 03 Jul 2019 10:26:31 +0200,
Wasko, Michal wrote:
> 
> >> My idea is that if line6 in the future fixes their code (they are quite active
> >> on the helix line) the call will not fail and we get a proper device without
> >> quirks.
> >> But If the driver fail to get the clock this settings works as a "failsafe"
> >> and get the device working.
> >> I also tried to contact their support but they don't care a lot about Linux
> >> for now :-(
> > The fallback might be good, but I'm not sure whether there are many
> > devices that hit the same problem.  If we encounter more, let's try to
> > make it as a fallback.
> > thanks,
> >
> > Takashi
> Since there is a chance that the issue will be addressed in USB device FW
> thanthe fallback mechanism will make sense. It would cover eventual future
> scenario withdevices that will have the issue fixed.

Fair enough.  Let's handle it as fallback, then.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-07-03  8:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-02  0:43 [PATCH] ALSA: usb-audio: fix Line6 Helix audio format rates Nicola Lunghi
2019-07-02 14:37 ` Takashi Iwai
2019-07-02 15:52   ` [alsa-devel] " Wasko, Michal
2019-07-02 15:57     ` Takashi Iwai
     [not found]       ` <CABPh3UMttE1s7oNt0-JLNm2N3wxb-JXBTLzMYZM42ENiy9NC9Q@mail.gmail.com>
2019-07-03  7:49         ` Takashi Iwai
2019-07-03  8:26           ` Wasko, Michal
2019-07-03  8:32             ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).