Re: [PATCH 3/3] mfd: cros_ec: Add ACPI GPE handler for LID0 devices

[Thierry Escande]

Hi Lee,

On 04/01/2017 10:06, Lee Jones wrote:
> On Fri, 16 Dec 2016, Thierry Escande wrote:
>
> > From: Archana Patni <archana.patni@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxxxxx>
> > ---
> >  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.

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)