[PATCH 0/3] mfd: cros_ec: various improvements

[PATCH 0/3] mfd: cros_ec: various improvements

[Thierry Escande]

Hi,

This series adds some improvements to the ChromeOS EC driver.
- New EC host commands are sent through suspend/resume ops.
- A flag now protect data transfer while device is suspended.
- LID0 device GPE now stays enabled for lid to work in suspend to
  idle path.

This patchset depends on [1] to apply.
[1] https://lkml.org/lkml/2016/12/2/360

Regards,
 Thierry

Archana Patni (1):
  mfd: cros_ec: Add ACPI GPE handler for LID0 devices

Joseph Lo (1):
  mfd: cros_ec: Prevent data transfer while device is suspended

Shawn Nematbakhsh (1):
  mfd: cros_ec: Send suspend state notification to EC

 drivers/mfd/cros_ec.c                   | 144 ++++++++++++++++++++++++++++++++
 drivers/platform/chrome/cros_ec_proto.c |   5 ++
 include/linux/mfd/cros_ec.h             |   2 +
 include/linux/mfd/cros_ec_commands.h    |  14 ++++
 4 files changed, 165 insertions(+)

-- 
2.7.4

[PATCH 1/3] mfd: cros_ec: Prevent data transfer while device is suspended

[Thierry Escande]

From: Joseph Lo <josephl@nvidia.com>

The cros_ec driver is still active while the device is suspended.
Besides that, it also tries to transfer data even after the I2C host had
been suspended. This patch uses a simple flag to prevent this.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/mfd/cros_ec.c                   | 2 ++
 drivers/platform/chrome/cros_ec_proto.c | 5 +++++
 include/linux/mfd/cros_ec.h             | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index abd8342..ad48633 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -165,6 +165,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
 
 	disable_irq(ec_dev->irq);
 	ec_dev->was_wake_device = ec_dev->wake_enabled;
+	ec_dev->suspended = true;
 
 	return 0;
 }
@@ -179,6 +180,7 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
 
 int cros_ec_resume(struct cros_ec_device *ec_dev)
 {
+	ec_dev->suspended = false;
 	enable_irq(ec_dev->irq);
 
 	/*
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 04053fe..ed5dee7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -447,6 +447,11 @@ static int get_next_event(struct cros_ec_device *ec_dev)
 	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
 	int ret;
 
+	if (ec_dev->suspended) {
+		dev_dbg(ec_dev->dev, "Device suspended.\n");
+		return -EHOSTDOWN;
+	}
+
 	msg->version = 0;
 	msg->command = EC_CMD_GET_NEXT_EVENT;
 	msg->insize = sizeof(ec_dev->event_data);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 76f7ef4..d585bf0 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -103,6 +103,7 @@ struct cros_ec_command {
  * @din_size: size of din buffer to allocate (zero to use static din)
  * @dout_size: size of dout buffer to allocate (zero to use static dout)
  * @wake_enabled: true if this device can wake the system from sleep
+ * @suspended: true if this device had been suspended
  * @cmd_xfer: send command to EC and get response
  *     Returns the number of bytes received if the communication succeeded, but
  *     that doesn't mean the EC was happy with the command. The caller
@@ -136,6 +137,7 @@ struct cros_ec_device {
 	int din_size;
 	int dout_size;
 	bool wake_enabled;
+	bool suspended;
 	int (*cmd_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
 	int (*pkt_xfer)(struct cros_ec_device *ec,
-- 
2.7.4

[PATCH 2/3] mfd: cros_ec: Send suspend state notification to EC

[Thierry Escande]

From: Shawn Nematbakhsh <shawnn@chromium.org>

Notify EC when going to or returning from suspend so that proper actions
related to wake events can be taken.

Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/mfd/cros_ec.c                | 49 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec_commands.h | 14 +++++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index ad48633..b8a5080 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/cros_ec.h>
+#include <linux/suspend.h>
 #include <asm/unaligned.h>
 
 #define CROS_EC_DEV_EC_INDEX 0
@@ -65,6 +66,24 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
+{
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_host_sleep_event req;
+	} __packed buf;
+
+	memset(&buf, 0, sizeof(buf));
+
+	buf.req.sleep_event = sleep_event;
+
+	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
+	buf.msg.version = 0;
+	buf.msg.outsize = sizeof(buf.req);
+
+	return cros_ec_cmd_xfer(ec_dev, &buf.msg);
+}
+
 int cros_ec_register(struct cros_ec_device *ec_dev)
 {
 	struct device *dev = ec_dev->dev;
@@ -136,6 +155,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		}
 	}
 
+	/*
+	 * Clear sleep event - this will fail harmlessly on platforms that
+	 * don't implement the sleep event host command.
+	 */
+	err = cros_ec_sleep_event(ec_dev, 0);
+	if (err < 0)
+		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
+			err);
+
 	dev_info(dev, "Chrome EC device registered\n");
 
 	return 0;
