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

[Thierry Escande <thierry.escande@collabora.com>]

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)
>>>>>
>>>
>