| Message ID | 1399970206-10269-1-git-send-email-david.henningsson@canonical.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
At Tue, 13 May 2014 10:36:46 +0200, David Henningsson wrote: > > Normally, we match on pci ssid only. This works but needs new code > for every machine. To catch more machines in the same quirk, let's add > a new type of quirk, where we match on > 1) PCI Subvendor ID (i e, not device, just vendor) > 2) Codec ID > 3) Pin configuration default > > If all these three match, we could be reasonably certain that the > quirk should apply to the machine even though it might not be the > exact same device. > > In case a quirk matches both the pci ssid quirk and this type of quirk, > then the pci ssid quirk is chosen. > > Signed-off-by: David Henningsson <david.henningsson@canonical.com> > --- > > Hi Takashi, > > Do you think this could be working? If so I'll ask Hui to move some machines over > and come up with a patch for that. Two questions: > > 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk > hopefully matches many machines, we can't use the machine name here. Should I > just skip it? > > 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look > more compact? I actually thought of checking the condition inside the fixup callback. That is, static void dell_fixup_headset(struct hda_codec *codec, const struct hda_fixup *fix, int action) { if (action != HDA_FIXUP_ACT_PRE_PROBE) return; if (codec->vendor_id == xxxx && pin_config_match(codec, xxxx_pincfg) codec->fixup_id = FIXUP_XXXX; else if (...) codec->fixup_id = FIXUP_YYYY; else codec->fixup_id = -1; if (codec->fixup_id != -1) snd_hda_apply_fixup(codec, action); } Of course, this assumes that this fixup is called at the very first, not as a chain. If it may be called as a chain, we need a different way to apply it. Takashi > > sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- > sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- > 2 files changed, 57 insertions(+), 9 deletions(-) > > diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c > index 90d2fda..6904779 100644 > --- a/sound/pci/hda/hda_auto_parser.c > +++ b/sound/pci/hda/hda_auto_parser.c > @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) > } > EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); > > -void snd_hda_pick_fixup(struct hda_codec *codec, > - const struct hda_model_fixup *models, > - const struct snd_pci_quirk *quirk, > - const struct hda_fixup *fixlist) > +static bool pin_config_match(struct hda_codec *codec, > + const struct hda_pintbl *pins) > +{ > + for (; pins->nid; pins++) { > + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); > + if (pins->val != def_conf) > + return false; > + } > + return true; > +} > + > +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > + const struct hda_model_fixup *models, > + const struct snd_pci_quirk *quirk, > + const struct snd_hda_pin_quirk *pin_quirk, > + const struct hda_fixup *fixlist) > { > const struct snd_pci_quirk *q; > int id = -1; > @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, > } > } > > + if (id < 0 && pin_quirk) { > + const struct snd_hda_pin_quirk *pq; > + for (pq = pin_quirk; pq->subvendor; pq++) { > + if (codec->bus->pci->subsystem_vendor != pq->subvendor) > + continue; > + if (codec->vendor_id != pq->codec) > + continue; > + if (pin_config_match(codec, pq->pins)) { > + id = pq->value; > +#ifdef CONFIG_SND_DEBUG_VERBOSE > + name = pq->name; > +#endif > + break; > + } > + } > + } > + > codec->fixup_id = id; > if (id >= 0) { > codec->fixup_list = fixlist; > codec->fixup_name = name; > } > } > -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); > +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); > diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h > index e51d155..41a6817 100644 > --- a/sound/pci/hda/hda_local.h > +++ b/sound/pci/hda/hda_local.h > @@ -425,15 +425,34 @@ enum { > HDA_FIXUP_ACT_FREE, > }; > > +struct snd_hda_pin_quirk { > + unsigned int codec; /* Codec vendor/device ID */ > + unsigned short subvendor; /* PCI subvendor ID */ > + const struct hda_pintbl *pins; /* list of matching pins */ > +#ifdef CONFIG_SND_DEBUG_VERBOSE > + const char *name; > +#endif > + int value; /* quirk value */ > +}; > + > int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); > void snd_hda_apply_verbs(struct hda_codec *codec); > void snd_hda_apply_pincfgs(struct hda_codec *codec, > const struct hda_pintbl *cfg); > void snd_hda_apply_fixup(struct hda_codec *codec, int action); > -void snd_hda_pick_fixup(struct hda_codec *codec, > - const struct hda_model_fixup *models, > - const struct snd_pci_quirk *quirk, > - const struct hda_fixup *fixlist); > +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > + const struct hda_model_fixup *models, > + const struct snd_pci_quirk *quirk, > + const struct snd_hda_pin_quirk *pin_quirk, > + const struct hda_fixup *fixlist); > + > +static inline void snd_hda_pick_fixup(struct hda_codec *codec, > + const struct hda_model_fixup *models, > + const struct snd_pci_quirk *quirk, > + const struct hda_fixup *fixlist) > +{ > + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); > +} > > /* > * unsolicited event handler > -- > 1.9.1 >
On 2014-05-13 11:06, Takashi Iwai wrote: > At Tue, 13 May 2014 10:36:46 +0200, > David Henningsson wrote: >> >> Normally, we match on pci ssid only. This works but needs new code >> for every machine. To catch more machines in the same quirk, let's add >> a new type of quirk, where we match on >> 1) PCI Subvendor ID (i e, not device, just vendor) >> 2) Codec ID >> 3) Pin configuration default >> >> If all these three match, we could be reasonably certain that the >> quirk should apply to the machine even though it might not be the >> exact same device. >> >> In case a quirk matches both the pci ssid quirk and this type of quirk, >> then the pci ssid quirk is chosen. >> >> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >> --- >> >> Hi Takashi, >> >> Do you think this could be working? If so I'll ask Hui to move some machines over >> and come up with a patch for that. Two questions: >> >> 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk >> hopefully matches many machines, we can't use the machine name here. Should I >> just skip it? >> >> 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look >> more compact? > > I actually thought of checking the condition inside the fixup > callback. That is, > > static void dell_fixup_headset(struct hda_codec *codec, > const struct hda_fixup *fix, int action) > { > if (action != HDA_FIXUP_ACT_PRE_PROBE) > return; > if (codec->vendor_id == xxxx && > pin_config_match(codec, xxxx_pincfg) > codec->fixup_id = FIXUP_XXXX; > else if (...) > codec->fixup_id = FIXUP_YYYY; > else > codec->fixup_id = -1; > if (codec->fixup_id != -1) > snd_hda_apply_fixup(codec, action); > } > > Of course, this assumes that this fixup is called at the very first, > not as a chain. If it may be called as a chain, we need a different > way to apply it. Hmm, still, what do you think about using my solution instead? It's more generic and could become useful in many different contexts in the future, not just matching headsets. And it integrates nicely with the existing quirk system as it is just a new method for matching quirks. Your solution seems more confusing to me (having one quirk that just selects another quirk?). Besides, could you explain how you expect this to be called? Are you suggesting a matching line like: SND_PCI_QUIRK_VENDOR(0x1028, "Dell", ...) ? > > > Takashi > >> >> sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- >> sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- >> 2 files changed, 57 insertions(+), 9 deletions(-) >> >> diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c >> index 90d2fda..6904779 100644 >> --- a/sound/pci/hda/hda_auto_parser.c >> +++ b/sound/pci/hda/hda_auto_parser.c >> @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) >> } >> EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); >> >> -void snd_hda_pick_fixup(struct hda_codec *codec, >> - const struct hda_model_fixup *models, >> - const struct snd_pci_quirk *quirk, >> - const struct hda_fixup *fixlist) >> +static bool pin_config_match(struct hda_codec *codec, >> + const struct hda_pintbl *pins) >> +{ >> + for (; pins->nid; pins++) { >> + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); >> + if (pins->val != def_conf) >> + return false; >> + } >> + return true; >> +} >> + >> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >> + const struct hda_model_fixup *models, >> + const struct snd_pci_quirk *quirk, >> + const struct snd_hda_pin_quirk *pin_quirk, >> + const struct hda_fixup *fixlist) >> { >> const struct snd_pci_quirk *q; >> int id = -1; >> @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, >> } >> } >> >> + if (id < 0 && pin_quirk) { >> + const struct snd_hda_pin_quirk *pq; >> + for (pq = pin_quirk; pq->subvendor; pq++) { >> + if (codec->bus->pci->subsystem_vendor != pq->subvendor) >> + continue; >> + if (codec->vendor_id != pq->codec) >> + continue; >> + if (pin_config_match(codec, pq->pins)) { >> + id = pq->value; >> +#ifdef CONFIG_SND_DEBUG_VERBOSE >> + name = pq->name; >> +#endif >> + break; >> + } >> + } >> + } >> + >> codec->fixup_id = id; >> if (id >= 0) { >> codec->fixup_list = fixlist; >> codec->fixup_name = name; >> } >> } >> -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); >> +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); >> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h >> index e51d155..41a6817 100644 >> --- a/sound/pci/hda/hda_local.h >> +++ b/sound/pci/hda/hda_local.h >> @@ -425,15 +425,34 @@ enum { >> HDA_FIXUP_ACT_FREE, >> }; >> >> +struct snd_hda_pin_quirk { >> + unsigned int codec; /* Codec vendor/device ID */ >> + unsigned short subvendor; /* PCI subvendor ID */ >> + const struct hda_pintbl *pins; /* list of matching pins */ >> +#ifdef CONFIG_SND_DEBUG_VERBOSE >> + const char *name; >> +#endif >> + int value; /* quirk value */ >> +}; >> + >> int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); >> void snd_hda_apply_verbs(struct hda_codec *codec); >> void snd_hda_apply_pincfgs(struct hda_codec *codec, >> const struct hda_pintbl *cfg); >> void snd_hda_apply_fixup(struct hda_codec *codec, int action); >> -void snd_hda_pick_fixup(struct hda_codec *codec, >> - const struct hda_model_fixup *models, >> - const struct snd_pci_quirk *quirk, >> - const struct hda_fixup *fixlist); >> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >> + const struct hda_model_fixup *models, >> + const struct snd_pci_quirk *quirk, >> + const struct snd_hda_pin_quirk *pin_quirk, >> + const struct hda_fixup *fixlist); >> + >> +static inline void snd_hda_pick_fixup(struct hda_codec *codec, >> + const struct hda_model_fixup *models, >> + const struct snd_pci_quirk *quirk, >> + const struct hda_fixup *fixlist) >> +{ >> + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); >> +} >> >> /* >> * unsolicited event handler >> -- >> 1.9.1 >> >
At Tue, 13 May 2014 11:39:02 +0200, David Henningsson wrote: > > > > On 2014-05-13 11:06, Takashi Iwai wrote: > > At Tue, 13 May 2014 10:36:46 +0200, > > David Henningsson wrote: > >> > >> Normally, we match on pci ssid only. This works but needs new code > >> for every machine. To catch more machines in the same quirk, let's add > >> a new type of quirk, where we match on > >> 1) PCI Subvendor ID (i e, not device, just vendor) > >> 2) Codec ID > >> 3) Pin configuration default > >> > >> If all these three match, we could be reasonably certain that the > >> quirk should apply to the machine even though it might not be the > >> exact same device. > >> > >> In case a quirk matches both the pci ssid quirk and this type of quirk, > >> then the pci ssid quirk is chosen. > >> > >> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >> --- > >> > >> Hi Takashi, > >> > >> Do you think this could be working? If so I'll ask Hui to move some machines over > >> and come up with a patch for that. Two questions: > >> > >> 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk > >> hopefully matches many machines, we can't use the machine name here. Should I > >> just skip it? > >> > >> 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look > >> more compact? > > > > I actually thought of checking the condition inside the fixup > > callback. That is, > > > > static void dell_fixup_headset(struct hda_codec *codec, > > const struct hda_fixup *fix, int action) > > { > > if (action != HDA_FIXUP_ACT_PRE_PROBE) > > return; > > if (codec->vendor_id == xxxx && > > pin_config_match(codec, xxxx_pincfg) > > codec->fixup_id = FIXUP_XXXX; > > else if (...) > > codec->fixup_id = FIXUP_YYYY; > > else > > codec->fixup_id = -1; > > if (codec->fixup_id != -1) > > snd_hda_apply_fixup(codec, action); > > } > > > > Of course, this assumes that this fixup is called at the very first, > > not as a chain. If it may be called as a chain, we need a different > > way to apply it. > > Hmm, still, what do you think about using my solution instead? It's more > generic and could become useful in many different contexts in the > future, not just matching headsets. And it integrates nicely with the > existing quirk system as it is just a new method for matching quirks. Maybe better not to change snd_hda_pick_fixup(), but add the pincfg lookup manually. The difference is that you know the order of fixup lookup in that way while doing it in snd_hda_pick_fixup() hides the order; e.g. it can be called even if you want to pick up it before the PCI SSID matching. > Your solution seems more confusing to me (having one quirk that just > selects another quirk?). Besides, could you explain how you expect this > to be called? Are you suggesting a matching line like: > SND_PCI_QUIRK_VENDOR(0x1028, "Dell", ...) ? Yes. Although the implementation above is the top fixup id only, a general idea was to allow it chainable, so that it can be applied as a bottom line with other extra fixups. Takashi > > > > > > > Takashi > > > >> > >> sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- > >> sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- > >> 2 files changed, 57 insertions(+), 9 deletions(-) > >> > >> diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c > >> index 90d2fda..6904779 100644 > >> --- a/sound/pci/hda/hda_auto_parser.c > >> +++ b/sound/pci/hda/hda_auto_parser.c > >> @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) > >> } > >> EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); > >> > >> -void snd_hda_pick_fixup(struct hda_codec *codec, > >> - const struct hda_model_fixup *models, > >> - const struct snd_pci_quirk *quirk, > >> - const struct hda_fixup *fixlist) > >> +static bool pin_config_match(struct hda_codec *codec, > >> + const struct hda_pintbl *pins) > >> +{ > >> + for (; pins->nid; pins++) { > >> + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); > >> + if (pins->val != def_conf) > >> + return false; > >> + } > >> + return true; > >> +} > >> + > >> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > >> + const struct hda_model_fixup *models, > >> + const struct snd_pci_quirk *quirk, > >> + const struct snd_hda_pin_quirk *pin_quirk, > >> + const struct hda_fixup *fixlist) > >> { > >> const struct snd_pci_quirk *q; > >> int id = -1; > >> @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, > >> } > >> } > >> > >> + if (id < 0 && pin_quirk) { > >> + const struct snd_hda_pin_quirk *pq; > >> + for (pq = pin_quirk; pq->subvendor; pq++) { > >> + if (codec->bus->pci->subsystem_vendor != pq->subvendor) > >> + continue; > >> + if (codec->vendor_id != pq->codec) > >> + continue; > >> + if (pin_config_match(codec, pq->pins)) { > >> + id = pq->value; > >> +#ifdef CONFIG_SND_DEBUG_VERBOSE > >> + name = pq->name; > >> +#endif > >> + break; > >> + } > >> + } > >> + } > >> + > >> codec->fixup_id = id; > >> if (id >= 0) { > >> codec->fixup_list = fixlist; > >> codec->fixup_name = name; > >> } > >> } > >> -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); > >> +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); > >> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h > >> index e51d155..41a6817 100644 > >> --- a/sound/pci/hda/hda_local.h > >> +++ b/sound/pci/hda/hda_local.h > >> @@ -425,15 +425,34 @@ enum { > >> HDA_FIXUP_ACT_FREE, > >> }; > >> > >> +struct snd_hda_pin_quirk { > >> + unsigned int codec; /* Codec vendor/device ID */ > >> + unsigned short subvendor; /* PCI subvendor ID */ > >> + const struct hda_pintbl *pins; /* list of matching pins */ > >> +#ifdef CONFIG_SND_DEBUG_VERBOSE > >> + const char *name; > >> +#endif > >> + int value; /* quirk value */ > >> +}; > >> + > >> int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); > >> void snd_hda_apply_verbs(struct hda_codec *codec); > >> void snd_hda_apply_pincfgs(struct hda_codec *codec, > >> const struct hda_pintbl *cfg); > >> void snd_hda_apply_fixup(struct hda_codec *codec, int action); > >> -void snd_hda_pick_fixup(struct hda_codec *codec, > >> - const struct hda_model_fixup *models, > >> - const struct snd_pci_quirk *quirk, > >> - const struct hda_fixup *fixlist); > >> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > >> + const struct hda_model_fixup *models, > >> + const struct snd_pci_quirk *quirk, > >> + const struct snd_hda_pin_quirk *pin_quirk, > >> + const struct hda_fixup *fixlist); > >> + > >> +static inline void snd_hda_pick_fixup(struct hda_codec *codec, > >> + const struct hda_model_fixup *models, > >> + const struct snd_pci_quirk *quirk, > >> + const struct hda_fixup *fixlist) > >> +{ > >> + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); > >> +} > >> > >> /* > >> * unsolicited event handler > >> -- > >> 1.9.1 > >> > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic >
On 2014-05-13 11:49, Takashi Iwai wrote: > At Tue, 13 May 2014 11:39:02 +0200, > David Henningsson wrote: >> >> >> >> On 2014-05-13 11:06, Takashi Iwai wrote: >>> At Tue, 13 May 2014 10:36:46 +0200, >>> David Henningsson wrote: >>>> >>>> Normally, we match on pci ssid only. This works but needs new code >>>> for every machine. To catch more machines in the same quirk, let's add >>>> a new type of quirk, where we match on >>>> 1) PCI Subvendor ID (i e, not device, just vendor) >>>> 2) Codec ID >>>> 3) Pin configuration default >>>> >>>> If all these three match, we could be reasonably certain that the >>>> quirk should apply to the machine even though it might not be the >>>> exact same device. >>>> >>>> In case a quirk matches both the pci ssid quirk and this type of quirk, >>>> then the pci ssid quirk is chosen. >>>> >>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >>>> --- >>>> >>>> Hi Takashi, >>>> >>>> Do you think this could be working? If so I'll ask Hui to move some machines over >>>> and come up with a patch for that. Two questions: >>>> >>>> 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk >>>> hopefully matches many machines, we can't use the machine name here. Should I >>>> just skip it? >>>> >>>> 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look >>>> more compact? >>> >>> I actually thought of checking the condition inside the fixup >>> callback. That is, >>> >>> static void dell_fixup_headset(struct hda_codec *codec, >>> const struct hda_fixup *fix, int action) >>> { >>> if (action != HDA_FIXUP_ACT_PRE_PROBE) >>> return; >>> if (codec->vendor_id == xxxx && >>> pin_config_match(codec, xxxx_pincfg) >>> codec->fixup_id = FIXUP_XXXX; >>> else if (...) >>> codec->fixup_id = FIXUP_YYYY; >>> else >>> codec->fixup_id = -1; >>> if (codec->fixup_id != -1) >>> snd_hda_apply_fixup(codec, action); >>> } >>> >>> Of course, this assumes that this fixup is called at the very first, >>> not as a chain. If it may be called as a chain, we need a different >>> way to apply it. >> >> Hmm, still, what do you think about using my solution instead? It's more >> generic and could become useful in many different contexts in the >> future, not just matching headsets. And it integrates nicely with the >> existing quirk system as it is just a new method for matching quirks. > > Maybe better not to change snd_hda_pick_fixup(), but add the pincfg > lookup manually. The difference is that you know the order of fixup > lookup in that way while doing it in snd_hda_pick_fixup() hides the > order; e.g. it can be called even if you want to pick up it before the > PCI SSID matching. I just attempted to rewrite the patch according to this, but I didn't like the result. I didn't find an elegant way to handle "nofixup" (you need to check "nofixup" in both pick_fixup functions in order to handle both orders), and I personally prefer to have one consistent ordering across all callers instead of letting the caller decide the order. >> Your solution seems more confusing to me (having one quirk that just >> selects another quirk?). Besides, could you explain how you expect this >> to be called? Are you suggesting a matching line like: >> SND_PCI_QUIRK_VENDOR(0x1028, "Dell", ...) ? > > Yes. Although the implementation above is the top fixup id only, a > general idea was to allow it chainable, so that it can be applied as a > bottom line with other extra fixups. > > > Takashi > >> >>> >>> >>> Takashi >>> >>>> >>>> sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- >>>> sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- >>>> 2 files changed, 57 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c >>>> index 90d2fda..6904779 100644 >>>> --- a/sound/pci/hda/hda_auto_parser.c >>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>> @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) >>>> } >>>> EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); >>>> >>>> -void snd_hda_pick_fixup(struct hda_codec *codec, >>>> - const struct hda_model_fixup *models, >>>> - const struct snd_pci_quirk *quirk, >>>> - const struct hda_fixup *fixlist) >>>> +static bool pin_config_match(struct hda_codec *codec, >>>> + const struct hda_pintbl *pins) >>>> +{ >>>> + for (; pins->nid; pins++) { >>>> + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); >>>> + if (pins->val != def_conf) >>>> + return false; >>>> + } >>>> + return true; >>>> +} >>>> + >>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >>>> + const struct hda_model_fixup *models, >>>> + const struct snd_pci_quirk *quirk, >>>> + const struct snd_hda_pin_quirk *pin_quirk, >>>> + const struct hda_fixup *fixlist) >>>> { >>>> const struct snd_pci_quirk *q; >>>> int id = -1; >>>> @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, >>>> } >>>> } >>>> >>>> + if (id < 0 && pin_quirk) { >>>> + const struct snd_hda_pin_quirk *pq; >>>> + for (pq = pin_quirk; pq->subvendor; pq++) { >>>> + if (codec->bus->pci->subsystem_vendor != pq->subvendor) >>>> + continue; >>>> + if (codec->vendor_id != pq->codec) >>>> + continue; >>>> + if (pin_config_match(codec, pq->pins)) { >>>> + id = pq->value; >>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE >>>> + name = pq->name; >>>> +#endif >>>> + break; >>>> + } >>>> + } >>>> + } >>>> + >>>> codec->fixup_id = id; >>>> if (id >= 0) { >>>> codec->fixup_list = fixlist; >>>> codec->fixup_name = name; >>>> } >>>> } >>>> -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); >>>> +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); >>>> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h >>>> index e51d155..41a6817 100644 >>>> --- a/sound/pci/hda/hda_local.h >>>> +++ b/sound/pci/hda/hda_local.h >>>> @@ -425,15 +425,34 @@ enum { >>>> HDA_FIXUP_ACT_FREE, >>>> }; >>>> >>>> +struct snd_hda_pin_quirk { >>>> + unsigned int codec; /* Codec vendor/device ID */ >>>> + unsigned short subvendor; /* PCI subvendor ID */ >>>> + const struct hda_pintbl *pins; /* list of matching pins */ >>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE >>>> + const char *name; >>>> +#endif >>>> + int value; /* quirk value */ >>>> +}; >>>> + >>>> int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); >>>> void snd_hda_apply_verbs(struct hda_codec *codec); >>>> void snd_hda_apply_pincfgs(struct hda_codec *codec, >>>> const struct hda_pintbl *cfg); >>>> void snd_hda_apply_fixup(struct hda_codec *codec, int action); >>>> -void snd_hda_pick_fixup(struct hda_codec *codec, >>>> - const struct hda_model_fixup *models, >>>> - const struct snd_pci_quirk *quirk, >>>> - const struct hda_fixup *fixlist); >>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >>>> + const struct hda_model_fixup *models, >>>> + const struct snd_pci_quirk *quirk, >>>> + const struct snd_hda_pin_quirk *pin_quirk, >>>> + const struct hda_fixup *fixlist); >>>> + >>>> +static inline void snd_hda_pick_fixup(struct hda_codec *codec, >>>> + const struct hda_model_fixup *models, >>>> + const struct snd_pci_quirk *quirk, >>>> + const struct hda_fixup *fixlist) >>>> +{ >>>> + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); >>>> +} >>>> >>>> /* >>>> * unsolicited event handler >>>> -- >>>> 1.9.1 >>>> >>> >> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic >> >
At Fri, 16 May 2014 08:11:35 +0200, David Henningsson wrote: > > > > On 2014-05-13 11:49, Takashi Iwai wrote: > > At Tue, 13 May 2014 11:39:02 +0200, > > David Henningsson wrote: > >> > >> > >> > >> On 2014-05-13 11:06, Takashi Iwai wrote: > >>> At Tue, 13 May 2014 10:36:46 +0200, > >>> David Henningsson wrote: > >>>> > >>>> Normally, we match on pci ssid only. This works but needs new code > >>>> for every machine. To catch more machines in the same quirk, let's add > >>>> a new type of quirk, where we match on > >>>> 1) PCI Subvendor ID (i e, not device, just vendor) > >>>> 2) Codec ID > >>>> 3) Pin configuration default > >>>> > >>>> If all these three match, we could be reasonably certain that the > >>>> quirk should apply to the machine even though it might not be the > >>>> exact same device. > >>>> > >>>> In case a quirk matches both the pci ssid quirk and this type of quirk, > >>>> then the pci ssid quirk is chosen. > >>>> > >>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >>>> --- > >>>> > >>>> Hi Takashi, > >>>> > >>>> Do you think this could be working? If so I'll ask Hui to move some machines over > >>>> and come up with a patch for that. Two questions: > >>>> > >>>> 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk > >>>> hopefully matches many machines, we can't use the machine name here. Should I > >>>> just skip it? > >>>> > >>>> 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look > >>>> more compact? > >>> > >>> I actually thought of checking the condition inside the fixup > >>> callback. That is, > >>> > >>> static void dell_fixup_headset(struct hda_codec *codec, > >>> const struct hda_fixup *fix, int action) > >>> { > >>> if (action != HDA_FIXUP_ACT_PRE_PROBE) > >>> return; > >>> if (codec->vendor_id == xxxx && > >>> pin_config_match(codec, xxxx_pincfg) > >>> codec->fixup_id = FIXUP_XXXX; > >>> else if (...) > >>> codec->fixup_id = FIXUP_YYYY; > >>> else > >>> codec->fixup_id = -1; > >>> if (codec->fixup_id != -1) > >>> snd_hda_apply_fixup(codec, action); > >>> } > >>> > >>> Of course, this assumes that this fixup is called at the very first, > >>> not as a chain. If it may be called as a chain, we need a different > >>> way to apply it. > >> > >> Hmm, still, what do you think about using my solution instead? It's more > >> generic and could become useful in many different contexts in the > >> future, not just matching headsets. And it integrates nicely with the > >> existing quirk system as it is just a new method for matching quirks. > > > > Maybe better not to change snd_hda_pick_fixup(), but add the pincfg > > lookup manually. The difference is that you know the order of fixup > > lookup in that way while doing it in snd_hda_pick_fixup() hides the > > order; e.g. it can be called even if you want to pick up it before the > > PCI SSID matching. > > I just attempted to rewrite the patch according to this, but I didn't > like the result. I didn't find an elegant way to handle "nofixup" (you > need to check "nofixup" in both pick_fixup functions in order to handle > both orders), and I personally prefer to have one consistent ordering > across all callers instead of letting the caller decide the order. Well, "nofixup" matching is one good point. But it doesn't mean that it's better to put a style in your patch, but rather it means that it's a generic problem even for the existing scheme. For example, there is already a similar workaround in patch_sigmatel.c for the old iMacs. Since Apple doesn't give any useful PCI SSID, we need to check codec SSIDs. But even some machines give bogus codec SSIDs (0000:0100) due to BIOS, so we have to filter out at first with PCI SSID, then apply codec SSID check. What can we do for fixing both cases (iMac and Dell)? We can't put every logic in a flat way into pick_fixup(). Both tells us: - we need multi-step filtering - some of filters can be reused (e.g. filtering via PCI vendor, some device SSID mask) - some filter can be arbitrary (e.g. pincfg matching) This implies that the need isn't to extend the current flat fixup parser but redesign somehow to be layered. Otherwise, we'll see the same trouble once if we need to add another matching method (OF, ACPI, DMI, whatever). So, until this is properly implemented, I'd like not to touch the the helper side, but keep changes inside the codec driver locally. thanks, Takashi > > >> Your solution seems more confusing to me (having one quirk that just > >> selects another quirk?). Besides, could you explain how you expect this > >> to be called? Are you suggesting a matching line like: > >> SND_PCI_QUIRK_VENDOR(0x1028, "Dell", ...) ? > > > > Yes. Although the implementation above is the top fixup id only, a > > general idea was to allow it chainable, so that it can be applied as a > > bottom line with other extra fixups. > > > > > > Takashi > > > >> > >>> > >>> > >>> Takashi > >>> > >>>> > >>>> sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- > >>>> sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- > >>>> 2 files changed, 57 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c > >>>> index 90d2fda..6904779 100644 > >>>> --- a/sound/pci/hda/hda_auto_parser.c > >>>> +++ b/sound/pci/hda/hda_auto_parser.c > >>>> @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) > >>>> } > >>>> EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); > >>>> > >>>> -void snd_hda_pick_fixup(struct hda_codec *codec, > >>>> - const struct hda_model_fixup *models, > >>>> - const struct snd_pci_quirk *quirk, > >>>> - const struct hda_fixup *fixlist) > >>>> +static bool pin_config_match(struct hda_codec *codec, > >>>> + const struct hda_pintbl *pins) > >>>> +{ > >>>> + for (; pins->nid; pins++) { > >>>> + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); > >>>> + if (pins->val != def_conf) > >>>> + return false; > >>>> + } > >>>> + return true; > >>>> +} > >>>> + > >>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > >>>> + const struct hda_model_fixup *models, > >>>> + const struct snd_pci_quirk *quirk, > >>>> + const struct snd_hda_pin_quirk *pin_quirk, > >>>> + const struct hda_fixup *fixlist) > >>>> { > >>>> const struct snd_pci_quirk *q; > >>>> int id = -1; > >>>> @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, > >>>> } > >>>> } > >>>> > >>>> + if (id < 0 && pin_quirk) { > >>>> + const struct snd_hda_pin_quirk *pq; > >>>> + for (pq = pin_quirk; pq->subvendor; pq++) { > >>>> + if (codec->bus->pci->subsystem_vendor != pq->subvendor) > >>>> + continue; > >>>> + if (codec->vendor_id != pq->codec) > >>>> + continue; > >>>> + if (pin_config_match(codec, pq->pins)) { > >>>> + id = pq->value; > >>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE > >>>> + name = pq->name; > >>>> +#endif > >>>> + break; > >>>> + } > >>>> + } > >>>> + } > >>>> + > >>>> codec->fixup_id = id; > >>>> if (id >= 0) { > >>>> codec->fixup_list = fixlist; > >>>> codec->fixup_name = name; > >>>> } > >>>> } > >>>> -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); > >>>> +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); > >>>> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h > >>>> index e51d155..41a6817 100644 > >>>> --- a/sound/pci/hda/hda_local.h > >>>> +++ b/sound/pci/hda/hda_local.h > >>>> @@ -425,15 +425,34 @@ enum { > >>>> HDA_FIXUP_ACT_FREE, > >>>> }; > >>>> > >>>> +struct snd_hda_pin_quirk { > >>>> + unsigned int codec; /* Codec vendor/device ID */ > >>>> + unsigned short subvendor; /* PCI subvendor ID */ > >>>> + const struct hda_pintbl *pins; /* list of matching pins */ > >>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE > >>>> + const char *name; > >>>> +#endif > >>>> + int value; /* quirk value */ > >>>> +}; > >>>> + > >>>> int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); > >>>> void snd_hda_apply_verbs(struct hda_codec *codec); > >>>> void snd_hda_apply_pincfgs(struct hda_codec *codec, > >>>> const struct hda_pintbl *cfg); > >>>> void snd_hda_apply_fixup(struct hda_codec *codec, int action); > >>>> -void snd_hda_pick_fixup(struct hda_codec *codec, > >>>> - const struct hda_model_fixup *models, > >>>> - const struct snd_pci_quirk *quirk, > >>>> - const struct hda_fixup *fixlist); > >>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, > >>>> + const struct hda_model_fixup *models, > >>>> + const struct snd_pci_quirk *quirk, > >>>> + const struct snd_hda_pin_quirk *pin_quirk, > >>>> + const struct hda_fixup *fixlist); > >>>> + > >>>> +static inline void snd_hda_pick_fixup(struct hda_codec *codec, > >>>> + const struct hda_model_fixup *models, > >>>> + const struct snd_pci_quirk *quirk, > >>>> + const struct hda_fixup *fixlist) > >>>> +{ > >>>> + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); > >>>> +} > >>>> > >>>> /* > >>>> * unsolicited event handler > >>>> -- > >>>> 1.9.1 > >>>> > >>> > >> > >> -- > >> David Henningsson, Canonical Ltd. > >> https://launchpad.net/~diwic > >> > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic >
On 2014-05-16 08:44, Takashi Iwai wrote: > At Fri, 16 May 2014 08:11:35 +0200, > David Henningsson wrote: >> >> >> >> On 2014-05-13 11:49, Takashi Iwai wrote: >>> At Tue, 13 May 2014 11:39:02 +0200, >>> David Henningsson wrote: >>>> >>>> >>>> >>>> On 2014-05-13 11:06, Takashi Iwai wrote: >>>>> At Tue, 13 May 2014 10:36:46 +0200, >>>>> David Henningsson wrote: >>>>>> >>>>>> Normally, we match on pci ssid only. This works but needs new code >>>>>> for every machine. To catch more machines in the same quirk, let's add >>>>>> a new type of quirk, where we match on >>>>>> 1) PCI Subvendor ID (i e, not device, just vendor) >>>>>> 2) Codec ID >>>>>> 3) Pin configuration default >>>>>> >>>>>> If all these three match, we could be reasonably certain that the >>>>>> quirk should apply to the machine even though it might not be the >>>>>> exact same device. >>>>>> >>>>>> In case a quirk matches both the pci ssid quirk and this type of quirk, >>>>>> then the pci ssid quirk is chosen. >>>>>> >>>>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >>>>>> --- >>>>>> >>>>>> Hi Takashi, >>>>>> >>>>>> Do you think this could be working? If so I'll ask Hui to move some machines over >>>>>> and come up with a patch for that. Two questions: >>>>>> >>>>>> 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk >>>>>> hopefully matches many machines, we can't use the machine name here. Should I >>>>>> just skip it? >>>>>> >>>>>> 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look >>>>>> more compact? >>>>> >>>>> I actually thought of checking the condition inside the fixup >>>>> callback. That is, >>>>> >>>>> static void dell_fixup_headset(struct hda_codec *codec, >>>>> const struct hda_fixup *fix, int action) >>>>> { >>>>> if (action != HDA_FIXUP_ACT_PRE_PROBE) >>>>> return; >>>>> if (codec->vendor_id == xxxx && >>>>> pin_config_match(codec, xxxx_pincfg) >>>>> codec->fixup_id = FIXUP_XXXX; >>>>> else if (...) >>>>> codec->fixup_id = FIXUP_YYYY; >>>>> else >>>>> codec->fixup_id = -1; >>>>> if (codec->fixup_id != -1) >>>>> snd_hda_apply_fixup(codec, action); >>>>> } >>>>> >>>>> Of course, this assumes that this fixup is called at the very first, >>>>> not as a chain. If it may be called as a chain, we need a different >>>>> way to apply it. >>>> >>>> Hmm, still, what do you think about using my solution instead? It's more >>>> generic and could become useful in many different contexts in the >>>> future, not just matching headsets. And it integrates nicely with the >>>> existing quirk system as it is just a new method for matching quirks. >>> >>> Maybe better not to change snd_hda_pick_fixup(), but add the pincfg >>> lookup manually. The difference is that you know the order of fixup >>> lookup in that way while doing it in snd_hda_pick_fixup() hides the >>> order; e.g. it can be called even if you want to pick up it before the >>> PCI SSID matching. >> >> I just attempted to rewrite the patch according to this, but I didn't >> like the result. I didn't find an elegant way to handle "nofixup" (you >> need to check "nofixup" in both pick_fixup functions in order to handle >> both orders), and I personally prefer to have one consistent ordering >> across all callers instead of letting the caller decide the order. > > Well, "nofixup" matching is one good point. But it doesn't mean that > it's better to put a style in your patch, but rather it means that > it's a generic problem even for the existing scheme. > > For example, there is already a similar workaround in patch_sigmatel.c > for the old iMacs. Since Apple doesn't give any useful PCI SSID, we > need to check codec SSIDs. But even some machines give bogus codec > SSIDs (0000:0100) due to BIOS, so we have to filter out at first with > PCI SSID, then apply codec SSID check. > > What can we do for fixing both cases (iMac and Dell)? We can't put > every logic in a flat way into pick_fixup(). Both tells us: > - we need multi-step filtering > - some of filters can be reused (e.g. filtering via PCI vendor, some > device SSID mask) > - some filter can be arbitrary (e.g. pincfg matching) > > This implies that the need isn't to extend the current flat fixup > parser but redesign somehow to be layered. Otherwise, we'll see the > same trouble once if we need to add another matching method (OF, ACPI, > DMI, whatever). > > So, until this is properly implemented, I'd like not to touch the > the helper side, but keep changes inside the codec driver locally. Ok, so let's do it "layered" then, so it will be more flexible for the caller what type of filtering is needed. Will post patches now. I left the iMac codec SSID stuff up to you to refactor, because I'm not sure exactly how you want it done. > > > thanks, > > Takashi > >> >>>> Your solution seems more confusing to me (having one quirk that just >>>> selects another quirk?). Besides, could you explain how you expect this >>>> to be called? Are you suggesting a matching line like: >>>> SND_PCI_QUIRK_VENDOR(0x1028, "Dell", ...) ? >>> >>> Yes. Although the implementation above is the top fixup id only, a >>> general idea was to allow it chainable, so that it can be applied as a >>> bottom line with other extra fixups. >>> >>> >>> Takashi >>> >>>> >>>>> >>>>> >>>>> Takashi >>>>> >>>>>> >>>>>> sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- >>>>>> sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- >>>>>> 2 files changed, 57 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c >>>>>> index 90d2fda..6904779 100644 >>>>>> --- a/sound/pci/hda/hda_auto_parser.c >>>>>> +++ b/sound/pci/hda/hda_auto_parser.c >>>>>> @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); >>>>>> >>>>>> -void snd_hda_pick_fixup(struct hda_codec *codec, >>>>>> - const struct hda_model_fixup *models, >>>>>> - const struct snd_pci_quirk *quirk, >>>>>> - const struct hda_fixup *fixlist) >>>>>> +static bool pin_config_match(struct hda_codec *codec, >>>>>> + const struct hda_pintbl *pins) >>>>>> +{ >>>>>> + for (; pins->nid; pins++) { >>>>>> + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); >>>>>> + if (pins->val != def_conf) >>>>>> + return false; >>>>>> + } >>>>>> + return true; >>>>>> +} >>>>>> + >>>>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >>>>>> + const struct hda_model_fixup *models, >>>>>> + const struct snd_pci_quirk *quirk, >>>>>> + const struct snd_hda_pin_quirk *pin_quirk, >>>>>> + const struct hda_fixup *fixlist) >>>>>> { >>>>>> const struct snd_pci_quirk *q; >>>>>> int id = -1; >>>>>> @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, >>>>>> } >>>>>> } >>>>>> >>>>>> + if (id < 0 && pin_quirk) { >>>>>> + const struct snd_hda_pin_quirk *pq; >>>>>> + for (pq = pin_quirk; pq->subvendor; pq++) { >>>>>> + if (codec->bus->pci->subsystem_vendor != pq->subvendor) >>>>>> + continue; >>>>>> + if (codec->vendor_id != pq->codec) >>>>>> + continue; >>>>>> + if (pin_config_match(codec, pq->pins)) { >>>>>> + id = pq->value; >>>>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE >>>>>> + name = pq->name; >>>>>> +#endif >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + } >>>>>> + >>>>>> codec->fixup_id = id; >>>>>> if (id >= 0) { >>>>>> codec->fixup_list = fixlist; >>>>>> codec->fixup_name = name; >>>>>> } >>>>>> } >>>>>> -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); >>>>>> +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); >>>>>> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h >>>>>> index e51d155..41a6817 100644 >>>>>> --- a/sound/pci/hda/hda_local.h >>>>>> +++ b/sound/pci/hda/hda_local.h >>>>>> @@ -425,15 +425,34 @@ enum { >>>>>> HDA_FIXUP_ACT_FREE, >>>>>> }; >>>>>> >>>>>> +struct snd_hda_pin_quirk { >>>>>> + unsigned int codec; /* Codec vendor/device ID */ >>>>>> + unsigned short subvendor; /* PCI subvendor ID */ >>>>>> + const struct hda_pintbl *pins; /* list of matching pins */ >>>>>> +#ifdef CONFIG_SND_DEBUG_VERBOSE >>>>>> + const char *name; >>>>>> +#endif >>>>>> + int value; /* quirk value */ >>>>>> +}; >>>>>> + >>>>>> int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); >>>>>> void snd_hda_apply_verbs(struct hda_codec *codec); >>>>>> void snd_hda_apply_pincfgs(struct hda_codec *codec, >>>>>> const struct hda_pintbl *cfg); >>>>>> void snd_hda_apply_fixup(struct hda_codec *codec, int action); >>>>>> -void snd_hda_pick_fixup(struct hda_codec *codec, >>>>>> - const struct hda_model_fixup *models, >>>>>> - const struct snd_pci_quirk *quirk, >>>>>> - const struct hda_fixup *fixlist); >>>>>> +void snd_hda_pick_pin_fixup(struct hda_codec *codec, >>>>>> + const struct hda_model_fixup *models, >>>>>> + const struct snd_pci_quirk *quirk, >>>>>> + const struct snd_hda_pin_quirk *pin_quirk, >>>>>> + const struct hda_fixup *fixlist); >>>>>> + >>>>>> +static inline void snd_hda_pick_fixup(struct hda_codec *codec, >>>>>> + const struct hda_model_fixup *models, >>>>>> + const struct snd_pci_quirk *quirk, >>>>>> + const struct hda_fixup *fixlist) >>>>>> +{ >>>>>> + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); >>>>>> +} >>>>>> >>>>>> /* >>>>>> * unsolicited event handler >>>>>> -- >>>>>> 1.9.1 >>>>>> >>>>> >>>> >>>> -- >>>> David Henningsson, Canonical Ltd. >>>> https://launchpad.net/~diwic >>>> >>> >> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic >> >
On 2014-05-16 12:37, David Henningsson wrote: > > > On 2014-05-16 08:44, Takashi Iwai wrote: >> At Fri, 16 May 2014 08:11:35 +0200, >> David Henningsson wrote: >>> >>> >>> >>> On 2014-05-13 11:49, Takashi Iwai wrote: >>>> At Tue, 13 May 2014 11:39:02 +0200, >>>> David Henningsson wrote: >>>>> >>>>> >>>>> >>>>> On 2014-05-13 11:06, Takashi Iwai wrote: >>>>>> At Tue, 13 May 2014 10:36:46 +0200, >>>>>> David Henningsson wrote: >>>>>>> >>>>>>> Normally, we match on pci ssid only. This works but needs new code >>>>>>> for every machine. To catch more machines in the same quirk, >>>>>>> let's add >>>>>>> a new type of quirk, where we match on >>>>>>> 1) PCI Subvendor ID (i e, not device, just vendor) >>>>>>> 2) Codec ID >>>>>>> 3) Pin configuration default >>>>>>> >>>>>>> If all these three match, we could be reasonably certain that the >>>>>>> quirk should apply to the machine even though it might not be the >>>>>>> exact same device. >>>>>>> >>>>>>> In case a quirk matches both the pci ssid quirk and this type of >>>>>>> quirk, >>>>>>> then the pci ssid quirk is chosen. >>>>>>> >>>>>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >>>>>>> --- >>>>>>> >>>>>>> Hi Takashi, >>>>>>> >>>>>>> Do you think this could be working? If so I'll ask Hui to move >>>>>>> some machines over >>>>>>> and come up with a patch for that. Two questions: >>>>>>> >>>>>>> 1) I'm unsure if the "name" entry makes sense here - I mean, >>>>>>> since the same quirk >>>>>>> hopefully matches many machines, we can't use the machine >>>>>>> name here. Should I >>>>>>> just skip it? >>>>>>> >>>>>>> 2) Should I make a macro (like SND_PCI_QUIRK) to make the >>>>>>> actual pinquirk tables look >>>>>>> more compact? >>>>>> >>>>>> I actually thought of checking the condition inside the fixup >>>>>> callback. That is, >>>>>> >>>>>> static void dell_fixup_headset(struct hda_codec *codec, >>>>>> const struct hda_fixup *fix, int action) >>>>>> { >>>>>> if (action != HDA_FIXUP_ACT_PRE_PROBE) >>>>>> return; >>>>>> if (codec->vendor_id == xxxx && >>>>>> pin_config_match(codec, xxxx_pincfg) >>>>>> codec->fixup_id = FIXUP_XXXX; >>>>>> else if (...) >>>>>> codec->fixup_id = FIXUP_YYYY; >>>>>> else >>>>>> codec->fixup_id = -1; >>>>>> if (codec->fixup_id != -1) >>>>>> snd_hda_apply_fixup(codec, action); >>>>>> } >>>>>> >>>>>> Of course, this assumes that this fixup is called at the very first, >>>>>> not as a chain. If it may be called as a chain, we need a different >>>>>> way to apply it. >>>>> >>>>> Hmm, still, what do you think about using my solution instead? It's >>>>> more >>>>> generic and could become useful in many different contexts in the >>>>> future, not just matching headsets. And it integrates nicely with the >>>>> existing quirk system as it is just a new method for matching quirks. >>>> >>>> Maybe better not to change snd_hda_pick_fixup(), but add the pincfg >>>> lookup manually. The difference is that you know the order of fixup >>>> lookup in that way while doing it in snd_hda_pick_fixup() hides the >>>> order; e.g. it can be called even if you want to pick up it before the >>>> PCI SSID matching. >>> >>> I just attempted to rewrite the patch according to this, but I didn't >>> like the result. I didn't find an elegant way to handle "nofixup" (you >>> need to check "nofixup" in both pick_fixup functions in order to handle >>> both orders), and I personally prefer to have one consistent ordering >>> across all callers instead of letting the caller decide the order. >> >> Well, "nofixup" matching is one good point. But it doesn't mean that >> it's better to put a style in your patch, but rather it means that >> it's a generic problem even for the existing scheme. >> >> For example, there is already a similar workaround in patch_sigmatel.c >> for the old iMacs. Since Apple doesn't give any useful PCI SSID, we >> need to check codec SSIDs. But even some machines give bogus codec >> SSIDs (0000:0100) due to BIOS, so we have to filter out at first with >> PCI SSID, then apply codec SSID check. >> >> What can we do for fixing both cases (iMac and Dell)? We can't put >> every logic in a flat way into pick_fixup(). Both tells us: >> - we need multi-step filtering >> - some of filters can be reused (e.g. filtering via PCI vendor, some >> device SSID mask) >> - some filter can be arbitrary (e.g. pincfg matching) >> >> This implies that the need isn't to extend the current flat fixup >> parser but redesign somehow to be layered. Otherwise, we'll see the >> same trouble once if we need to add another matching method (OF, ACPI, >> DMI, whatever). >> >> So, until this is properly implemented, I'd like not to touch the >> the helper side, but keep changes inside the codec driver locally. > > Ok, so let's do it "layered" then, so it will be more flexible for the > caller what type of filtering is needed. Will post patches now. > > I left the iMac codec SSID stuff up to you to refactor, because I'm not > sure exactly how you want it done. Hi Takashi, Since you want this cleaned up before 3.16 and the merge window will soon open, can you please comment on the patches I posted a week ago, which makes it possible to use a more layered approach? These are the two patches I'm talking about: http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076691.html http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076692.html Thanks,
At Fri, 23 May 2014 07:17:40 +0200, David Henningsson wrote: > > > > On 2014-05-16 12:37, David Henningsson wrote: > > > > > > On 2014-05-16 08:44, Takashi Iwai wrote: > >> At Fri, 16 May 2014 08:11:35 +0200, > >> David Henningsson wrote: > >>> > >>> > >>> > >>> On 2014-05-13 11:49, Takashi Iwai wrote: > >>>> At Tue, 13 May 2014 11:39:02 +0200, > >>>> David Henningsson wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 2014-05-13 11:06, Takashi Iwai wrote: > >>>>>> At Tue, 13 May 2014 10:36:46 +0200, > >>>>>> David Henningsson wrote: > >>>>>>> > >>>>>>> Normally, we match on pci ssid only. This works but needs new code > >>>>>>> for every machine. To catch more machines in the same quirk, > >>>>>>> let's add > >>>>>>> a new type of quirk, where we match on > >>>>>>> 1) PCI Subvendor ID (i e, not device, just vendor) > >>>>>>> 2) Codec ID > >>>>>>> 3) Pin configuration default > >>>>>>> > >>>>>>> If all these three match, we could be reasonably certain that the > >>>>>>> quirk should apply to the machine even though it might not be the > >>>>>>> exact same device. > >>>>>>> > >>>>>>> In case a quirk matches both the pci ssid quirk and this type of > >>>>>>> quirk, > >>>>>>> then the pci ssid quirk is chosen. > >>>>>>> > >>>>>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >>>>>>> --- > >>>>>>> > >>>>>>> Hi Takashi, > >>>>>>> > >>>>>>> Do you think this could be working? If so I'll ask Hui to move > >>>>>>> some machines over > >>>>>>> and come up with a patch for that. Two questions: > >>>>>>> > >>>>>>> 1) I'm unsure if the "name" entry makes sense here - I mean, > >>>>>>> since the same quirk > >>>>>>> hopefully matches many machines, we can't use the machine > >>>>>>> name here. Should I > >>>>>>> just skip it? > >>>>>>> > >>>>>>> 2) Should I make a macro (like SND_PCI_QUIRK) to make the > >>>>>>> actual pinquirk tables look > >>>>>>> more compact? > >>>>>> > >>>>>> I actually thought of checking the condition inside the fixup > >>>>>> callback. That is, > >>>>>> > >>>>>> static void dell_fixup_headset(struct hda_codec *codec, > >>>>>> const struct hda_fixup *fix, int action) > >>>>>> { > >>>>>> if (action != HDA_FIXUP_ACT_PRE_PROBE) > >>>>>> return; > >>>>>> if (codec->vendor_id == xxxx && > >>>>>> pin_config_match(codec, xxxx_pincfg) > >>>>>> codec->fixup_id = FIXUP_XXXX; > >>>>>> else if (...) > >>>>>> codec->fixup_id = FIXUP_YYYY; > >>>>>> else > >>>>>> codec->fixup_id = -1; > >>>>>> if (codec->fixup_id != -1) > >>>>>> snd_hda_apply_fixup(codec, action); > >>>>>> } > >>>>>> > >>>>>> Of course, this assumes that this fixup is called at the very first, > >>>>>> not as a chain. If it may be called as a chain, we need a different > >>>>>> way to apply it. > >>>>> > >>>>> Hmm, still, what do you think about using my solution instead? It's > >>>>> more > >>>>> generic and could become useful in many different contexts in the > >>>>> future, not just matching headsets. And it integrates nicely with the > >>>>> existing quirk system as it is just a new method for matching quirks. > >>>> > >>>> Maybe better not to change snd_hda_pick_fixup(), but add the pincfg > >>>> lookup manually. The difference is that you know the order of fixup > >>>> lookup in that way while doing it in snd_hda_pick_fixup() hides the > >>>> order; e.g. it can be called even if you want to pick up it before the > >>>> PCI SSID matching. > >>> > >>> I just attempted to rewrite the patch according to this, but I didn't > >>> like the result. I didn't find an elegant way to handle "nofixup" (you > >>> need to check "nofixup" in both pick_fixup functions in order to handle > >>> both orders), and I personally prefer to have one consistent ordering > >>> across all callers instead of letting the caller decide the order. > >> > >> Well, "nofixup" matching is one good point. But it doesn't mean that > >> it's better to put a style in your patch, but rather it means that > >> it's a generic problem even for the existing scheme. > >> > >> For example, there is already a similar workaround in patch_sigmatel.c > >> for the old iMacs. Since Apple doesn't give any useful PCI SSID, we > >> need to check codec SSIDs. But even some machines give bogus codec > >> SSIDs (0000:0100) due to BIOS, so we have to filter out at first with > >> PCI SSID, then apply codec SSID check. > >> > >> What can we do for fixing both cases (iMac and Dell)? We can't put > >> every logic in a flat way into pick_fixup(). Both tells us: > >> - we need multi-step filtering > >> - some of filters can be reused (e.g. filtering via PCI vendor, some > >> device SSID mask) > >> - some filter can be arbitrary (e.g. pincfg matching) > >> > >> This implies that the need isn't to extend the current flat fixup > >> parser but redesign somehow to be layered. Otherwise, we'll see the > >> same trouble once if we need to add another matching method (OF, ACPI, > >> DMI, whatever). > >> > >> So, until this is properly implemented, I'd like not to touch the > >> the helper side, but keep changes inside the codec driver locally. > > > > Ok, so let's do it "layered" then, so it will be more flexible for the > > caller what type of filtering is needed. Will post patches now. > > > > I left the iMac codec SSID stuff up to you to refactor, because I'm not > > sure exactly how you want it done. > > Hi Takashi, > > Since you want this cleaned up before 3.16 and the merge window will > soon open, can you please comment on the patches I posted a week ago, > which makes it possible to use a more layered approach? They mostly look OK, but I just postponed since there are no users for them yet. One thing to be fixed is that the code should work without PCI, so put a NULL check. thanks, Takashi > These are the two patches I'm talking about: > > http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076691.html > > http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076692.html > > Thanks, > > -- > 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 90d2fda..6904779 100644 --- a/sound/pci/hda/hda_auto_parser.c +++ b/sound/pci/hda/hda_auto_parser.c @@ -839,10 +839,22 @@ void snd_hda_apply_fixup(struct hda_codec *codec, int action) } EXPORT_SYMBOL_GPL(snd_hda_apply_fixup); -void snd_hda_pick_fixup(struct hda_codec *codec, - const struct hda_model_fixup *models, - const struct snd_pci_quirk *quirk, - const struct hda_fixup *fixlist) +static bool pin_config_match(struct hda_codec *codec, + const struct hda_pintbl *pins) +{ + for (; pins->nid; pins++) { + u32 def_conf = snd_hda_codec_get_pincfg(codec, pins->nid); + if (pins->val != def_conf) + return false; + } + return true; +} + +void snd_hda_pick_pin_fixup(struct hda_codec *codec, + const struct hda_model_fixup *models, + const struct snd_pci_quirk *quirk, + const struct snd_hda_pin_quirk *pin_quirk, + const struct hda_fixup *fixlist) { const struct snd_pci_quirk *q; int id = -1; @@ -889,10 +901,27 @@ void snd_hda_pick_fixup(struct hda_codec *codec, } } + if (id < 0 && pin_quirk) { + const struct snd_hda_pin_quirk *pq; + for (pq = pin_quirk; pq->subvendor; pq++) { + if (codec->bus->pci->subsystem_vendor != pq->subvendor) + continue; + if (codec->vendor_id != pq->codec) + continue; + if (pin_config_match(codec, pq->pins)) { + id = pq->value; +#ifdef CONFIG_SND_DEBUG_VERBOSE + name = pq->name; +#endif + break; + } + } + } + codec->fixup_id = id; if (id >= 0) { codec->fixup_list = fixlist; codec->fixup_name = name; } } -EXPORT_SYMBOL_GPL(snd_hda_pick_fixup); +EXPORT_SYMBOL_GPL(snd_hda_pick_pin_fixup); diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index e51d155..41a6817 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -425,15 +425,34 @@ enum { HDA_FIXUP_ACT_FREE, }; +struct snd_hda_pin_quirk { + unsigned int codec; /* Codec vendor/device ID */ + unsigned short subvendor; /* PCI subvendor ID */ + const struct hda_pintbl *pins; /* list of matching pins */ +#ifdef CONFIG_SND_DEBUG_VERBOSE + const char *name; +#endif + int value; /* quirk value */ +}; + int snd_hda_add_verbs(struct hda_codec *codec, const struct hda_verb *list); void snd_hda_apply_verbs(struct hda_codec *codec); void snd_hda_apply_pincfgs(struct hda_codec *codec, const struct hda_pintbl *cfg); void snd_hda_apply_fixup(struct hda_codec *codec, int action); -void snd_hda_pick_fixup(struct hda_codec *codec, - const struct hda_model_fixup *models, - const struct snd_pci_quirk *quirk, - const struct hda_fixup *fixlist); +void snd_hda_pick_pin_fixup(struct hda_codec *codec, + const struct hda_model_fixup *models, + const struct snd_pci_quirk *quirk, + const struct snd_hda_pin_quirk *pin_quirk, + const struct hda_fixup *fixlist); + +static inline void snd_hda_pick_fixup(struct hda_codec *codec, + const struct hda_model_fixup *models, + const struct snd_pci_quirk *quirk, + const struct hda_fixup *fixlist) +{ + snd_hda_pick_pin_fixup(codec, models, quirk, NULL, fixlist); +} /* * unsolicited event handler
Normally, we match on pci ssid only. This works but needs new code for every machine. To catch more machines in the same quirk, let's add a new type of quirk, where we match on 1) PCI Subvendor ID (i e, not device, just vendor) 2) Codec ID 3) Pin configuration default If all these three match, we could be reasonably certain that the quirk should apply to the machine even though it might not be the exact same device. In case a quirk matches both the pci ssid quirk and this type of quirk, then the pci ssid quirk is chosen. Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- Hi Takashi, Do you think this could be working? If so I'll ask Hui to move some machines over and come up with a patch for that. Two questions: 1) I'm unsure if the "name" entry makes sense here - I mean, since the same quirk hopefully matches many machines, we can't use the machine name here. Should I just skip it? 2) Should I make a macro (like SND_PCI_QUIRK) to make the actual pinquirk tables look more compact? sound/pci/hda/hda_auto_parser.c | 39 ++++++++++++++++++++++++++++++++++----- sound/pci/hda/hda_local.h | 27 +++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 9 deletions(-)