@@ -159,6 +187,16 @@ EXPORT_SYMBOL(cros_ec_remove);
 int cros_ec_suspend(struct cros_ec_device *ec_dev)
 {
 	struct device *dev = ec_dev->dev;
+	int ret;
+	u8 sleep_event;
+
+	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
+						  HOST_SLEEP_EVENT_S0IX_RESUME;
+
+	ret = cros_ec_sleep_event(ec_dev, sleep_event);
+	if (ret < 0)
+		dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
+			ret);
 
 	if (device_may_wakeup(dev))
 		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
@@ -180,9 +218,20 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
 
 int cros_ec_resume(struct cros_ec_device *ec_dev)
 {
+	int ret;
+	u8 sleep_event;
+
 	ec_dev->suspended = false;
 	enable_irq(ec_dev->irq);
 
+	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
+						  HOST_SLEEP_EVENT_S0IX_RESUME;
+
+	ret = cros_ec_sleep_event(ec_dev, sleep_event);
+	if (ret < 0)
+		dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
+			ret);
+
 	/*
 	 * In some cases, we need to distinguish between events that occur
 	 * during suspend if the EC is not a wake source. For example,
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 76728ff..4d55132 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2304,6 +2304,20 @@ struct ec_params_ext_power_current_limit {
 	uint32_t limit; /* in mA */
 } __packed;
 
+/* Inform the EC when entering a sleep state */
+#define EC_CMD_HOST_SLEEP_EVENT 0xa9
+
+enum host_sleep_event {
+	HOST_SLEEP_EVENT_S3_SUSPEND   = 1,
+	HOST_SLEEP_EVENT_S3_RESUME    = 2,
+	HOST_SLEEP_EVENT_S0IX_SUSPEND = 3,
+	HOST_SLEEP_EVENT_S0IX_RESUME  = 4
+};
+
+struct ec_params_host_sleep_event {
+	uint8_t sleep_event;
+} __packed;
+
 /*****************************************************************************/
 /* Smart battery pass-through */
 
-- 
2.7.4

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

[Thierry Escande]

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
 #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
+
+#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)
+{
+	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
+
 	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) {
+		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)
+			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)
-- 
2.7.4

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

[Lee Jones]

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

Re: [PATCH 1/3] mfd: cros_ec: Prevent data transfer while device is suspended

[Lee Jones]

On Fri, 16 Dec 2016, Thierry Escande wrote:

> From: Joseph Lo <josephl@nvidia.com>
> 
> The cros_ec driver is still active while the device is suspended.
> Besides that, it also tries to transfer data even after the I2C host had
> been suspended. This patch uses a simple flag to prevent this.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/mfd/cros_ec.c                   | 2 ++
>  drivers/platform/chrome/cros_ec_proto.c | 5 +++++
>  include/linux/mfd/cros_ec.h             | 2 ++
>  3 files changed, 9 insertions(+)

