| Message ID | 1401092564-14293-4-git-send-email-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c687200b9d4ef60042a50f7d942cfef120cc7bf1 |
| Delegated to: | Takashi Iwai |
| Headers | show |
(Add Alex Hung to CC) On 2014-05-26 10:22, Hui Wang wrote: > A lot a machine have the same codec, but they have different default > pinconf setting just because the def association and sequence is > different, as a result they can't share a hda_pintbl[], to overcome > it, we don't compare def association and sequence in the pinconf > matching. Uhm, really? Alex, does this seem reasonable from a BIOS perspective, i e, that BIOS people normally would set def association and sequence different while leaving everything else unchanged? > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > sound/pci/hda/hda_auto_parser.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c > index b684c6e..3cf9137 100644 > --- a/sound/pci/hda/hda_auto_parser.c > +++ b/sound/pci/hda/hda_auto_parser.c > @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec *codec, > { > for (; pins->nid; pins++) { > u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); > - if (pins->val != def_conf) > + u32 mask = 0xffffff00; > + if ((pins->val & mask) != (def_conf & mask)) > return false; > } > return true; >
Hi David, BIOS today implements verbtable which is provided by codec vendor based on hardware design, and it is indeed not uncommon that the verbtable includes used pin only and leaves unused pins untouched. On Mon, May 26, 2014 at 6:11 PM, David Henningsson <david.henningsson@canonical.com> wrote: > (Add Alex Hung to CC) > > On 2014-05-26 10:22, Hui Wang wrote: >> >> A lot a machine have the same codec, but they have different default >> pinconf setting just because the def association and sequence is >> different, as a result they can't share a hda_pintbl[], to overcome >> it, we don't compare def association and sequence in the pinconf >> matching. > > > Uhm, really? Alex, does this seem reasonable from a BIOS perspective, i e, > that BIOS people normally would set def association and sequence different > while leaving everything else unchanged? > >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> sound/pci/hda/hda_auto_parser.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/sound/pci/hda/hda_auto_parser.c >> b/sound/pci/hda/hda_auto_parser.c >> index b684c6e..3cf9137 100644 >> --- a/sound/pci/hda/hda_auto_parser.c >> +++ b/sound/pci/hda/hda_auto_parser.c >> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec *codec, >> { >> for (; pins->nid; pins++) { >> u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); >> - if (pins->val != def_conf) >> + u32 mask = 0xffffff00; >> + if ((pins->val & mask) != (def_conf & mask)) >> return false; >> } >> return true; >> > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
On 2014-05-27 04:25, Alex Hung wrote: > Hi David, > > BIOS today implements verbtable which is provided by codec vendor > based on hardware design, and it is indeed not uncommon that the > verbtable includes used pin only and leaves unused pins untouched. Sure, but those unused pins would then have the same default value that the codec initializes it with. Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the same verbtable for several machines if they share the same audio hardware. But none of this explains why anyone would just change def association and sequence value between machines? It makes no sense. > > On Mon, May 26, 2014 at 6:11 PM, David Henningsson > <david.henningsson@canonical.com> wrote: >> (Add Alex Hung to CC) >> >> On 2014-05-26 10:22, Hui Wang wrote: >>> >>> A lot a machine have the same codec, but they have different default >>> pinconf setting just because the def association and sequence is >>> different, as a result they can't share a hda_pintbl[], to overcome >>> it, we don't compare def association and sequence in the pinconf >>> matching. >> >> >> Uhm, really? Alex, does this seem reasonable from a BIOS perspective, i e, >> that BIOS people normally would set def association and sequence different >> while leaving everything else unchanged? >> >>> >>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>> --- >>> sound/pci/hda/hda_auto_parser.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/sound/pci/hda/hda_auto_parser.c >>> b/sound/pci/hda/hda_auto_parser.c >>> index b684c6e..3cf9137 100644 >>> --- a/sound/pci/hda/hda_auto_parser.c >>> +++ b/sound/pci/hda/hda_auto_parser.c >>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec *codec, >>> { >>> for (; pins->nid; pins++) { >>> u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); >>> - if (pins->val != def_conf) >>> + u32 mask = 0xffffff00; >>> + if ((pins->val & mask) != (def_conf & mask)) >>> return false; >>> } >>> return true; >>> >> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic > > >
On 05/27/2014 12:41 PM, David Henningsson wrote: > > > On 2014-05-27 04:25, Alex Hung wrote: >> Hi David, >> >> BIOS today implements verbtable which is provided by codec vendor >> based on hardware design, and it is indeed not uncommon that the >> verbtable includes used pin only and leaves unused pins untouched. > > Sure, but those unused pins would then have the same default value > that the codec initializes it with. > > Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the > same verbtable for several machines if they share the same audio > hardware. > > But none of this explains why anyone would just change def association > and sequence value between machines? It makes no sense. So far, I met one example for this case: the Dell laptops with the same 0x10ec0255 codec, On some machines: pin 0x12, 0x90a60170 pin 0x14, 0x90170120 pin 0x21, 0x02211030 On another machine: pin 0x12, 0x90a60140 pin 0x14, 0x90170110 pin 0x21, 0x02211020 The def config of the rest pins are same. Regards, Hui. > >> >> On Mon, May 26, 2014 at 6:11 PM, David Henningsson >> <david.henningsson@canonical.com> wrote: >>> (Add Alex Hung to CC) >>> >>> On 2014-05-26 10:22, Hui Wang wrote: >>>> >>>> A lot a machine have the same codec, but they have different default >>>> pinconf setting just because the def association and sequence is >>>> different, as a result they can't share a hda_pintbl[], to overcome >>>> it, we don't compare def association and sequence in the pinconf >>>> matching. >>> >>> >>> Uhm, really? Alex, does this seem reasonable from a BIOS >>> perspective, i e, >>> that BIOS people normally would set def association and sequence >>> different >>> while leaving everything else unchanged? >>> >>>> >>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>> --- >>>> sound/pci/hda/hda_auto_parser.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/sound/pci/hda/hda_auto_parser.c >>>> b/sound/pci/hda/hda_auto_parser.c >>>> index b684c6e..3cf9137 100644 >>>> --- a/sound/pci/hda/hda_auto_parser.c >>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec >>>> *codec, >>>> { >>>> for (; pins->nid; pins++) { >>>> u32 def_conf = snd_hda_codec_get_pincfg(codec, >>>> pins->nid); >>>> - if (pins->val != def_conf) >>>> + u32 mask = 0xffffff00; >>>> + if ((pins->val & mask) != (def_conf & mask)) >>>> return false; >>>> } >>>> return true; >>>> >>> >>> -- >>> David Henningsson, Canonical Ltd. >>> https://launchpad.net/~diwic >> >> >> >
On 2014-05-27 08:40, Hui Wang wrote: > On 05/27/2014 12:41 PM, David Henningsson wrote: >> >> >> On 2014-05-27 04:25, Alex Hung wrote: >>> Hi David, >>> >>> BIOS today implements verbtable which is provided by codec vendor >>> based on hardware design, and it is indeed not uncommon that the >>> verbtable includes used pin only and leaves unused pins untouched. >> >> Sure, but those unused pins would then have the same default value >> that the codec initializes it with. >> >> Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the >> same verbtable for several machines if they share the same audio >> hardware. >> >> But none of this explains why anyone would just change def association >> and sequence value between machines? It makes no sense. > > So far, I met one example for this case: > > the Dell laptops with the same 0x10ec0255 codec, > > On some machines: > pin 0x12, 0x90a60170 > pin 0x14, 0x90170120 > pin 0x21, 0x02211030 > > On another machine: > pin 0x12, 0x90a60140 > pin 0x14, 0x90170110 > pin 0x21, 0x02211020 > > > The def config of the rest pins are same. If there is only one example (and only two different options), I think we should revert this patch and use two different pin-matching quirks instead. After all, ignoring the def assoc/sequence values also means a greater risk of catching unwanted machines. Better err on the more careful side. This is IMO, what do others think? > > > Regards, > Hui. >> >>> >>> On Mon, May 26, 2014 at 6:11 PM, David Henningsson >>> <david.henningsson@canonical.com> wrote: >>>> (Add Alex Hung to CC) >>>> >>>> On 2014-05-26 10:22, Hui Wang wrote: >>>>> >>>>> A lot a machine have the same codec, but they have different default >>>>> pinconf setting just because the def association and sequence is >>>>> different, as a result they can't share a hda_pintbl[], to overcome >>>>> it, we don't compare def association and sequence in the pinconf >>>>> matching. >>>> >>>> >>>> Uhm, really? Alex, does this seem reasonable from a BIOS >>>> perspective, i e, >>>> that BIOS people normally would set def association and sequence >>>> different >>>> while leaving everything else unchanged? >>>> >>>>> >>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>>> --- >>>>> sound/pci/hda/hda_auto_parser.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/sound/pci/hda/hda_auto_parser.c >>>>> b/sound/pci/hda/hda_auto_parser.c >>>>> index b684c6e..3cf9137 100644 >>>>> --- a/sound/pci/hda/hda_auto_parser.c >>>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec >>>>> *codec, >>>>> { >>>>> for (; pins->nid; pins++) { >>>>> u32 def_conf = snd_hda_codec_get_pincfg(codec, >>>>> pins->nid); >>>>> - if (pins->val != def_conf) >>>>> + u32 mask = 0xffffff00; >>>>> + if ((pins->val & mask) != (def_conf & mask)) >>>>> return false; >>>>> } >>>>> return true; >>>>> >>>> >>>> -- >>>> David Henningsson, Canonical Ltd. >>>> https://launchpad.net/~diwic >>> >>> >>> >> >
At Tue, 27 May 2014 08:47:33 +0200, David Henningsson wrote: > > > > On 2014-05-27 08:40, Hui Wang wrote: > > On 05/27/2014 12:41 PM, David Henningsson wrote: > >> > >> > >> On 2014-05-27 04:25, Alex Hung wrote: > >>> Hi David, > >>> > >>> BIOS today implements verbtable which is provided by codec vendor > >>> based on hardware design, and it is indeed not uncommon that the > >>> verbtable includes used pin only and leaves unused pins untouched. > >> > >> Sure, but those unused pins would then have the same default value > >> that the codec initializes it with. > >> > >> Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the > >> same verbtable for several machines if they share the same audio > >> hardware. > >> > >> But none of this explains why anyone would just change def association > >> and sequence value between machines? It makes no sense. > > > > So far, I met one example for this case: > > > > the Dell laptops with the same 0x10ec0255 codec, > > > > On some machines: > > pin 0x12, 0x90a60170 > > pin 0x14, 0x90170120 > > pin 0x21, 0x02211030 > > > > On another machine: > > pin 0x12, 0x90a60140 > > pin 0x14, 0x90170110 > > pin 0x21, 0x02211020 > > > > > > The def config of the rest pins are same. > > If there is only one example (and only two different options), I think > we should revert this patch and use two different pin-matching quirks > instead. > > After all, ignoring the def assoc/sequence values also means a greater > risk of catching unwanted machines. Better err on the more careful side. > > This is IMO, what do others think? I'm for the safer side, too. Takashi > > > > > > > Regards, > > Hui. > >> > >>> > >>> On Mon, May 26, 2014 at 6:11 PM, David Henningsson > >>> <david.henningsson@canonical.com> wrote: > >>>> (Add Alex Hung to CC) > >>>> > >>>> On 2014-05-26 10:22, Hui Wang wrote: > >>>>> > >>>>> A lot a machine have the same codec, but they have different default > >>>>> pinconf setting just because the def association and sequence is > >>>>> different, as a result they can't share a hda_pintbl[], to overcome > >>>>> it, we don't compare def association and sequence in the pinconf > >>>>> matching. > >>>> > >>>> > >>>> Uhm, really? Alex, does this seem reasonable from a BIOS > >>>> perspective, i e, > >>>> that BIOS people normally would set def association and sequence > >>>> different > >>>> while leaving everything else unchanged? > >>>> > >>>>> > >>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> > >>>>> --- > >>>>> sound/pci/hda/hda_auto_parser.c | 3 ++- > >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/sound/pci/hda/hda_auto_parser.c > >>>>> b/sound/pci/hda/hda_auto_parser.c > >>>>> index b684c6e..3cf9137 100644 > >>>>> --- a/sound/pci/hda/hda_auto_parser.c > >>>>> +++ b/sound/pci/hda/hda_auto_parser.c > >>>>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec > >>>>> *codec, > >>>>> { > >>>>> for (; pins->nid; pins++) { > >>>>> u32 def_conf = snd_hda_codec_get_pincfg(codec, > >>>>> pins->nid); > >>>>> - if (pins->val != def_conf) > >>>>> + u32 mask = 0xffffff00; > >>>>> + if ((pins->val & mask) != (def_conf & mask)) > >>>>> return false; > >>>>> } > >>>>> return true; > >>>>> > >>>> > >>>> -- > >>>> David Henningsson, Canonical Ltd. > >>>> https://launchpad.net/~diwic > >>> > >>> > >>> > >> > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic >
On 05/27/2014 02:53 PM, Takashi Iwai wrote: > At Tue, 27 May 2014 08:47:33 +0200, > David Henningsson wrote: >> >> >> On 2014-05-27 08:40, Hui Wang wrote: >>> On 05/27/2014 12:41 PM, David Henningsson wrote: >>>> >>>> On 2014-05-27 04:25, Alex Hung wrote: >>>>> Hi David, >>>>> >>>>> BIOS today implements verbtable which is provided by codec vendor >>>>> based on hardware design, and it is indeed not uncommon that the >>>>> verbtable includes used pin only and leaves unused pins untouched. >>>> Sure, but those unused pins would then have the same default value >>>> that the codec initializes it with. >>>> >>>> Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the >>>> same verbtable for several machines if they share the same audio >>>> hardware. >>>> >>>> But none of this explains why anyone would just change def association >>>> and sequence value between machines? It makes no sense. >>> So far, I met one example for this case: >>> >>> the Dell laptops with the same 0x10ec0255 codec, >>> >>> On some machines: >>> pin 0x12, 0x90a60170 >>> pin 0x14, 0x90170120 >>> pin 0x21, 0x02211030 >>> >>> On another machine: >>> pin 0x12, 0x90a60140 >>> pin 0x14, 0x90170110 >>> pin 0x21, 0x02211020 >>> >>> >>> The def config of the rest pins are same. >> If there is only one example (and only two different options), I think >> we should revert this patch and use two different pin-matching quirks >> instead. >> >> After all, ignoring the def assoc/sequence values also means a greater >> risk of catching unwanted machines. Better err on the more careful side. >> >> This is IMO, what do others think? > I'm for the safer side, too. > > > Takashi OK, agree to revert it. Regards, Hui. >>> >>> Regards, >>> Hui. >>>>> On Mon, May 26, 2014 at 6:11 PM, David Henningsson >>>>> <david.henningsson@canonical.com> wrote: >>>>>> (Add Alex Hung to CC) >>>>>> >>>>>> On 2014-05-26 10:22, Hui Wang wrote: >>>>>>> A lot a machine have the same codec, but they have different default >>>>>>> pinconf setting just because the def association and sequence is >>>>>>> different, as a result they can't share a hda_pintbl[], to overcome >>>>>>> it, we don't compare def association and sequence in the pinconf >>>>>>> matching. >>>>>> >>>>>> Uhm, really? Alex, does this seem reasonable from a BIOS >>>>>> perspective, i e, >>>>>> that BIOS people normally would set def association and sequence >>>>>> different >>>>>> while leaving everything else unchanged? >>>>>> >>>>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>>>>> --- >>>>>>> sound/pci/hda/hda_auto_parser.c | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/sound/pci/hda/hda_auto_parser.c >>>>>>> b/sound/pci/hda/hda_auto_parser.c >>>>>>> index b684c6e..3cf9137 100644 >>>>>>> --- a/sound/pci/hda/hda_auto_parser.c >>>>>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>>>>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec >>>>>>> *codec, >>>>>>> { >>>>>>> for (; pins->nid; pins++) { >>>>>>> u32 def_conf = snd_hda_codec_get_pincfg(codec, >>>>>>> pins->nid); >>>>>>> - if (pins->val != def_conf) >>>>>>> + u32 mask = 0xffffff00; >>>>>>> + if ((pins->val & mask) != (def_conf & mask)) >>>>>>> return false; >>>>>>> } >>>>>>> return true; >>>>>>> >>>>>> -- >>>>>> David Henningsson, Canonical Ltd. >>>>>> https://launchpad.net/~diwic >>>>> >>>>> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic >> >
On 05/27/2014 02:53 PM, Takashi Iwai wrote: > At Tue, 27 May 2014 08:47:33 +0200, > David Henningsson wrote: >> >> >> On 2014-05-27 08:40, Hui Wang wrote: >>> On 05/27/2014 12:41 PM, David Henningsson wrote: >>>> >>>> On 2014-05-27 04:25, Alex Hung wrote: >>>>> Hi David, >>>>> >>>>> BIOS today implements verbtable which is provided by codec vendor >>>>> based on hardware design, and it is indeed not uncommon that the >>>>> verbtable includes used pin only and leaves unused pins untouched. >>>> Sure, but those unused pins would then have the same default value >>>> that the codec initializes it with. >>>> >>>> Also, it wouldn't be uncommon for BIOS (or codec vendors) to use the >>>> same verbtable for several machines if they share the same audio >>>> hardware. >>>> >>>> But none of this explains why anyone would just change def association >>>> and sequence value between machines? It makes no sense. >>> So far, I met one example for this case: >>> >>> the Dell laptops with the same 0x10ec0255 codec, >>> >>> On some machines: >>> pin 0x12, 0x90a60170 >>> pin 0x14, 0x90170120 >>> pin 0x21, 0x02211030 >>> >>> On another machine: >>> pin 0x12, 0x90a60140 >>> pin 0x14, 0x90170110 >>> pin 0x21, 0x02211020 >>> >>> >>> The def config of the rest pins are same. >> If there is only one example (and only two different options), I think >> we should revert this patch and use two different pin-matching quirks >> instead. >> >> After all, ignoring the def assoc/sequence values also means a greater >> risk of catching unwanted machines. Better err on the more careful side. >> >> This is IMO, what do others think? > I'm for the safer side, too. > > > Takashi I will write a reverting patch within this week, and send it out accompany with the patch moving the some existing machines from old quirk matching table to new quirk matching table. >>> >>> Regards, >>> Hui. >>>>> On Mon, May 26, 2014 at 6:11 PM, David Henningsson >>>>> <david.henningsson@canonical.com> wrote: >>>>>> (Add Alex Hung to CC) >>>>>> >>>>>> On 2014-05-26 10:22, Hui Wang wrote: >>>>>>> A lot a machine have the same codec, but they have different default >>>>>>> pinconf setting just because the def association and sequence is >>>>>>> different, as a result they can't share a hda_pintbl[], to overcome >>>>>>> it, we don't compare def association and sequence in the pinconf >>>>>>> matching. >>>>>> >>>>>> Uhm, really? Alex, does this seem reasonable from a BIOS >>>>>> perspective, i e, >>>>>> that BIOS people normally would set def association and sequence >>>>>> different >>>>>> while leaving everything else unchanged? >>>>>> >>>>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>>>>> --- >>>>>>> sound/pci/hda/hda_auto_parser.c | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/sound/pci/hda/hda_auto_parser.c >>>>>>> b/sound/pci/hda/hda_auto_parser.c >>>>>>> index b684c6e..3cf9137 100644 >>>>>>> --- a/sound/pci/hda/hda_auto_parser.c >>>>>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>>>>> @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec >>>>>>> *codec, >>>>>>> { >>>>>>> for (; pins->nid; pins++) { >>>>>>> u32 def_conf = snd_hda_codec_get_pincfg(codec, >>>>>>> pins->nid); >>>>>>> - if (pins->val != def_conf) >>>>>>> + u32 mask = 0xffffff00; >>>>>>> + if ((pins->val & mask) != (def_conf & mask)) >>>>>>> return false; >>>>>>> } >>>>>>> return true; >>>>>>> >>>>>> -- >>>>>> David Henningsson, Canonical Ltd. >>>>>> https://launchpad.net/~diwic >>>>> >>>>> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic >> >
diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c index b684c6e..3cf9137 100644 --- a/sound/pci/hda/hda_auto_parser.c +++ b/sound/pci/hda/hda_auto_parser.c @@ -844,7 +844,8 @@ static bool pin_config_match(struct hda_codec *codec, { for (; pins->nid; pins++) { u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); - if (pins->val != def_conf) + u32 mask = 0xffffff00; + if ((pins->val & mask) != (def_conf & mask)) return false; } return true;
A lot a machine have the same codec, but they have different default pinconf setting just because the def association and sequence is different, as a result they can't share a hda_pintbl[], to overcome it, we don't compare def association and sequence in the pinconf matching. Signed-off-by: Hui Wang <hui.wang@canonical.com> --- sound/pci/hda/hda_auto_parser.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)