Re: 1.3.71: drivers/char/apm_bios.c

Stephen.Rothwell@nec.com.au
Thu, 07 Mar 1996 10:04:02 +1100


Hi Rik,

Rik Faith <faith@cs.unc.edu> writes:
> On Tue 5 Mar 1996 11:07:29 +0100,
> Ulrich Windl <windl@rkdvmhp1.dvm.klinik.uni-regensburg.de> wrote:
> > bash$ diff -u /usr/src/linux/drivers/char/apm_bios.c /tmp/apm_bios.c
> > --- /usr/src/linux/drivers/char/apm_bios.c Tue Mar 5 09:44:17 1996
> > +++ /tmp/apm_bios.c Tue Mar 5 10:57:55 1996
> > @@ -396,6 +396,8 @@
> > {
> > u_short error;
> >
> > + if (!apm_enabled)
> > + return APM_DISABLED;
> > APM_SET_POWER_STATE(state, error);
> > if (error & 0xff)
> > return (error >> 8);
>
> This patch should not be necessary because apm_set_power_state is only
> called if apm_enabled.

True.

> > @@ -407,6 +409,8 @@
> > {
> > u_short error;
> >
> > + if (!apm_enabled)
> > + return APM_DISABLED;
> > APM_SET_DISPLAY_POWER_STATE(state, error);
> > if (error & 0xff)
> > return (error >> 8);
>
> This patch is needed, and is appropriate.

Not necessarily.

> > @@ -419,6 +423,8 @@
> > {
> > u_short error;
> >
> > + if (!apm_enabled)
> > + return APM_DISABLED;
> > APM_ENABLE_POWER_MANAGEMENT((apm_bios_info.version > 0x100)
> > ? 0x0001 : 0xffff,
> > error);
> > @@ -442,6 +448,8 @@
> > {
> > u_short error;
> >
> > + if (!apm_enabled)
> > + return APM_DISABLED;
> > APM_ENGAGE_POWER_MANAGEMENT(device, error);
> > if (error & 0xff)
> > return (error >> 8);
>
> These patches are not needed, since apm_enable_power_management and
> apm_engage_power_management are only called if apm_enabled.

True.

> > @@ -465,8 +473,6 @@
> > #ifdef CONFIG_APM_DISPLAY_BLANK
> > int error;
> >
> > - if (apm_bios_info.version == 0)
> > - return 0;
> > error = apm_set_display_power_state(APM_STATE_STANDBY);
> > if (error == APM_SUCCESS)
> > return 1;
>
> This patch is wrong, since this operation is not supported with APM BIOS
> 1.0. [If this happens to work on your APM 1.0 machine, please make a
> similar change to apm_display_unblank and send me email -- otherwise your
> screen may blank and not be restored.]

This MAY be supported on APM 1.0 machines (at least my reading of the spec
allows for it to work). It appears not to work on some machines, but it does
on mine (NEC Ultralite Versa 33/C).

> > @@ -739,11 +745,10 @@
> > #ifdef CONFIG_APM_CPU_IDLE
> > unsigned short error;
> >
> > + if (!apm_enabled)
> > + return;
> > #ifndef ALWAYS_CALL_BUSY
> > if (!clock_slowed)
> > - return;
> > -#else
> > - if (!apm_enabled)
> > return;
> > #endif
>
> This patch is ok. I don't know where the original of this came from (i.e.,
> the #else clause). [In general, it is polite to CC authors/maintainers
> when you post patches (e.g., as Ulrich CC'd me for this posting -- allowing
> me to respond quickly, and to test the patches before final submission to
> Linus). Don't assume that everyone reads linux-kernel.]

My version of the patch:
(Rik, please pass this on to Linus if you agree, thanks.)

--- apm_bios.c.old Wed Mar 6 16:23:44 1996
+++ apm_bios.c Wed Mar 6 16:36:28 1996
@@ -465,7 +465,7 @@
#ifdef CONFIG_APM_DISPLAY_BLANK
int error;

- if (apm_bios_info.version == 0)
+ if (!apm_enabled)
return 0;
error = apm_set_display_power_state(APM_STATE_STANDBY);
if (error == APM_SUCCESS)
@@ -480,7 +480,7 @@
#ifdef CONFIG_APM_DISPLAY_BLANK
int error;

- if (apm_bios_info.version == 0)
+ if (!apm_enabled)
return 0;
error = apm_set_display_power_state(APM_STATE_READY);
if (error == APM_SUCCESS)
@@ -739,12 +739,12 @@
#ifdef CONFIG_APM_CPU_IDLE
unsigned short error;

+ if (!apm_enabled)
+ return;
+
#ifndef ALWAYS_CALL_BUSY
if (!clock_slowed)
return;
-#else
- if (!apm_enabled)
- return;
#endif

APM_SET_CPU_BUSY(error);
@@ -918,7 +918,7 @@
int time_units = -1;
char *units = "?";

- if (apm_bios_info.version == 0)
+ if (!apm_enabled)
return 0;
p = buf;

Cheers,
Stephen

--
Stephen Rothwell                    Stephen.Rothwell@nec.com.au
NEC Information Systems Australia   Phone: +61-6-2508747
Software Development Centre         Fax:   +61-6-2508746
Canberra, Australia