Can this patch be taken without 3/3?

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index abd8342..ad48633 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -165,6 +165,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>  
>  	disable_irq(ec_dev->irq);
>  	ec_dev->was_wake_device = ec_dev->wake_enabled;
> +	ec_dev->suspended = true;
>  
>  	return 0;
>  }
> @@ -179,6 +180,7 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
>  
>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>  {
> +	ec_dev->suspended = false;
>  	enable_irq(ec_dev->irq);
>  
>  	/*
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 04053fe..ed5dee7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -447,6 +447,11 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>  	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
>  	int ret;
>  
> +	if (ec_dev->suspended) {
> +		dev_dbg(ec_dev->dev, "Device suspended.\n");
> +		return -EHOSTDOWN;
> +	}
> +
>  	msg->version = 0;
>  	msg->command = EC_CMD_GET_NEXT_EVENT;
>  	msg->insize = sizeof(ec_dev->event_data);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 76f7ef4..d585bf0 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -103,6 +103,7 @@ struct cros_ec_command {
>   * @din_size: size of din buffer to allocate (zero to use static din)
>   * @dout_size: size of dout buffer to allocate (zero to use static dout)
>   * @wake_enabled: true if this device can wake the system from sleep
> + * @suspended: true if this device had been suspended
>   * @cmd_xfer: send command to EC and get response
>   *     Returns the number of bytes received if the communication succeeded, but
>   *     that doesn't mean the EC was happy with the command. The caller
> @@ -136,6 +137,7 @@ struct cros_ec_device {
>  	int din_size;
>  	int dout_size;
>  	bool wake_enabled;
> +	bool suspended;
>  	int (*cmd_xfer)(struct cros_ec_device *ec,
>  			struct cros_ec_command *msg);
>  	int (*pkt_xfer)(struct cros_ec_device *ec,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] mfd: cros_ec: Send suspend state notification to EC

[Lee Jones]

On Fri, 16 Dec 2016, Thierry Escande wrote:

> From: Shawn Nematbakhsh <shawnn@chromium.org>
> 
> Notify EC when going to or returning from suspend so that proper actions
> related to wake events can be taken.
> 
> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/mfd/cros_ec.c                | 49 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec_commands.h | 14 +++++++++++
>  2 files changed, 63 insertions(+)

Can this patch be taken without 3/3?

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index ad48633..b8a5080 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/cros_ec.h>
> +#include <linux/suspend.h>
>  #include <asm/unaligned.h>
>  
>  #define CROS_EC_DEV_EC_INDEX 0
> @@ -65,6 +66,24 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> +{
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_host_sleep_event req;
> +	} __packed buf;
> +
> +	memset(&buf, 0, sizeof(buf));
> +
> +	buf.req.sleep_event = sleep_event;
> +
> +	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> +	buf.msg.version = 0;
> +	buf.msg.outsize = sizeof(buf.req);
> +
> +	return cros_ec_cmd_xfer(ec_dev, &buf.msg);
> +}
> +
>  int cros_ec_register(struct cros_ec_device *ec_dev)
>  {
>  	struct device *dev = ec_dev->dev;
> @@ -136,6 +155,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  		}
>  	}
>  
> +	/*
> +	 * Clear sleep event - this will fail harmlessly on platforms that
> +	 * don't implement the sleep event host command.
> +	 */
> +	err = cros_ec_sleep_event(ec_dev, 0);
> +	if (err < 0)
> +		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
> +			err);
> +
>  	dev_info(dev, "Chrome EC device registered\n");
>  
>  	return 0;
> @@ -159,6 +187,16 @@ EXPORT_SYMBOL(cros_ec_remove);
>  int cros_ec_suspend(struct cros_ec_device *ec_dev)
>  {
>  	struct device *dev = ec_dev->dev;
> +	int ret;
> +	u8 sleep_event;
> +
> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
> +
> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
> +	if (ret < 0)
> +		dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
> +			ret);
>  
>  	if (device_may_wakeup(dev))
>  		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
> @@ -180,9 +218,20 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
>  
>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>  {
> +	int ret;
> +	u8 sleep_event;
> +
>  	ec_dev->suspended = false;
>  	enable_irq(ec_dev->irq);
>  
> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
> +
> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
> +	if (ret < 0)
> +		dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> +			ret);
> +
>  	/*
>  	 * In some cases, we need to distinguish between events that occur
>  	 * during suspend if the EC is not a wake source. For example,
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 76728ff..4d55132 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -2304,6 +2304,20 @@ struct ec_params_ext_power_current_limit {
>  	uint32_t limit; /* in mA */
>  } __packed;
>  
> +/* Inform the EC when entering a sleep state */
> +#define EC_CMD_HOST_SLEEP_EVENT 0xa9
> +
> +enum host_sleep_event {
> +	HOST_SLEEP_EVENT_S3_SUSPEND   = 1,
> +	HOST_SLEEP_EVENT_S3_RESUME    = 2,
> +	HOST_SLEEP_EVENT_S0IX_SUSPEND = 3,
> +	HOST_SLEEP_EVENT_S0IX_RESUME  = 4
> +};
> +
> +struct ec_params_host_sleep_event {
> +	uint8_t sleep_event;
> +} __packed;
> +
>  /*****************************************************************************/
>  /* Smart battery pass-through */
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] mfd: cros_ec: Send suspend state notification to EC

