Re: [alsa-devel] [PATCH] ALSA: HDA: hda_local: Less magic numbers.
Robin H. Johnson wrote:
Explain some of the magic numbers I saw while trying to fix the AD1989 SPDIF issue. Maybe should just use the expanded form directly in the verbs?
Signed-off-by: Robin H. Johnson robbat2@gentoo.org
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index d688f50..7957fef 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -368,12 +368,15 @@ int snd_hda_parse_pin_def_config(struct hda_codec *codec, #define AMP_OUT_UNMUTE 0xb000 #define AMP_OUT_ZERO 0xb000 /* pinctl values */ -#define PIN_IN 0x20 -#define PIN_VREF80 0x24 -#define PIN_VREF50 0x21 -#define PIN_OUT 0x40 -#define PIN_HP 0xc0 -#define PIN_HP_AMP 0x80 +#define PIN_IN (AC_PINCTL_IN_EN) +#define PIN_VREFHIZ (AC_PINCTL_IN_EN | AC_PINCTL_VREF_HIZ) +#define PIN_VREF50 (AC_PINCTL_IN_EN | AC_PINCTL_VREF_50) +#define PIN_VREFGRD (AC_PINCTL_IN_EN | AC_PINCTL_VREF_GRD) +#define PIN_VREF80 (AC_PINCTL_IN_EN | AC_PINCTL_VREF_80) +#define PIN_VREF100 (AC_PINCTL_IN_EN | AC_PINCTL_VREF_100) +#define PIN_OUT (AC_PINCTL_OUT_EN) +#define PIN_HP (AC_PINCTL_OUT_EN | AC_PINCTL_HP_EN) +#define PIN_HP_AMP (AC_PINCTL_HP_EN)
Shrug. In my opinion the above patch simply reworks the explanation, shifting the "magic" from the PIN_* definitions to the AC_PINCTL_* defines. One could take things one step further and replace the PIN_* defines with AC_PINCTL_* references throughout the code, but this would make the code far more verbose and harder to follow.
At the end of the day I'm not fussed either way. Having said that, I don't really see the practical advantage of the change but if Takashi et al thing it's a good idea then I wouldn't oppose it.
Regards jonathan
On Mon, Sep 15, 2008 at 09:10:10AM +0930, Jonathan Woithe wrote:
Shrug. In my opinion the above patch simply reworks the explanation, shifting the "magic" from the PIN_* definitions to the AC_PINCTL_* defines. One could take things one step further and replace the PIN_* defines with AC_PINCTL_* references throughout the code, but this would make the code far more verbose and harder to follow.
At the end of the day I'm not fussed either way. Having said that, I don't really see the practical advantage of the change but if Takashi et al thing it's a good idea then I wouldn't oppose it.
A quick glance at the codebase shows:
AC_PINCTL* long form: patch_conexant.c patch_realtek.c patch_sigmatel.c
/PIN_(VREF|IN|OUT|HP)/ hda_codec.c patch_analog.c patch_atihdmi.c patch_cmedia.c patch_conexant.c patch_realtek.c patch_sigmatel.c patch_via.c
Raw hex for AC_VERB_SET_PIN_WIDGET_CONTROL verbs: patch_analog.c patch_realtek.c (LOTS) patch_via.c (LOTS)
The raw hex ones SHOULD be replaced, but the rest of them I'm not fussed either way. Takashi can make a decision as to which variant is preferred, I don't mind doing the cleanup.
At Mon, 15 Sep 2008 13:16:15 -0700, Robin H. Johnson wrote:
On Mon, Sep 15, 2008 at 09:10:10AM +0930, Jonathan Woithe wrote:
Shrug. In my opinion the above patch simply reworks the explanation, shifting the "magic" from the PIN_* definitions to the AC_PINCTL_* defines. One could take things one step further and replace the PIN_* defines with AC_PINCTL_* references throughout the code, but this would make the code far more verbose and harder to follow.
At the end of the day I'm not fussed either way. Having said that, I don't really see the practical advantage of the change but if Takashi et al thing it's a good idea then I wouldn't oppose it.
A quick glance at the codebase shows:
AC_PINCTL* long form: patch_conexant.c patch_realtek.c patch_sigmatel.c
/PIN_(VREF|IN|OUT|HP)/ hda_codec.c patch_analog.c patch_atihdmi.c patch_cmedia.c patch_conexant.c patch_realtek.c patch_sigmatel.c patch_via.c
Raw hex for AC_VERB_SET_PIN_WIDGET_CONTROL verbs: patch_analog.c patch_realtek.c (LOTS) patch_via.c (LOTS)
The raw hex ones SHOULD be replaced, but the rest of them I'm not fussed either way. Takashi can make a decision as to which variant is preferred, I don't mind doing the cleanup.
Robin's patch is a right improvement wrt readability for whom are not familiar with HD-audio codec spec, so I applied it as is.
Well, I also don't care much about the rest, too. And, I don't think it's worth to work on this further so seriously, although I don't mind patches that can be really convincing (if any).
FWIW, AC_* and PIN_* have somewhat different fields: the former is the definition of each bit, and the latter is the values used in the real world.
thanks,
Takashi
Hi Takashi
A quick glance at the codebase shows:
AC_PINCTL* long form: patch_conexant.c patch_realtek.c patch_sigmatel.c
/PIN_(VREF|IN|OUT|HP)/ hda_codec.c patch_analog.c patch_atihdmi.c patch_cmedia.c patch_conexant.c patch_realtek.c patch_sigmatel.c patch_via.c
Raw hex for AC_VERB_SET_PIN_WIDGET_CONTROL verbs: patch_analog.c patch_realtek.c (LOTS) patch_via.c (LOTS)
The raw hex ones SHOULD be replaced, but the rest of them I'm not fussed either way. Takashi can make a decision as to which variant is preferred, I don't mind doing the cleanup.
Robin's patch is a right improvement wrt readability for whom are not familiar with HD-audio codec spec, so I applied it as is.
I have no problem with this so long as it hasn't broken anything. I can't easily test this myself until I return from LPC.
FWIW, AC_* and PIN_* have somewhat different fields: the former is the definition of each bit, and the latter is the values used in the real world.
Thanks - that's very succintly put. :-)
Regards jonathan
participants (3)
-
Jonathan Woithe -
Robin H. Johnson -
Takashi Iwai