Fixes in intel8x0.c

Fixes in intel8x0.c

[Pavel Hofman]

Hi,

while trying to dive into complexities of intel8x0.c I found the following:

static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
.....

Is it really supposed to be 41000, instead of 44100?


In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
struct ichdev *ichdev)
...
	runtime->hw.rates = ichdev->pcm->rates;

The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
is defined

unsigned int rates[6];	/* see AC97_RATES_* defines */

I guess this way always rates[0] from the list is taken, unlike e.g. in
via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];

It seems a bug to me, though I may be wrong of course.


Thanks and regards,

Pavel

Re: Fixes in intel8x0.c

[Takashi Iwai]

At Tue, 26 May 2009 01:19:06 +0200,
Pavel Hofman wrote:
> 
> Hi,
> 
> while trying to dive into complexities of intel8x0.c I found the following:
> 
> static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
> 	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
> 	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
> .....
> 
> Is it really supposed to be 41000, instead of 44100?

Looks like so, judging from the patch Jaroslav wrote.

> In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
> struct ichdev *ichdev)
> ...
> 	runtime->hw.rates = ichdev->pcm->rates;
> 
> The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
> is defined
> 
> unsigned int rates[6];	/* see AC97_RATES_* defines */
> 
> I guess this way always rates[0] from the list is taken, unlike e.g. in
> via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];

41000 is the base clock.  The actual rate written is

	rate * clock / 48000

Normally the base clock is 48000.  But some boards have a different
base clock and the rate has to be adjusted to match with that.


Takashi

Re: Fixes in intel8x0.c

[Pavel Hofman]

Takashi Iwai wrote:
> At Tue, 26 May 2009 01:19:06 +0200,
> Pavel Hofman wrote:
>> Hi,
>>
>> while trying to dive into complexities of intel8x0.c I found the following:
>>
>> static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
>> 	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
>> 	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
>> .....
>>
>> Is it really supposed to be 41000, instead of 44100?
> 
> Looks like so, judging from the patch Jaroslav wrote.
> 
>> In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
>> struct ichdev *ichdev)
>> ...
>> 	runtime->hw.rates = ichdev->pcm->rates;
>>
>> The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
>> is defined
>>
>> unsigned int rates[6];	/* see AC97_RATES_* defines */
>>
>> I guess this way always rates[0] from the list is taken, unlike e.g. in
>> via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];
> 
> 41000 is the base clock.  The actual rate written is
> 
> 	rate * clock / 48000
> 
> Normally the base clock is 48000.  But some boards have a different
> base clock and the rate has to be adjusted to match with that.
> 

Takashi, thanks for your answer, 41000 is OK.