[Thierry Escande]

Hi Lee,

On 04/01/2017 10:07, Lee Jones wrote:
> On Fri, 16 Dec 2016, Thierry Escande wrote:
>
>> From: Shawn Nematbakhsh <shawnn@chromium.org>
>>
>> Notify EC when going to or returning from suspend so that proper actions
>> related to wake events can be taken.
>>
>> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
>> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
>> ---
>>  drivers/mfd/cros_ec.c                | 49 ++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/cros_ec_commands.h | 14 +++++++++++
>>  2 files changed, 63 insertions(+)
>
> Can this patch be taken without 3/3?
Yes, both 1st and 2nd patches can be taken without the 3rd one. I'll 
send a v2 with 3/3 in a separate patch.

Regards,
  Thierry

>
> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index ad48633..b8a5080 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/module.h>
>>  #include <linux/mfd/core.h>
>>  #include <linux/mfd/cros_ec.h>
>> +#include <linux/suspend.h>
>>  #include <asm/unaligned.h>
>>
>>  #define CROS_EC_DEV_EC_INDEX 0
>> @@ -65,6 +66,24 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>
>> +static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>> +{
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_host_sleep_event req;
>> +	} __packed buf;
>> +
>> +	memset(&buf, 0, sizeof(buf));
>> +
>> +	buf.req.sleep_event = sleep_event;
>> +
>> +	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>> +	buf.msg.version = 0;
>> +	buf.msg.outsize = sizeof(buf.req);
>> +
>> +	return cros_ec_cmd_xfer(ec_dev, &buf.msg);
>> +}
>> +
>>  int cros_ec_register(struct cros_ec_device *ec_dev)
>>  {
>>  	struct device *dev = ec_dev->dev;
>> @@ -136,6 +155,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>>  		}
>>  	}
>>
>> +	/*
>> +	 * Clear sleep event - this will fail harmlessly on platforms that
>> +	 * don't implement the sleep event host command.
>> +	 */
>> +	err = cros_ec_sleep_event(ec_dev, 0);
>> +	if (err < 0)
>> +		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
>> +			err);
>> +
>>  	dev_info(dev, "Chrome EC device registered\n");
>>
>>  	return 0;
>> @@ -159,6 +187,16 @@ EXPORT_SYMBOL(cros_ec_remove);
>>  int cros_ec_suspend(struct cros_ec_device *ec_dev)
>>  {
>>  	struct device *dev = ec_dev->dev;
>> +	int ret;
>> +	u8 sleep_event;
>> +
>> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
>> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
>> +
>> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
>> +	if (ret < 0)
>> +		dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
>> +			ret);
>>
>>  	if (device_may_wakeup(dev))
>>  		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
>> @@ -180,9 +218,20 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
>>
>>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>>  {
>> +	int ret;
>> +	u8 sleep_event;
>> +
>>  	ec_dev->suspended = false;
>>  	enable_irq(ec_dev->irq);
>>
>> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
>> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
>> +
>> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
>> +	if (ret < 0)
>> +		dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
>> +			ret);
>> +
>>  	/*
>>  	 * In some cases, we need to distinguish between events that occur
>>  	 * during suspend if the EC is not a wake source. For example,
>> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
>> index 76728ff..4d55132 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
>> @@ -2304,6 +2304,20 @@ struct ec_params_ext_power_current_limit {
>>  	uint32_t limit; /* in mA */
>>  } __packed;
>>
>> +/* Inform the EC when entering a sleep state */
>> +#define EC_CMD_HOST_SLEEP_EVENT 0xa9
>> +
>> +enum host_sleep_event {
>> +	HOST_SLEEP_EVENT_S3_SUSPEND   = 1,
>> +	HOST_SLEEP_EVENT_S3_RESUME    = 2,
>> +	HOST_SLEEP_EVENT_S0IX_SUSPEND = 3,
>> +	HOST_SLEEP_EVENT_S0IX_RESUME  = 4
>> +};
>> +
>> +struct ec_params_host_sleep_event {
>> +	uint8_t sleep_event;
>> +} __packed;
>> +
>>  /*****************************************************************************/
>>  /* Smart battery pass-through */
>>
>

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@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.

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

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

