On Fri, Mar 10, 2006 at 10:46:49PM +0100, Danny Kukawka wrote:
> Hi,
>
> I attached to patches for powersave. Both are for brightness issues:
>
> 1.) powersave-fix_brightness_powersave.diff:
> This patch fixes the output of 'powersave -k <x>' if the set value is higher
> than the highest available level. Currently powersave alway print the given
> value as set level. I added a goto to the GET_BRIGHTNESS path to print the
> current level.
This can be expensive. I remember that writing / reading the brightness
interface on the PANASONIC CF51 took considerable time. We need to retest
this with a current kernel.
> 2.) powersave-add_pmu_brightness.diff:
> This patch adds brightness support for PMU (ppc/mac) to powersave.
see below.
> Index: daemon/brightness.h
> ===================================================================
> --- daemon/brightness.h (revision 2020)
> +++ daemon/brightness.h (working copy)
> @@ -33,6 +33,7 @@
> #define ACPI_TOSHIBA "/proc/acpi/toshiba/lcd"
> #define ACPI_PANASONIC "/proc/acpi/panasonic/"
> #define LCD_OMNIBOOK "/proc/omnibook/lcd"
> +#define DEV_PMU "/dev/pmu"
maybe we could do some arch-specific IFDEFs here? /dev/pmu will not exist on
x86, and /proc/acpi/* will not exist on ppc. Just an idea, could be useful
in other code-parts, too.
> Index: daemon/brightness.cpp
> ===================================================================
> --- daemon/brightness.cpp (revision 2020)
> +++ daemon/brightness.cpp (working copy)
> @@ -35,6 +35,9 @@
> #include <sys/utsname.h>
> #include <sstream>
>
> +#include <sys/ioctl.h>
> +#include <linux/pmu.h>
> +
> #include "powerlib.h"
> /* merge #include "config_pm.h" */
>
> @@ -76,6 +79,12 @@
> return new BrightnessOmnibook();
> }
>
> + if ((fd = open(DEV_PMU, O_RDONLY)) > 0) {
> + close(fd);
> + return new BrightnessPMU();
> + }
> +
> +
> if (fd) {
> close(fd);
> }
> @@ -727,3 +736,75 @@
> return 11;
> }
>
> +/* Brightness (PMU object (for ppc/mac/iBook)) */
> +
> +void BrightnessPMU::Init()
> +{
> + last_percent = -1;
> + iface = DEV_PMU;
> + return;
> +}
> +
> +int BrightnessPMU::Get()
> +{
> + int level = -1;
> +
> + if ((fd = open(iface, O_RDWR)) < 0) {
> + perror(iface);
perror is useless here. We have already long since closed stdout and stderr
I know that it is in the other brightness interfaces, but no need to copy
more useless stuff ;-)
> + goto out;
> + }
> +
> + if (ioctl (fd, PMU_IOC_GET_BACKLIGHT, &level) < 0 ) {
> + pDebug(DBG_WARN, "Failed ioctl on /dev/pmu with
> PMU_IOC_GET_BACKLIGHT");
> + level = -1;
> + goto out;
> + }
> +
> +out:
Well, that's a useful goto! ;-)
> + if (fd) {
> + close(fd);
> + }
> + return level;
makes me wonder if fd could be 0 for some reason? Probably not in practice,
but theoretically yes: we close stdin, stdout and stderr on startup. If for
some reason the brightness interface is the first one to be initialized, it
could get fd 0...
Again, this is not special to your code.
> +}
> +
> +void BrightnessPMU::Set(int level)
> +{
> + if ((fd = open(iface, O_RDWR)) < 1) {
> + perror(iface);
> + goto out;
> + }
> +
> + if (level > 15)
> + level = 15;
> + else if (level < 1)
> + level = 1;
allow zero here, see below.
> +
> + if(ioctl (fd, PMU_IOC_SET_BACKLIGHT, &level) < 0) {
> + pDebug(DBG_WARN, "Failed ioctl on /dev/pmu with
> PMU_IOC_GET_BACKLIGHT");
> + goto out;
> + }
> +
> +out:
> + if (fd) {
> + close(fd);
> + }
> + return;
> +}
> +
> +void BrightnessPMU::Min()
> +{
> + Set(1); // looks as 0 is display off, use as last 1
allow zero above, leave 1 here. My reasoning is:
- allow the display to be switched off manually (either by "powersave -k d"
of by an explicit set to 0)
- the "Min()", "Max()", and "Med()" methods are designed for usage in
powersaved schemes (when no client is running), so "Min()" should be the
minimal "usable" brightness, but you should not prevent a manual setting of
"light off".
> + return;
> +}
> +
> +void BrightnessPMU::Med()
> +{
> + Set(8);
> + return;
> +}
> +
> +int BrightnessPMU::GetLevels()
> +{
> + return 15;
and if we have zero to 15, this has to return 16 :-)
--
Stefan Seyfried \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \ -- Leonard Cohen
_______________________________________________
powersave-devel mailing list
[email protected]
http://forge.novell.com/mailman/listinfo/powersave-devel
