From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032335AbdAEHup (ORCPT ); Thu, 5 Jan 2017 02:50:45 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:38512 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031379AbdAEHu3 (ORCPT ); Thu, 5 Jan 2017 02:50:29 -0500 Date: Thu, 5 Jan 2017 07:54:08 +0000 From: Lee Jones To: Thierry Escande Cc: Benson Leung , linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] mfd: cros_ec: Add ACPI GPE handler for LID0 devices Message-ID: <20170105075408.GI24225@dell> References: <1481911058-28604-1-git-send-email-thierry.escande@collabora.com> <1481911058-28604-4-git-send-email-thierry.escande@collabora.com> <20170104090612.GL27589@dell> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > > > 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 > > > Signed-off-by: Thierry Escande > > > --- > > > 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 > > > +#endif > > > > Please don't place #ifery in C files. > > > > > #include > > > #include > > > #include > > > @@ -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? > > > + 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) > > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog