Messages in this thread | | | Subject | Re: [PATCH 3/3] mfd: cros_ec: Add ACPI GPE handler for LID0 devices | From | Thierry Escande <> | Date | Thu, 5 Jan 2017 17:39:08 +0100 |
| |
On 05/01/2017 15:43, Lee Jones wrote: > Rafael, > > On Thu, 05 Jan 2017, Thierry Escande wrote: > >> Hi Lee, >> >> On 05/01/2017 08:54, Lee Jones wrote: >>> On Wed, 04 Jan 2017, Thierry Escande wrote: >>> >>>> Hi Lee, >>>> >>>> On 04/01/2017 10:06, Lee Jones wrote: >>>>> On Fri, 16 Dec 2016, Thierry Escande wrote: >>>>> >>>>>> From: Archana Patni <archana.patni@intel.com> >>>>>> >>>>>> This patch installs an ACPI GPE handler for LID0 ACPI device to indicate >>>>>> ACPI core that this GPE should stay enabled for lid to work in suspend >>>>>> to idle path. >>>>>> >>>>>> Signed-off-by: Archana Patni <archana.patni@intel.com> >>>>>> Signed-off-by: Thierry Escande <thierry.escande@collabora.com> >>>>>> --- >>>>>> drivers/mfd/cros_ec.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>> 1 file changed, 95 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c >>>>>> index b8a5080..628718e 100644 >>>>>> --- a/drivers/mfd/cros_ec.c >>>>>> +++ b/drivers/mfd/cros_ec.c >>>>>> @@ -17,6 +17,9 @@ >>>>>> * battery charging and regulator control, firmware update. >>>>>> */ >>>>>> >>>>>> +#ifdef CONFIG_ACPI >>>>>> +#include <linux/acpi.h> >>>>>> +#endif >>>>> >>>>> Please don't place #ifery in C files. >>>>> >>>>>> #include <linux/of_platform.h> >>>>>> #include <linux/interrupt.h> >>>>>> #include <linux/slab.h> >>>>>> @@ -29,6 +32,73 @@ >>>>>> #define CROS_EC_DEV_EC_INDEX 0 >>>>>> #define CROS_EC_DEV_PD_INDEX 1 >>>>>> >>>>>> +#ifdef CONFIG_ACPI >>>>> >>>>> Remove this. >>>>> >>>>>> +#define ACPI_LID_DEVICE "LID0" >>>>>> + >>>>>> +static int ec_wake_gpe = -EINVAL; >>>>>> + >>>>>> +/* >>>>>> + * This handler indicates to ACPI core that this GPE should stay enabled for >>>>>> + * lid to work in suspend to idle path. >>>>>> + */ >>>>>> +static u32 cros_ec_gpe_handler(acpi_handle gpe_device, u32 gpe_number, >>>>>> + void *data) >>>>>> +{ >>>>>> + return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; >>>>>> +} >>>>>> + >>>>>> +/* >>>>>> + * Get ACPI GPE for LID0 device. >>>>>> + */ >>>>>> +static int cros_ec_get_ec_wake_gpe(struct device *dev) >>>>>> +{ >>>> If it's ok for you, I'll keep one #ifdef CONFIG_ACPI around the body of this >>>> function. Otherwise it won't compile if CONFIG_ACPI is not set. >>> >>> Can you try placing: >>> >>> if (IS_ENABLED(CONFIG_ACPI)) >>> >>> ... before the call to cros_ec_get_ec_wake_gpe() and see if the >>> compiler will do-the-right-thing please? >> >> With CONFIG_ACPI not defined, the acpi_dev structure is not defined as well >> as the acpi_bus_get_device() API. So no, the compiler is not smart enough >> even if the function body is skipped by the test: >> if (!IS_ENABLED(CONFIG_ACPI)) >> return -ENODEV; > > Is there any way we can make ACPI play nicely if it's not enabled? > > I'm thinking stubs. Like we do for most other things.
Or I can move the 2 functions into a separate file compiled only if ACPI is enabled.
Regards, Thierry
> >>>>>> + struct acpi_device *cros_acpi_dev; >>>>>> + struct acpi_device *adev; >>>>>> + acpi_handle handle; >>>>>> + acpi_status status; >>>>>> + int ret; >>>>>> + >>>>>> + cros_acpi_dev = ACPI_COMPANION(dev); >>>>>> + >>>>>> + if (!cros_acpi_dev || !cros_acpi_dev->parent || >>>>>> + !cros_acpi_dev->parent->handle) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + status = acpi_get_handle(cros_acpi_dev->parent->handle, ACPI_LID_DEVICE, >>>>>> + &handle); >>>>>> + if (ACPI_FAILURE(status)) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + ret = acpi_bus_get_device(handle, &adev); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + return adev->wakeup.gpe_number; >>>>>> +} >>>>>> + >>>>>> +static int cros_ec_install_handler(struct device *dev) >>>>>> +{ >>>>>> + acpi_status status; >>>>>> + >>>>>> + ec_wake_gpe = cros_ec_get_ec_wake_gpe(dev); >>>>>> + >>>>>> + if (ec_wake_gpe < 0) >>>>>> + return ec_wake_gpe; >>>>>> + >>>>>> + status = acpi_install_gpe_handler(NULL, ec_wake_gpe, >>>>>> + ACPI_GPE_EDGE_TRIGGERED, >>>>>> + &cros_ec_gpe_handler, NULL); >>>>>> + if (ACPI_FAILURE(status)) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + dev_info(dev, "Initialized, GPE = 0x%x\n", ec_wake_gpe); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +#endif >>>>>> + >>>>>> static struct cros_ec_platform ec_p = { >>>>>> .ec_name = CROS_EC_DEV_NAME, >>>>>> .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_EC_INDEX), >>>>>> @@ -166,6 +236,10 @@ int cros_ec_register(struct cros_ec_device *ec_dev) >>>>>> >>>>>> dev_info(dev, "Chrome EC device registered\n"); >>>>>> >>>>>> +#ifdef CONFIG_ACPI >>>>>> + cros_ec_install_handler(dev); >>>>>> +#endif >>>>> >>>>> Here, just do: >>>>> >>>>> if (IS_ENABLED(CONFIG_ACPI)) >>>>> cros_ec_install_handler(dev); >>>>> >>>>> And let the compiler take care of the rest. >>>>> >>>>>> return 0; >>>>>> >>>>>> fail_mfd: >>>>>> @@ -179,6 +253,17 @@ int cros_ec_remove(struct cros_ec_device *ec_dev) >>>>>> { >>>>>> mfd_remove_devices(ec_dev->dev); >>>>>> >>>>>> +#ifdef CONFIG_ACPI >>>>>> + if (ec_wake_gpe >= 0) { >>>>> >>>>> if (IS_ENABLED(CONFIG_ACPI) && ec_wake_gpe >= 0) { >>>>> >>>>>> + acpi_status status; >>>>>> + >>>>>> + status = acpi_remove_gpe_handler(NULL, ec_wake_gpe, >>>>>> + &cros_ec_gpe_handler); >>>>>> + if (ACPI_FAILURE(status)) >>>>>> + pr_err("failed to remove gpe handler\n"); >>>>>> + } >>>>>> +#endif >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> EXPORT_SYMBOL(cros_ec_remove); >>>>>> @@ -190,8 +275,16 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev) >>>>>> int ret; >>>>>> u8 sleep_event; >>>>>> >>>>>> - sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME : >>>>>> - HOST_SLEEP_EVENT_S0IX_RESUME; >>>>>> + if (!pm_suspend_via_firmware()) { >>>>>> + sleep_event = HOST_SLEEP_EVENT_S0IX_SUSPEND; >>>>>> +#ifdef CONFIG_ACPI >>>>>> + /* Clearing the GPE status for any pending event */ >>>>>> + if (ec_wake_gpe >= 0) >>>>> >>>>> As above. >>>>> >>>>>> + acpi_clear_gpe(NULL, ec_wake_gpe); >>>>>> +#endif >>>>>> + } else { >>>>>> + sleep_event = HOST_SLEEP_EVENT_S3_SUSPEND; >>>>>> + } >>>>>> >>>>>> ret = cros_ec_sleep_event(ec_dev, sleep_event); >>>>>> if (ret < 0) >>>>> >>> >
| |