[alsa-devel] [RFC PATCH] ALSA: hda - Add a new quirk match based on default pin configuration
David Henningsson
david.henningsson at canonical.com
Fri May 16 08:11:35 CEST 2014
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 at 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
>>
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
More information about the Alsa-devel
mailing list