* [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT @ 2008-06-07 16:22 Thadeu Lima de Souza Cascardo 2008-06-07 17:48 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2008-06-07 16:22 UTC (permalink / raw) To: linux-kernel; +Cc: Takashi Iwai While CONFIG_HDA_POWER_SAVE_DEFAULT is used as a timeout in seconds, CONFIG_AC97_POWER_SAVE_DEFAULT simply enables or disables AC97 power saving. --- sound/pci/Kconfig | 6 +++--- sound/pci/ac97/ac97_codec.c | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig index 7e47421..fe6aad2 100644 --- a/sound/pci/Kconfig +++ b/sound/pci/Kconfig @@ -968,11 +968,11 @@ config SND_AC97_POWER_SAVE sysfs, too. config SND_AC97_POWER_SAVE_DEFAULT - int "Default time-out for AC97 power-save mode" + bool "Activate AC97 power-save mode by default" depends on SND_AC97_POWER_SAVE default 0 help - The default time-out value in seconds for AC97 automatic - power-save mode. 0 means to disable the power-save mode. + By default, AC97 power-save mode is not active. This option + activates it by default. endmenu diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 45fd290..0566e3a 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -48,7 +48,11 @@ module_param(enable_loopback, bool, 0444); MODULE_PARM_DESC(enable_loopback, "Enable AC97 ADC/DAC Loopback Control"); #ifdef CONFIG_SND_AC97_POWER_SAVE -static int power_save = CONFIG_SND_AC97_POWER_SAVE_DEFAULT; +#ifdef CONFIG_SND_AC97_POWER_SAVE_DEFAULT +static int power_save = 1; +#else +static int power_save = 0; +#endif module_param(power_save, bool, 0644); MODULE_PARM_DESC(power_save, "Enable AC97 power-saving control"); #endif -- 1.5.5.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT 2008-06-07 16:22 [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT Thadeu Lima de Souza Cascardo @ 2008-06-07 17:48 ` Takashi Iwai 2008-06-07 19:40 ` Thadeu Lima de Souza Cascardo 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2008-06-07 17:48 UTC (permalink / raw) To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel At Sat, 7 Jun 2008 13:22:39 -0300, Thadeu Lima de Souza Cascardo wrote: > > While CONFIG_HDA_POWER_SAVE_DEFAULT is used as a timeout in seconds, > CONFIG_AC97_POWER_SAVE_DEFAULT simply enables or disables AC97 power > saving. Thanks for the patch. However, I can't take this as is. The reasons are below: The power_save option takes indeed an integer value, and this kconfig is nothing but its default value. In your way, it's set always 1 if selected. This is a very bad choice as power_save value, because you'll turn on/off after one second. This may lead to too frequent click noises. Thus, even if we need to make it bool, a more sensitive value must be chosen. And, which value is sensitive is a matter of taste, and you cannot define it alone by yourself. thanks, Takashi > --- > sound/pci/Kconfig | 6 +++--- > sound/pci/ac97/ac97_codec.c | 6 +++++- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig > index 7e47421..fe6aad2 100644 > --- a/sound/pci/Kconfig > +++ b/sound/pci/Kconfig > @@ -968,11 +968,11 @@ config SND_AC97_POWER_SAVE > sysfs, too. > > config SND_AC97_POWER_SAVE_DEFAULT > - int "Default time-out for AC97 power-save mode" > + bool "Activate AC97 power-save mode by default" > depends on SND_AC97_POWER_SAVE > default 0 > help > - The default time-out value in seconds for AC97 automatic > - power-save mode. 0 means to disable the power-save mode. > + By default, AC97 power-save mode is not active. This option > + activates it by default. > > endmenu > diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c > index 45fd290..0566e3a 100644 > --- a/sound/pci/ac97/ac97_codec.c > +++ b/sound/pci/ac97/ac97_codec.c > @@ -48,7 +48,11 @@ module_param(enable_loopback, bool, 0444); > MODULE_PARM_DESC(enable_loopback, "Enable AC97 ADC/DAC Loopback Control"); > > #ifdef CONFIG_SND_AC97_POWER_SAVE > -static int power_save = CONFIG_SND_AC97_POWER_SAVE_DEFAULT; > +#ifdef CONFIG_SND_AC97_POWER_SAVE_DEFAULT > +static int power_save = 1; > +#else > +static int power_save = 0; > +#endif > module_param(power_save, bool, 0644); > MODULE_PARM_DESC(power_save, "Enable AC97 power-saving control"); > #endif > -- > 1.5.5.3 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT 2008-06-07 17:48 ` Takashi Iwai @ 2008-06-07 19:40 ` Thadeu Lima de Souza Cascardo 2008-06-08 7:23 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2008-06-07 19:40 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 2863 bytes --] On Sat, Jun 07, 2008 at 07:48:16PM +0200, Takashi Iwai wrote: > At Sat, 7 Jun 2008 13:22:39 -0300, > Thadeu Lima de Souza Cascardo wrote: > > > > While CONFIG_HDA_POWER_SAVE_DEFAULT is used as a timeout in seconds, > > CONFIG_AC97_POWER_SAVE_DEFAULT simply enables or disables AC97 power > > saving. > > Thanks for the patch. However, I can't take this as is. The reasons > are below: > > The power_save option takes indeed an integer value, and this kconfig > is nothing but its default value. > > In your way, it's set always 1 if selected. This is a very bad choice > as power_save value, because you'll turn on/off after one second. > This may lead to too frequent click noises. > Thus, even if we need to make it bool, a more sensitive value must be > chosen. And, which value is sensitive is a matter of taste, and you > cannot define it alone by yourself. > As I said in my comment, that is true for CONFIG_HDA_POWER_SAVE_DEFAULT, which is, in fact, used as a number of seconds, and I left that as is. The static power_save variable in ac97_codec.c, however, is only used in a macro ac97_is_power_save_mode, which, in turn, is only used in two if's. > thanks, > > Takashi > Thank you. > > --- > > sound/pci/Kconfig | 6 +++--- > > sound/pci/ac97/ac97_codec.c | 6 +++++- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig > > index 7e47421..fe6aad2 100644 > > --- a/sound/pci/Kconfig > > +++ b/sound/pci/Kconfig > > @@ -968,11 +968,11 @@ config SND_AC97_POWER_SAVE > > sysfs, too. > > > > config SND_AC97_POWER_SAVE_DEFAULT > > - int "Default time-out for AC97 power-save mode" > > + bool "Activate AC97 power-save mode by default" > > depends on SND_AC97_POWER_SAVE > > default 0 > > help > > - The default time-out value in seconds for AC97 automatic > > - power-save mode. 0 means to disable the power-save mode. > > + By default, AC97 power-save mode is not active. This option > > + activates it by default. > > > > endmenu > > diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c > > index 45fd290..0566e3a 100644 > > --- a/sound/pci/ac97/ac97_codec.c > > +++ b/sound/pci/ac97/ac97_codec.c > > @@ -48,7 +48,11 @@ module_param(enable_loopback, bool, 0444); > > MODULE_PARM_DESC(enable_loopback, "Enable AC97 ADC/DAC Loopback Control"); > > > > #ifdef CONFIG_SND_AC97_POWER_SAVE > > -static int power_save = CONFIG_SND_AC97_POWER_SAVE_DEFAULT; > > +#ifdef CONFIG_SND_AC97_POWER_SAVE_DEFAULT > > +static int power_save = 1; > > +#else > > +static int power_save = 0; > > +#endif > > module_param(power_save, bool, 0644); > > MODULE_PARM_DESC(power_save, "Enable AC97 power-saving control"); > > #endif > > -- > > 1.5.5.3 > > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT 2008-06-07 19:40 ` Thadeu Lima de Souza Cascardo @ 2008-06-08 7:23 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2008-06-08 7:23 UTC (permalink / raw) To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel At Sat, 7 Jun 2008 16:40:07 -0300, Thadeu Lima de Souza Cascardo wrote: > > On Sat, Jun 07, 2008 at 07:48:16PM +0200, Takashi Iwai wrote: > > At Sat, 7 Jun 2008 13:22:39 -0300, > > Thadeu Lima de Souza Cascardo wrote: > > > > > > While CONFIG_HDA_POWER_SAVE_DEFAULT is used as a timeout in seconds, > > > CONFIG_AC97_POWER_SAVE_DEFAULT simply enables or disables AC97 power > > > saving. > > > > Thanks for the patch. However, I can't take this as is. The reasons > > are below: > > > > The power_save option takes indeed an integer value, and this kconfig > > is nothing but its default value. > > > > In your way, it's set always 1 if selected. This is a very bad choice > > as power_save value, because you'll turn on/off after one second. > > This may lead to too frequent click noises. > > Thus, even if we need to make it bool, a more sensitive value must be > > chosen. And, which value is sensitive is a matter of taste, and you > > cannot define it alone by yourself. > > > > As I said in my comment, that is true for CONFIG_HDA_POWER_SAVE_DEFAULT, > which is, in fact, used as a number of seconds, and I left that as is. > The static power_save variable in ac97_codec.c, however, is only used in > a macro ac97_is_power_save_mode, which, in turn, is only used in two > if's. Ah, right. It's a simply because I forgot to commit the patch mistakenly. The power_save option must take integer as its timeout value, just like for snd-hda-intel driver, as described in Documentation/sound/alsa/powersave.txt. The patch below is already applied to my git tree, thus for the next release (will be included in linux-next soon). thanks, Takashi >From 92a0a8d2fce86124451bc6d1c1e2d9ce3805df2d Mon Sep 17 00:00:00 2001 From: Takashi Iwai <tiwai@suse.de> Date: Sun, 8 Jun 2008 09:17:27 +0200 Subject: [ALSA] ac97 - Fix power_save option value as time-out The power_save option was set as boot although it was meant to be a timeout value like the same option of snd-hda-intel originally. Now fixed to the same style. Signed-off-by: Takashi Iwai <tiwai@suse.de> diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 45fd290..2d2f16e 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -49,8 +49,9 @@ MODULE_PARM_DESC(enable_loopback, "Enable AC97 ADC/DAC Loopback Control"); #ifdef CONFIG_SND_AC97_POWER_SAVE static int power_save = CONFIG_SND_AC97_POWER_SAVE_DEFAULT; -module_param(power_save, bool, 0644); -MODULE_PARM_DESC(power_save, "Enable AC97 power-saving control"); +module_param(power_save, int, 0644); +MODULE_PARM_DESC(power_save, "Automatic power-saving timeout " + "(in second, 0 = disable)."); #endif /* @@ -2362,7 +2363,7 @@ int snd_ac97_update_power(struct snd_ac97 *ac97, int reg, int powerup) * that open/close frequently) */ schedule_delayed_work(&ac97->power_work, - msecs_to_jiffies(2000)); + msecs_to_jiffies(power_save * 1000)); else { cancel_delayed_work(&ac97->power_work); update_power_regs(ac97); ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-06-08 7:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-06-07 16:22 [PATCH] Correct type and description of CONFIG_AC97_POWER_SAVE_DEFAULT Thadeu Lima de Souza Cascardo 2008-06-07 17:48 ` Takashi Iwai 2008-06-07 19:40 ` Thadeu Lima de Souza Cascardo 2008-06-08 7:23 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®