Messages in this thread Patch in this message | | | Subject | Re: [PATCH 3/3] mfd: cros_ec: Add ACPI GPE handler for LID0 devices | From | Thierry Escande <> | Date | Thu, 5 Jan 2017 11:37:26 +0100 |
| |
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;
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) >>> >
| |