How about the second question regarding the assignment of rates field
being an array? I am remotely troubleshooting SPDIF output of Dell D600
which seems to be stuck at 48kHz (plughw always resamples to 48kHz, even
though the analog  PCM accepts 44.1kHz. The above mentioned code is
related to this (not exactly the cause though.

Thanks a lot,

Pavel.

Re: Fixes in intel8x0.c

[Takashi Iwai]

At Tue, 26 May 2009 10:09:57 +0200,
Pavel Hofman wrote:
> 
> Takashi Iwai wrote:
> > At Tue, 26 May 2009 01:19:06 +0200,
> > Pavel Hofman wrote:
> >> Hi,
> >>
> >> while trying to dive into complexities of intel8x0.c I found the following:
> >>
> >> static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
> >> 	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
> >> 	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
> >> .....
> >>
> >> Is it really supposed to be 41000, instead of 44100?
> > 
> > Looks like so, judging from the patch Jaroslav wrote.
> > 
> >> In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
> >> struct ichdev *ichdev)
> >> ...
> >> 	runtime->hw.rates = ichdev->pcm->rates;
> >>
> >> The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
> >> is defined
> >>
> >> unsigned int rates[6];	/* see AC97_RATES_* defines */
> >>
> >> I guess this way always rates[0] from the list is taken, unlike e.g. in
> >> via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];
> > 
> > 41000 is the base clock.  The actual rate written is
> > 
> > 	rate * clock / 48000
> > 
> > Normally the base clock is 48000.  But some boards have a different
> > base clock and the rate has to be adjusted to match with that.
> > 
> 
> Takashi, thanks for your answer, 41000 is OK.
> 
> How about the second question regarding the assignment of rates field
> being an array? I am remotely troubleshooting SPDIF output of Dell D600
> which seems to be stuck at 48kHz (plughw always resamples to 48kHz, even
> though the analog  PCM accepts 44.1kHz. The above mentioned code is
> related to this (not exactly the cause though.

I don't see any problem in the code.  Are you sure that it's really related?


Takashi

Re: Fixes in intel8x0.c

[Pavel Hofman]

Takashi Iwai wrote:
> At Tue, 26 May 2009 10:09:57 +0200,
> Pavel Hofman wrote:
>> Takashi Iwai wrote:
>>> At Tue, 26 May 2009 01:19:06 +0200,
>>> Pavel Hofman wrote:
>>>> Hi,
>>>>
>>>> while trying to dive into complexities of intel8x0.c I found the following:
>>>>
>>>> static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
>>>> 	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
>>>> 	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
>>>> .....
>>>>
>>>> Is it really supposed to be 41000, instead of 44100?
>>> Looks like so, judging from the patch Jaroslav wrote.
>>>
>>>> In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
>>>> struct ichdev *ichdev)
>>>> ...
>>>> 	runtime->hw.rates = ichdev->pcm->rates;
>>>>
>>>> The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
>>>> is defined
>>>>
>>>> unsigned int rates[6];	/* see AC97_RATES_* defines */
>>>>
>>>> I guess this way always rates[0] from the list is taken, unlike e.g. in
>>>> via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];
>>> 41000 is the base clock.  The actual rate written is
>>>
>>> 	rate * clock / 48000
>>>
>>> Normally the base clock is 48000.  But some boards have a different
>>> base clock and the rate has to be adjusted to match with that.
>>>
>> Takashi, thanks for your answer, 41000 is OK.
>>
>> How about the second question regarding the assignment of rates field
>> being an array? I am remotely troubleshooting SPDIF output of Dell D600
>> which seems to be stuck at 48kHz (plughw always resamples to 48kHz, even
>> though the analog  PCM accepts 44.1kHz. The above mentioned code is
>> related to this (not exactly the cause though.
> 
> I don't see any problem in the code.  Are you sure that it's really related?

Sorry, my fault, I confused ac97_pcm with snd_ac97.

Thanks,

Pavel.

Re: Fixes in intel8x0.c

[Takashi Iwai]

At Tue, 26 May 2009 11:55:05 +0200,
Pavel Hofman wrote:
> 
> Takashi Iwai wrote:
> > At Tue, 26 May 2009 10:09:57 +0200,
> > Pavel Hofman wrote:
> >> Takashi Iwai wrote:
> >>> At Tue, 26 May 2009 01:19:06 +0200,
> >>> Pavel Hofman wrote:
> >>>> Hi,
> >>>>
> >>>> while trying to dive into complexities of intel8x0.c I found the following:
> >>>>
> >>>> static struct snd_pci_quirk intel8x0_clock_list[] __devinitdata = {
> >>>> 	SND_PCI_QUIRK(0x0e11, 0x008a, "AD1885", 41000),
> >>>> 	SND_PCI_QUIRK(0x1028, 0x00be, "AD1885", 44100),
> >>>> .....
> >>>>
> >>>> Is it really supposed to be 41000, instead of 44100?
> >>> Looks like so, judging from the patch Jaroslav wrote.
> >>>
> >>>> In static int snd_intel8x0_pcm_open(struct snd_pcm_substream *substream,
> >>>> struct ichdev *ichdev)
> >>>> ...
> >>>> 	runtime->hw.rates = ichdev->pcm->rates;
> >>>>
> >>>> The pcm field in ichdev is snd_ac97, while the rates field in snd_ac97
> >>>> is defined
> >>>>
> >>>> unsigned int rates[6];	/* see AC97_RATES_* defines */
> >>>>
> >>>> I guess this way always rates[0] from the list is taken, unlike e.g. in
> >>>> via82xx.c: runtime->hw.rates = chip->ac97->rates[AC97_RATES_SPDIF];
> >>> 41000 is the base clock.  The actual rate written is
> >>>
> >>> 	rate * clock / 48000
> >>>
> >>> Normally the base clock is 48000.  But some boards have a different
> >>> base clock and the rate has to be adjusted to match with that.
> >>>
> >> Takashi, thanks for your answer, 41000 is OK.
> >>
> >> How about the second question regarding the assignment of rates field
> >> being an array? I am remotely troubleshooting SPDIF output of Dell D600
> >> which seems to be stuck at 48kHz (plughw always resamples to 48kHz, even
> >> though the analog  PCM accepts 44.1kHz. The above mentioned code is
> >> related to this (not exactly the cause though.
> > 
> > I don't see any problem in the code.  Are you sure that it's really related?
> 
> Sorry, my fault, I confused ac97_pcm with snd_ac97.

Well, actually it's a part of snd_ac97_codec module.

The logic in ac97_pcm.c is somewhat different from via82xx.c and others,
and is more complex due to the slog assignment mechanism.
(And actually SPDIF is limited only to 48k on many codecs, and the driver
 behaves intentionally so.)


Takashi