[Lee Jones]

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?

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

Re: [PATCH 1/3] mfd: cros_ec: Prevent data transfer while device is suspended

[Lee Jones]

On Fri, 16 Dec 2016, Thierry Escande wrote:

> From: Joseph Lo <josephl@nvidia.com>
> 
> The cros_ec driver is still active while the device is suspended.
> Besides that, it also tries to transfer data even after the I2C host had
> been suspended. This patch uses a simple flag to prevent this.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/mfd/cros_ec.c                   | 2 ++
>  drivers/platform/chrome/cros_ec_proto.c | 5 +++++
>  include/linux/mfd/cros_ec.h             | 2 ++
>  3 files changed, 9 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index abd8342..ad48633 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -165,6 +165,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
>  
>  	disable_irq(ec_dev->irq);
>  	ec_dev->was_wake_device = ec_dev->wake_enabled;
> +	ec_dev->suspended = true;
>  
>  	return 0;
>  }
> @@ -179,6 +180,7 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
>  
>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>  {
> +	ec_dev->suspended = false;
>  	enable_irq(ec_dev->irq);
>  
>  	/*
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 04053fe..ed5dee7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -447,6 +447,11 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>  	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
>  	int ret;
>  
> +	if (ec_dev->suspended) {
> +		dev_dbg(ec_dev->dev, "Device suspended.\n");
> +		return -EHOSTDOWN;
> +	}
> +
>  	msg->version = 0;
>  	msg->command = EC_CMD_GET_NEXT_EVENT;
>  	msg->insize = sizeof(ec_dev->event_data);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 76f7ef4..d585bf0 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -103,6 +103,7 @@ struct cros_ec_command {
>   * @din_size: size of din buffer to allocate (zero to use static din)
>   * @dout_size: size of dout buffer to allocate (zero to use static dout)
>   * @wake_enabled: true if this device can wake the system from sleep
> + * @suspended: true if this device had been suspended
>   * @cmd_xfer: send command to EC and get response
>   *     Returns the number of bytes received if the communication succeeded, but
>   *     that doesn't mean the EC was happy with the command. The caller
> @@ -136,6 +137,7 @@ struct cros_ec_device {
>  	int din_size;
>  	int dout_size;
>  	bool wake_enabled;
> +	bool suspended;
>  	int (*cmd_xfer)(struct cros_ec_device *ec,
>  			struct cros_ec_command *msg);
>  	int (*pkt_xfer)(struct cros_ec_device *ec,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] mfd: cros_ec: Send suspend state notification to EC

[Lee Jones]

On Fri, 16 Dec 2016, Thierry Escande wrote:

> From: Shawn Nematbakhsh <shawnn@chromium.org>
> 
> Notify EC when going to or returning from suspend so that proper actions
> related to wake events can be taken.
> 
> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/mfd/cros_ec.c                | 49 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec_commands.h | 14 +++++++++++
>  2 files changed, 63 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index ad48633..b8a5080 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/cros_ec.h>
> +#include <linux/suspend.h>
>  #include <asm/unaligned.h>
>  
>  #define CROS_EC_DEV_EC_INDEX 0
> @@ -65,6 +66,24 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> +{
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_host_sleep_event req;
> +	} __packed buf;
> +
> +	memset(&buf, 0, sizeof(buf));
> +
> +	buf.req.sleep_event = sleep_event;
> +
> +	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> +	buf.msg.version = 0;
> +	buf.msg.outsize = sizeof(buf.req);
> +
> +	return cros_ec_cmd_xfer(ec_dev, &buf.msg);
> +}
> +
>  int cros_ec_register(struct cros_ec_device *ec_dev)
>  {
>  	struct device *dev = ec_dev->dev;
> @@ -136,6 +155,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  		}
>  	}
>  
> +	/*
> +	 * Clear sleep event - this will fail harmlessly on platforms that
> +	 * don't implement the sleep event host command.
> +	 */
> +	err = cros_ec_sleep_event(ec_dev, 0);
> +	if (err < 0)
> +		dev_dbg(ec_dev->dev, "Error %d clearing sleep event to ec",
> +			err);
> +
>  	dev_info(dev, "Chrome EC device registered\n");
>  
>  	return 0;
> @@ -159,6 +187,16 @@ EXPORT_SYMBOL(cros_ec_remove);
>  int cros_ec_suspend(struct cros_ec_device *ec_dev)
>  {
>  	struct device *dev = ec_dev->dev;
> +	int ret;
> +	u8 sleep_event;
> +
> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
> +
> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
> +	if (ret < 0)
> +		dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
> +			ret);
>  
>  	if (device_may_wakeup(dev))
>  		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
> @@ -180,9 +218,20 @@ static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
>  
>  int cros_ec_resume(struct cros_ec_device *ec_dev)
>  {
> +	int ret;
> +	u8 sleep_event;
> +
>  	ec_dev->suspended = false;
>  	enable_irq(ec_dev->irq);
>  
> +	sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
> +						  HOST_SLEEP_EVENT_S0IX_RESUME;
> +
> +	ret = cros_ec_sleep_event(ec_dev, sleep_event);
> +	if (ret < 0)
> +		dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> +			ret);
> +
>  	/*
>  	 * In some cases, we need to distinguish between events that occur
>  	 * during suspend if the EC is not a wake source. For example,
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 76728ff..4d55132 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -2304,6 +2304,20 @@ struct ec_params_ext_power_current_limit {
>  	uint32_t limit; /* in mA */
>  } __packed;
>  
> +/* Inform the EC when entering a sleep state */
> +#define EC_CMD_HOST_SLEEP_EVENT 0xa9
> +
> +enum host_sleep_event {
> +	HOST_SLEEP_EVENT_S3_SUSPEND   = 1,
> +	HOST_SLEEP_EVENT_S3_RESUME    = 2,
> +	HOST_SLEEP_EVENT_S0IX_SUSPEND = 3,
> +	HOST_SLEEP_EVENT_S0IX_RESUME  = 4
> +};
> +
> +struct ec_params_host_sleep_event {
> +	uint8_t sleep_event;
> +} __packed;
> +
>  /*****************************************************************************/
>  /* Smart battery pass-through */
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Thierry Escande]

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

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

[Lee Jones]

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.

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

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

[Thierry Escande]

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