From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [RFC PATCH] ALSA: hda - Add a new quirk match based on default pin configuration Date: Tue, 13 May 2014 11:49:07 +0200 Message-ID: References: <1399970206-10269-1-git-send-email-david.henningsson@canonical.com> <5371E836.3010205@canonical.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id AB981265375 for ; Tue, 13 May 2014 11:49:07 +0200 (CEST) In-Reply-To: <5371E836.3010205@canonical.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: David Henningsson Cc: hui.wang@canonical.com, alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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 > >> --- > >> > >> 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 >