On Sunday 23 November 2008 02:09:03 pm Witold Szczeponik wrote:
> Subject: Enable PNPACI Power Management
Hi Witold,
Thanks for your patch. I'm glad somebody is paying attention to
PNP and power. I CC'd Adam and Rafael because they care about this
area, too, but might not read everything on linux-acpi.
> This patch sets the power of PnP ACPI devices to D0 when they
> are activated and to D3 when they are disabled. The latter is
> in correspondence with the ACPI 3.0 specification, whereas the
> former is added in order to be able to power up a device after
> it has been previously disabled (or when booting up a system).
> (As a consequence, the patch makes the PnP ACPI code more ACPI
> compliant.)
Do you know of anything that specifies the order of the _CRS/_PS0
and the _PS3/_DIS evaluation? I don't know much about power
management, and I couldn't find anything obvious in the spec.
It seems plausible that we should run _CRS before turning on
the power, but I really don't know.
> The patch fixes the problem with some IBM ThinkPads (at least
> the 600E and the 600X) where the serial ports have a dedicated
> power source that needs to be brought up before the serial port
> can be used. Without this patch, the serial port is enabled
> but has no power.
Is pnpacpi_set_resources() the only place that needs this change?
For active devices, we normally don't call pnpacpi_set_resources()
at all. So I suppose on these ThinkPads, we exercise this path
because the serial ports are initially disabled?
> No regressions were observed on hardware that does not require
> this patch.
>
> The patch is applied against 2.6.27.7 (vanilla).
>
>
> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@xxxxxxx>
>
>
> Index: linux/drivers/pnp/pnpacpi/core.c
> ===================================================================
> --- linux.orig/drivers/pnp/pnpacpi/core.c
> +++ linux/drivers/pnp/pnpacpi/core.c
> @@ -98,18 +98,24 @@ static int pnpacpi_set_resources(struct
> status = acpi_set_current_resources(handle, &buffer);
> if (ACPI_FAILURE(status))
> ret = -EINVAL;
> + else if (acpi_bus_power_manageable(handle))
> + ret = acpi_bus_set_power(handle, ACPI_STATE_D0);
I don't really like testing acpi_bus_power_manageable() first.
I think we should just call acpi_bus_set_power() and let *it*
bail out if the device doesn't support it.
> kfree(buffer.pointer);
> return ret;
> }
>
> static int pnpacpi_disable_resources(struct pnp_dev *dev)
> {
> + acpi_handle handle = dev->data;
> + int ret = 0;
> acpi_status status;
>
> - /* acpi_unregister_gsi(pnp_irq(dev, 0)); */
Can you leave the "unregister_gsi" comment there, since it's not
related to your patch? It's a reminder that we need to think about
how to handle interrupts when enabling/disabling devices.
> - status = acpi_evaluate_object((acpi_handle) dev->data,
> - "_DIS", NULL, NULL);
> - return ACPI_FAILURE(status) ? -ENODEV : 0;
> + if (acpi_bus_power_manageable(handle))
> + ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
> + status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
> + if (ACPI_FAILURE(status))
> + ret = -ENODEV;
> + return ret;
> }
>
> #ifdef CONFIG_ACPI_SLEEP
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
