[PATCH 0/8] platform/chrome: cros_ec: Fixes and improvements

[PATCH 0/8] platform/chrome: cros_ec: Fixes and improvements

[Thierry Escande]

Hi,

This series contains various fixes and improvements for the ChromeOS
Embedded Controller drivers. These concern PM suspend/resume fixes,
sysfs interface, and module initialization.

Regards,
 Thierry

Daniel Hung-yu Wu (1):
  platform/chrome: cros_ec: register shutdown function for debugfs

Douglas Anderson (1):
  mfd: cros_ec: Stop the debugfs work when suspended

Gwendal Grignou (3):
  iio: cros_ec: Relax sampling frequency before suspending
  platform/chrome: cros_ec: Add sysfs entry to set keyboard wake lid
    angle
  platform/chrome: cros_ec: sysfs: Modify error handling

Joseph Lo (1):
  mfd: cros_ec_i2c: move the system sleep pm ops to late

Vincent Palatin (1):
  mfd: cros_ec: fail early if we cannot identify the EC

Wei-Ning Huang (1):
  mfd: cros_ec_i2c: add ACPI module device table

 .../iio/common/cros_ec_sensors/cros_ec_sensors.c   |   1 +
 .../common/cros_ec_sensors/cros_ec_sensors_core.c  |  52 +++++++++++
 .../common/cros_ec_sensors/cros_ec_sensors_core.h  |   2 +
 drivers/iio/light/cros_ec_light_prox.c             |   1 +
 drivers/mfd/cros_ec.c                              |   6 +-
 drivers/mfd/cros_ec_i2c.c                          |  17 +++-
 drivers/platform/chrome/cros_ec_debugfs.c          |  18 ++++
 drivers/platform/chrome/cros_ec_debugfs.h          |   2 +
 drivers/platform/chrome/cros_ec_dev.c              |  45 +++++----
 drivers/platform/chrome/cros_ec_sysfs.c            | 104 ++++++++++++++++-----
 include/linux/mfd/cros_ec.h                        |   1 +
 11 files changed, 206 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH 1/8] iio: cros_ec: Relax sampling frequency before suspending

[Thierry Escande]

From: Gwendal Grignou <gwendal@chromium.org>

If an application set a tight sampling frequency, given the interrupt
use is a wakup source, suspend will not happen: the kernel will receive
a wake up interrupt and will cancel the suspend process.

Given cros_ec sensors type is non wake up, this patch adds prepare and
complete callbacks to set 1s sampling period just before suspend. This
ensures the sensor hub will not be a source of interrupt during the
suspend process.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 .../iio/common/cros_ec_sensors/cros_ec_sensors.c   |  1 +
 .../common/cros_ec_sensors/cros_ec_sensors_core.c  | 49 ++++++++++++++++++++++
 .../common/cros_ec_sensors/cros_ec_sensors_core.h  |  2 +
 drivers/iio/light/cros_ec_light_prox.c             |  1 +
 4 files changed, 53 insertions(+)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
index 38e8783..116da2c 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -292,6 +292,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_sensors_ids);
 static struct platform_driver cros_ec_sensors_platform_driver = {
 	.driver = {
 		.name	= "cros-ec-sensors",
+		.pm	= &cros_ec_sensors_pm_ops,
 	},
 	.probe		= cros_ec_sensors_probe,
 	.id_table	= cros_ec_sensors_ids,
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 416cae5..a620eb5 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -446,5 +446,54 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
 
+static int __maybe_unused cros_ec_sensors_prepare(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+
+	if (st->curr_sampl_freq == 0)
+		return 0;
+
+	/*
+	 * If the sensors are sampled at high frequency, we will not be able to
+	 * sleep. Set sampling to a long period if necessary.
+	 */
+	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
+		mutex_lock(&st->cmd_lock);
+		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
+		st->param.ec_rate.data = CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY;
+		cros_ec_motion_send_host_cmd(st, 0);
+		mutex_unlock(&st->cmd_lock);
+	}
+	return 0;
+}
+
+static void __maybe_unused cros_ec_sensors_complete(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+
+	if (st->curr_sampl_freq == 0)
+		return;
+
+	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
+		mutex_lock(&st->cmd_lock);
+		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
+		st->param.ec_rate.data = st->curr_sampl_freq;
+		cros_ec_motion_send_host_cmd(st, 0);
+		mutex_unlock(&st->cmd_lock);
+	}
+}
+
+const struct dev_pm_ops cros_ec_sensors_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+	.prepare = cros_ec_sensors_prepare,
+	.complete = cros_ec_sensors_complete
+#endif
+};
+EXPORT_SYMBOL_GPL(cros_ec_sensors_pm_ops);
+
 MODULE_DESCRIPTION("ChromeOS EC sensor hub core functions");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
index 8bc2ca3..2edf68d 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
@@ -169,6 +169,8 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
 			       struct iio_chan_spec const *chan,
 			       int val, int val2, long mask);
 
+extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
+
 /* List of extended channel specification for all sensors */
 extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
 
diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 7217223..f8658d4 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -279,6 +279,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
 static struct platform_driver cros_ec_light_prox_platform_driver = {
 	.driver = {
 		.name	= "cros-ec-light-prox",
+		.pm	= &cros_ec_sensors_pm_ops,
 	},
 	.probe		= cros_ec_light_prox_probe,
 	.id_table	= cros_ec_light_prox_ids,
-- 
2.7.4

Re: [PATCH 1/8] iio: cros_ec: Relax sampling frequency before suspending

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:43AM +0200, Thierry Escande wrote:
> From: Gwendal Grignou <gwendal@chromium.org>
> 
> If an application set a tight sampling frequency, given the interrupt
> use is a wakup source, suspend will not happen: the kernel will receive
> a wake up interrupt and will cancel the suspend process.
> 
> Given cros_ec sensors type is non wake up, this patch adds prepare and
> complete callbacks to set 1s sampling period just before suspend. This
> ensures the sensor hub will not be a source of interrupt during the
> suspend process.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Remember to copy the original author, Gwendal Also, this is an iio patch,
so you should make sure to send this to linux-iio@vger.kernel.org and to 
Jonathan Cameron <jic23@kernel.org> as well.

> ---
>  .../iio/common/cros_ec_sensors/cros_ec_sensors.c   |  1 +
>  .../common/cros_ec_sensors/cros_ec_sensors_core.c  | 49 ++++++++++++++++++++++
>  .../common/cros_ec_sensors/cros_ec_sensors_core.h  |  2 +
>  drivers/iio/light/cros_ec_light_prox.c             |  1 +
>  4 files changed, 53 insertions(+)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 38e8783..116da2c 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -292,6 +292,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_sensors_ids);
>  static struct platform_driver cros_ec_sensors_platform_driver = {
>  	.driver = {
>  		.name	= "cros-ec-sensors",
> +		.pm	= &cros_ec_sensors_pm_ops,
>  	},
>  	.probe		= cros_ec_sensors_probe,
>  	.id_table	= cros_ec_sensors_ids,
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 416cae5..a620eb5 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -446,5 +446,54 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
>  
> +static int __maybe_unused cros_ec_sensors_prepare(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +
> +	if (st->curr_sampl_freq == 0)
> +		return 0;
> +
> +	/*
> +	 * If the sensors are sampled at high frequency, we will not be able to
> +	 * sleep. Set sampling to a long period if necessary.
> +	 */
> +	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
> +		mutex_lock(&st->cmd_lock);
> +		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> +		st->param.ec_rate.data = CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY;
> +		cros_ec_motion_send_host_cmd(st, 0);
> +		mutex_unlock(&st->cmd_lock);
> +	}
> +	return 0;
> +}
> +
> +static void __maybe_unused cros_ec_sensors_complete(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +
> +	if (st->curr_sampl_freq == 0)
> +		return;
> +
> +	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
> +		mutex_lock(&st->cmd_lock);
> +		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> +		st->param.ec_rate.data = st->curr_sampl_freq;
> +		cros_ec_motion_send_host_cmd(st, 0);
> +		mutex_unlock(&st->cmd_lock);
> +	}
> +}
> +
> +const struct dev_pm_ops cros_ec_sensors_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> +	.prepare = cros_ec_sensors_prepare,
> +	.complete = cros_ec_sensors_complete
> +#endif
> +};
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_pm_ops);
> +
>  MODULE_DESCRIPTION("ChromeOS EC sensor hub core functions");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> index 8bc2ca3..2edf68d 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> @@ -169,6 +169,8 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
>  			       struct iio_chan_spec const *chan,
>  			       int val, int val2, long mask);
>  
> +extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
> +
>  /* List of extended channel specification for all sensors */
>  extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
>  
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 7217223..f8658d4 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -279,6 +279,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
>  static struct platform_driver cros_ec_light_prox_platform_driver = {
>  	.driver = {
>  		.name	= "cros-ec-light-prox",
> +		.pm	= &cros_ec_sensors_pm_ops,
>  	},
>  	.probe		= cros_ec_light_prox_probe,
>  	.id_table	= cros_ec_light_prox_ids,
> -- 
> 2.7.4
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/8] iio: cros_ec: Relax sampling frequency before suspending

[Jonathan Cameron]

On Thu, 10 Aug 2017 20:25:56 -0700
Benson Leung <bleung@google.com> wrote:

> Hi Thierry,
> 
> On Fri, Aug 11, 2017 at 12:16:43AM +0200, Thierry Escande wrote:
> > From: Gwendal Grignou <gwendal@chromium.org>
> > 
> > If an application set a tight sampling frequency, given the interrupt
> > use is a wakup source, suspend will not happen: the kernel will receive
> > a wake up interrupt and will cancel the suspend process.
> > 
> > Given cros_ec sensors type is non wake up, this patch adds prepare and
> > complete callbacks to set 1s sampling period just before suspend. This
> > ensures the sensor hub will not be a source of interrupt during the
> > suspend process.
> > 
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com>  
> 
> Remember to copy the original author, Gwendal Also, this is an iio patch,
> so you should make sure to send this to linux-iio@vger.kernel.org and to 
> Jonathan Cameron <jic23@kernel.org> as well.
Thanks,

Looks fine to me
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  .../iio/common/cros_ec_sensors/cros_ec_sensors.c   |  1 +
> >  .../common/cros_ec_sensors/cros_ec_sensors_core.c  | 49 ++++++++++++++++++++++
> >  .../common/cros_ec_sensors/cros_ec_sensors_core.h  |  2 +
> >  drivers/iio/light/cros_ec_light_prox.c             |  1 +
> >  4 files changed, 53 insertions(+)
> > 
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > index 38e8783..116da2c 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > @@ -292,6 +292,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_sensors_ids);
> >  static struct platform_driver cros_ec_sensors_platform_driver = {
> >  	.driver = {
> >  		.name	= "cros-ec-sensors",
> > +		.pm	= &cros_ec_sensors_pm_ops,
> >  	},
> >  	.probe		= cros_ec_sensors_probe,
> >  	.id_table	= cros_ec_sensors_ids,
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index 416cae5..a620eb5 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -446,5 +446,54 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
> >  }
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_write);
> >  
> > +static int __maybe_unused cros_ec_sensors_prepare(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> > +
> > +	if (st->curr_sampl_freq == 0)
> > +		return 0;
> > +
> > +	/*
> > +	 * If the sensors are sampled at high frequency, we will not be able to
> > +	 * sleep. Set sampling to a long period if necessary.
> > +	 */
> > +	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
> > +		mutex_lock(&st->cmd_lock);
> > +		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> > +		st->param.ec_rate.data = CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY;
> > +		cros_ec_motion_send_host_cmd(st, 0);
> > +		mutex_unlock(&st->cmd_lock);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void __maybe_unused cros_ec_sensors_complete(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> > +
> > +	if (st->curr_sampl_freq == 0)
> > +		return;
> > +
> > +	if (st->curr_sampl_freq < CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY) {
> > +		mutex_lock(&st->cmd_lock);
> > +		st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> > +		st->param.ec_rate.data = st->curr_sampl_freq;
> > +		cros_ec_motion_send_host_cmd(st, 0);
> > +		mutex_unlock(&st->cmd_lock);
> > +	}
> > +}
> > +
> > +const struct dev_pm_ops cros_ec_sensors_pm_ops = {
> > +#ifdef CONFIG_PM_SLEEP
> > +	.prepare = cros_ec_sensors_prepare,
> > +	.complete = cros_ec_sensors_complete
> > +#endif
> > +};
> > +EXPORT_SYMBOL_GPL(cros_ec_sensors_pm_ops);
> > +
> >  MODULE_DESCRIPTION("ChromeOS EC sensor hub core functions");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> > index 8bc2ca3..2edf68d 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.h
> > @@ -169,6 +169,8 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
> >  			       struct iio_chan_spec const *chan,
> >  			       int val, int val2, long mask);
> >  
> > +extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
> > +
> >  /* List of extended channel specification for all sensors */
> >  extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
> >  
> > diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> > index 7217223..f8658d4 100644
> > --- a/drivers/iio/light/cros_ec_light_prox.c
> > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > @@ -279,6 +279,7 @@ MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
> >  static struct platform_driver cros_ec_light_prox_platform_driver = {
> >  	.driver = {
> >  		.name	= "cros-ec-light-prox",
> > +		.pm	= &cros_ec_sensors_pm_ops,
> >  	},
> >  	.probe		= cros_ec_light_prox_probe,
> >  	.id_table	= cros_ec_light_prox_ids,
> > -- 
> > 2.7.4
> >   
> 

[PATCH 2/8] mfd: cros_ec_i2c: move the system sleep pm ops to late

[Thierry Escande]

From: Joseph Lo <josephl@nvidia.com>

The cros_ec_i2c driver is still active after it had suspended or before it
resumes. Besides that, it also tried to transfer data even after the I2C
host had been suspended. This leads the system to crash.

During the test, we also observed that the EC needs to be resumed
earlier due to some status polling from the EC firmware (e.g. battery
status). This patch moves the PM ops to late stage to make it work
normally.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/mfd/cros_ec_i2c.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index 9f70de1..576fcc4 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -341,8 +341,9 @@ static int cros_ec_i2c_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(cros_ec_i2c_pm_ops, cros_ec_i2c_suspend,
-			  cros_ec_i2c_resume);
+const struct dev_pm_ops cros_ec_i2c_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_i2c_suspend, cros_ec_i2c_resume)
+};
 
 static const struct of_device_id cros_ec_i2c_of_match[] = {
 	{ .compatible = "google,cros-ec-i2c", },
-- 
2.7.4

Re: [PATCH 2/8] mfd: cros_ec_i2c: move the system sleep pm ops to late

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:44AM +0200, Thierry Escande wrote:
> From: Joseph Lo <josephl@nvidia.com>
> 

Again, please copy the original author.

> The cros_ec_i2c driver is still active after it had suspended or before it
> resumes. Besides that, it also tried to transfer data even after the I2C
> host had been suspended. This leads the system to crash.
> 
> During the test, we also observed that the EC needs to be resumed
> earlier due to some status polling from the EC firmware (e.g. battery
> status). This patch moves the PM ops to late stage to make it work
> normally.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Benson Leung <bleung@chromium.org>

Thanks!
Benson

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/8] mfd: cros_ec: Stop the debugfs work when suspended

[Thierry Escande]

From: Douglas Anderson <dianders@chromium.org>

This patch stops the debugfs worker thread when the device is suspended.
This change avoids messages like:

  cros-ec-spi spi5.0: spi transfer failed: -108
  cros-ec-spi spi5.0: cs-deassert spi transfer failed: -108
  cros-ec-ctl cros-ec-ctl.0.auto: EC communication failed

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/platform/chrome/cros_ec_debugfs.c | 18 ++++++++++++++++++
 drivers/platform/chrome/cros_ec_debugfs.h |  2 ++
 drivers/platform/chrome/cros_ec_dev.c     |  4 ++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index 4cc66f4..515f411 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -399,3 +399,21 @@ void cros_ec_debugfs_remove(struct cros_ec_dev *ec)
 	debugfs_remove_recursive(ec->debug_info->dir);
 	cros_ec_cleanup_console_log(ec->debug_info);
 }
+
+void cros_ec_debugfs_suspend(struct cros_ec_dev *ec)
+{
+	/*
+	 * cros_ec_debugfs_init() failures are non-fatal; it's also possible
+	 * that we initted things but decided that console log wasn't supported.
+	 * We'll use the same set of checks that cros_ec_debugfs_remove() +
+	 * cros_ec_cleanup_console_log() end up using to handle those cases.
+	 */
+	if (ec->debug_info && ec->debug_info->log_buffer.buf)
+		cancel_delayed_work_sync(&ec->debug_info->log_poll_work);
+}
+
+void cros_ec_debugfs_resume(struct cros_ec_dev *ec)
+{
+	if (ec->debug_info && ec->debug_info->log_buffer.buf)
+		schedule_delayed_work(&ec->debug_info->log_poll_work, 0);
+}
diff --git a/drivers/platform/chrome/cros_ec_debugfs.h b/drivers/platform/chrome/cros_ec_debugfs.h
index 1ff3a50..d29e410 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.h
+++ b/drivers/platform/chrome/cros_ec_debugfs.h
@@ -23,5 +23,7 @@
 /* debugfs stuff */
 int cros_ec_debugfs_init(struct cros_ec_dev *ec);
 void cros_ec_debugfs_remove(struct cros_ec_dev *ec);
+void cros_ec_debugfs_suspend(struct cros_ec_dev *ec);
+void cros_ec_debugfs_resume(struct cros_ec_dev *ec);
 
 #endif  /* _DRV_CROS_EC_DEBUGFS_H_ */
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index cf6c4f0..2571f5e 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -470,6 +470,8 @@ static __maybe_unused int ec_device_suspend(struct device *dev)
 {
 	struct cros_ec_dev *ec = dev_get_drvdata(dev);
 
+	cros_ec_debugfs_suspend(ec);
+
 	lb_suspend(ec);
 
 	return 0;
@@ -481,6 +483,8 @@ static __maybe_unused int ec_device_resume(struct device *dev)
 
 	lb_resume(ec);
 
+	cros_ec_debugfs_resume(ec);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 3/8] mfd: cros_ec: Stop the debugfs work when suspended

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:45AM +0200, Thierry Escande wrote:
> From: Douglas Anderson <dianders@chromium.org>
> 
> This patch stops the debugfs worker thread when the device is suspended.
> This change avoids messages like:
> 
>   cros-ec-spi spi5.0: spi transfer failed: -108
>   cros-ec-spi spi5.0: cs-deassert spi transfer failed: -108
>   cros-ec-ctl cros-ec-ctl.0.auto: EC communication failed
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Looks good. Applied.

Thanks,
Benson
-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/8] platform/chrome: cros_ec: register shutdown function for debugfs

[Thierry Escande]

From: Daniel Hung-yu Wu <hywu@google.com>

Reboot or shutdown during delayed works could corrupt communication with
EC and certain I2C controller may not be able to recover from the error
state.

This patch registers a shutdown callback used to cancel the debugfs log
worker thread.

Signed-off-by: Daniel Hung-yu Wu <hywu@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/platform/chrome/cros_ec_dev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 2571f5e..225d8e9 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -460,6 +460,14 @@ static int ec_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void ec_device_shutdown(struct platform_device *pdev)
+{
+	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
+
+	/* Be sure to clear up debugfs delayed works */
+	cros_ec_debugfs_remove(ec);
+}
+
 static const struct platform_device_id cros_ec_id[] = {
 	{ "cros-ec-ctl", 0 },
 	{ /* sentinel */ },
@@ -502,6 +510,7 @@ static struct platform_driver cros_ec_dev_driver = {
 	},
 	.probe = ec_device_probe,
 	.remove = ec_device_remove,
+	.shutdown = ec_device_shutdown,
 };
 
 static int __init cros_ec_dev_init(void)
-- 
2.7.4

Re: [PATCH 4/8] platform/chrome: cros_ec: register shutdown function for debugfs

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:46AM +0200, Thierry Escande wrote:
> From: Daniel Hung-yu Wu <hywu@google.com>
> 
> Reboot or shutdown during delayed works could corrupt communication with
> EC and certain I2C controller may not be able to recover from the error
> state.
> 
> This patch registers a shutdown callback used to cancel the debugfs log
> worker thread.
> 
> Signed-off-by: Daniel Hung-yu Wu <hywu@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Applied. Thanks.

Benson
-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 5/8] mfd: cros_ec: fail early if we cannot identify the EC

[Thierry Escande]

From: Vincent Palatin <vpalatin@chromium.org>

If we cannot communicate with the EC chip to detect the protocol version
and its features, it's very likely useless to continue. Else we will
commit all kind of uninformed mistakes (using the wrong protocol, the
wrong buffer size, mixing the EC with other chips).

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/mfd/cros_ec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index b0ca5a4c..c5528ae 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -112,7 +112,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 
 	mutex_init(&ec_dev->lock);
 
-	cros_ec_query_all(ec_dev);
+	err = cros_ec_query_all(ec_dev);
+	if (err) {
+		dev_err(dev, "Cannot identify the EC: error %d\n", err);
+		return err;
+	}
 
 	if (ec_dev->irq) {
 		err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
-- 
2.7.4

Re: [PATCH 5/8] mfd: cros_ec: fail early if we cannot identify the EC

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:47AM +0200, Thierry Escande wrote:
> From: Vincent Palatin <vpalatin@chromium.org>
> 
> If we cannot communicate with the EC chip to detect the protocol version
> and its features, it's very likely useless to continue. Else we will
> commit all kind of uninformed mistakes (using the wrong protocol, the
> wrong buffer size, mixing the EC with other chips).
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Benson Leung <bleung@chromium.org>

> ---
>  drivers/mfd/cros_ec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index b0ca5a4c..c5528ae 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -112,7 +112,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  
>  	mutex_init(&ec_dev->lock);
>  
> -	cros_ec_query_all(ec_dev);
> +	err = cros_ec_query_all(ec_dev);
> +	if (err) {
> +		dev_err(dev, "Cannot identify the EC: error %d\n", err);
> +		return err;
> +	}
>  
>  	if (ec_dev->irq) {
>  		err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
> -- 
> 2.7.4
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/8] mfd: cros_ec_i2c: add ACPI module device table

[Thierry Escande]

From: Wei-Ning Huang <wnhuang@google.com>

Add ACPI module device table for matching cros-ec devices to load the
cros_ec_i2c driver automatically.

Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/mfd/cros_ec_i2c.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index 576fcc4..56fc667 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -345,11 +346,13 @@ const struct dev_pm_ops cros_ec_i2c_pm_ops = {
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_i2c_suspend, cros_ec_i2c_resume)
 };
 
+#ifdef CONFIG_OF
 static const struct of_device_id cros_ec_i2c_of_match[] = {
 	{ .compatible = "google,cros-ec-i2c", },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
+#endif
 
 static const struct i2c_device_id cros_ec_i2c_id[] = {
 	{ "cros-ec-i2c", 0 },
@@ -357,9 +360,18 @@ static const struct i2c_device_id cros_ec_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, cros_ec_i2c_id);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cros_ec_i2c_acpi_id[] = {
+	{ "GOOG0008", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_acpi_id);
+#endif
+
 static struct i2c_driver cros_ec_driver = {
 	.driver	= {
 		.name	= "cros-ec-i2c",
+		.acpi_match_table = ACPI_PTR(cros_ec_i2c_acpi_id),
 		.of_match_table = of_match_ptr(cros_ec_i2c_of_match),
 		.pm	= &cros_ec_i2c_pm_ops,
 	},
-- 
2.7.4

Re: [PATCH 6/8] mfd: cros_ec_i2c: add ACPI module device table

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:48AM +0200, Thierry Escande wrote:
> From: Wei-Ning Huang <wnhuang@google.com>
> 
> Add ACPI module device table for matching cros-ec devices to load the
> cros_ec_i2c driver automatically.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Benson Leung <bleung@chromium.org>

> ---
>  drivers/mfd/cros_ec_i2c.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
> index 576fcc4..56fc667 100644
> --- a/drivers/mfd/cros_ec_i2c.c
> +++ b/drivers/mfd/cros_ec_i2c.c
> @@ -13,6 +13,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -345,11 +346,13 @@ const struct dev_pm_ops cros_ec_i2c_pm_ops = {
>  	SET_LATE_SYSTEM_SLEEP_PM_OPS(cros_ec_i2c_suspend, cros_ec_i2c_resume)
>  };
>  
> +#ifdef CONFIG_OF
>  static const struct of_device_id cros_ec_i2c_of_match[] = {
>  	{ .compatible = "google,cros-ec-i2c", },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
> +#endif
>  
>  static const struct i2c_device_id cros_ec_i2c_id[] = {
>  	{ "cros-ec-i2c", 0 },
> @@ -357,9 +360,18 @@ static const struct i2c_device_id cros_ec_i2c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, cros_ec_i2c_id);
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cros_ec_i2c_acpi_id[] = {
> +	{ "GOOG0008", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_acpi_id);
> +#endif
> +
>  static struct i2c_driver cros_ec_driver = {
>  	.driver	= {
>  		.name	= "cros-ec-i2c",
> +		.acpi_match_table = ACPI_PTR(cros_ec_i2c_acpi_id),
>  		.of_match_table = of_match_ptr(cros_ec_i2c_of_match),
>  		.pm	= &cros_ec_i2c_pm_ops,
>  	},
> -- 
> 2.7.4
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 7/8] platform/chrome: cros_ec: Add sysfs entry to set keyboard wake lid angle

[Thierry Escande]

From: Gwendal Grignou <gwendal@chromium.org>

This adds a sysfs attribute (/sys/class/chromeos/cros_ec/kb_wake_angle)
used to set and get the keyboard wake lid angle. This attribute is
present only if 2 accelerometers are controlled by the EC.

This patch also moves the cros_ec features check before the device is
added so the features map obtained from the EC is ready on time.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/platform/chrome/cros_ec_dev.c   | 32 ++++++-------
 drivers/platform/chrome/cros_ec_sysfs.c | 80 +++++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec.h             |  1 +
 3 files changed, 94 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 225d8e9..a7d711c 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -304,8 +304,8 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 
 	resp = (struct ec_response_motion_sense *)msg->data;
 	sensor_num = resp->dump.sensor_count;
-	/* Allocate 2 extra sensors in case lid angle or FIFO are needed */
-	sensor_cells = kzalloc(sizeof(struct mfd_cell) * (sensor_num + 2),
+	/* Allocate one extra sensor for MOTION_SENSE_FIFO if needed */
+	sensor_cells = kzalloc(sizeof(struct mfd_cell) * (sensor_num + 1),
 			       GFP_KERNEL);
 	if (sensor_cells == NULL)
 		goto error;
@@ -361,16 +361,8 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 		sensor_type[resp->info.type]++;
 		id++;
 	}
-	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2) {
-		sensor_platforms[id].sensor_num = sensor_num;
-
-		sensor_cells[id].name = "cros-ec-angle";
-		sensor_cells[id].id = 0;
-		sensor_cells[id].platform_data = &sensor_platforms[id];
-		sensor_cells[id].pdata_size =
-			sizeof(struct cros_ec_sensor_platform);
-		id++;
-	}
+	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
+		ec->has_kb_wake_angle = true;
 	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
 		sensor_cells[id].name = "cros-ec-ring";
 		id++;
@@ -423,6 +415,15 @@ static int ec_device_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
+	/* check whether this EC is a sensor hub. */
+	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) {
+		pr_err("has EC_FEATURE_MOTION_SENSE\n");
+		cros_ec_sensors_register(ec);
+	}
+
+	/* Take control of the lightbar from the EC. */
+	lb_manual_suspend_ctrl(ec, 1);
+
 	retval = cdev_device_add(&ec->cdev, &ec->class_dev);
 	if (retval) {
 		dev_err(dev, "cdev_device_add failed => %d\n", retval);
@@ -432,13 +433,6 @@ static int ec_device_probe(struct platform_device *pdev)
 	if (cros_ec_debugfs_init(ec))
 		dev_warn(dev, "failed to create debugfs directory\n");
 
-	/* check whether this EC is a sensor hub. */
-	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
-		cros_ec_sensors_register(ec);
-
-	/* Take control of the lightbar from the EC. */
-	lb_manual_suspend_ctrl(ec, 1);
-
 	return 0;
 
 failed:
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f3baf99..60ff122 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -278,20 +278,100 @@ static ssize_t show_ec_flashinfo(struct device *dev,
 	return ret;
 }
 
+/* Keyboard wake angle control */
+
+static ssize_t show_kb_wake_angle(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct ec_response_motion_sense *resp;
+	struct ec_params_motion_sense *param;
+	struct cros_ec_command *msg;
+	int ret;
+	struct cros_ec_dev *ec = container_of(
+			dev, struct cros_ec_dev, class_dev);
+
+	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	param = (struct ec_params_motion_sense *)msg->data;
+	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
+	msg->version = 2;
+	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
+	param->kb_wake_angle.data = EC_MOTION_SENSE_NO_VALUE;
+	msg->outsize = sizeof(*param);
+	msg->insize = sizeof(*resp);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0)
+		return ret;
+	resp = (struct ec_response_motion_sense *)msg->data;
+	return scnprintf(buf, PAGE_SIZE, "%d\n",
+			resp->kb_wake_angle.ret);
+}
+
+static ssize_t store_kb_wake_angle(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct ec_params_motion_sense *param;
+	struct cros_ec_command *msg;
+	int ret;
+	struct cros_ec_dev *ec = container_of(
+			dev, struct cros_ec_dev, class_dev);
+	uint16_t angle;
+
+	ret = kstrtou16(buf, 0, &angle);
+	if (ret)
+		return ret;
+
+	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	param = (struct ec_params_motion_sense *)msg->data;
+	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
+	msg->version = 2;
+	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
+	param->kb_wake_angle.data = angle;
+	msg->outsize = sizeof(*param);
+	msg->insize = sizeof(struct ec_response_motion_sense);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
 /* Module initialization */
 
 static DEVICE_ATTR(reboot, S_IWUSR | S_IRUGO, show_ec_reboot, store_ec_reboot);
 static DEVICE_ATTR(version, S_IRUGO, show_ec_version, NULL);
 static DEVICE_ATTR(flashinfo, S_IRUGO, show_ec_flashinfo, NULL);
+static DEVICE_ATTR(kb_wake_angle, S_IWUSR | S_IRUGO, show_kb_wake_angle,
+		   store_kb_wake_angle);
 
 static struct attribute *__ec_attrs[] = {
+	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
 	&dev_attr_flashinfo.attr,
 	NULL,
 };
 
+static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
+				    struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+					      class_dev);
+
+	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
+		return 0;
+
+	return a->mode;
+}
+
 struct attribute_group cros_ec_attr_group = {
 	.attrs = __ec_attrs,
+	.is_visible = cros_ec_ctrl_visible,
 };
 
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 4e887ba..350b8a4 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -191,6 +191,7 @@ struct cros_ec_dev {
 	struct cros_ec_device *ec_dev;
 	struct device *dev;
 	struct cros_ec_debugfs *debug_info;
+	bool has_kb_wake_angle;
 	u16 cmd_offset;
 	u32 features[2];
 };
-- 
2.7.4

Re: [PATCH 7/8] platform/chrome: cros_ec: Add sysfs entry to set keyboard wake lid angle

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 7990 bytes --]

Hi Thierry,

On Fri, Aug 11, 2017 at 12:16:49AM +0200, Thierry Escande wrote:
> From: Gwendal Grignou <gwendal@chromium.org>
> 
> This adds a sysfs attribute (/sys/class/chromeos/cros_ec/kb_wake_angle)
> used to set and get the keyboard wake lid angle. This attribute is
> present only if 2 accelerometers are controlled by the EC.
> 
> This patch also moves the cros_ec features check before the device is
> added so the features map obtained from the EC is ready on time.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/platform/chrome/cros_ec_dev.c   | 32 ++++++-------
>  drivers/platform/chrome/cros_ec_sysfs.c | 80 +++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h             |  1 +
>  3 files changed, 94 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index 225d8e9..a7d711c 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -304,8 +304,8 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  
>  	resp = (struct ec_response_motion_sense *)msg->data;
>  	sensor_num = resp->dump.sensor_count;
> -	/* Allocate 2 extra sensors in case lid angle or FIFO are needed */
> -	sensor_cells = kzalloc(sizeof(struct mfd_cell) * (sensor_num + 2),
> +	/* Allocate one extra sensor for MOTION_SENSE_FIFO if needed */
> +	sensor_cells = kzalloc(sizeof(struct mfd_cell) * (sensor_num + 1),
>  			       GFP_KERNEL);
>  	if (sensor_cells == NULL)
>  		goto error;
> @@ -361,16 +361,8 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  		sensor_type[resp->info.type]++;
>  		id++;
>  	}
> -	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2) {
> -		sensor_platforms[id].sensor_num = sensor_num;
> -
> -		sensor_cells[id].name = "cros-ec-angle";
> -		sensor_cells[id].id = 0;
> -		sensor_cells[id].platform_data = &sensor_platforms[id];
> -		sensor_cells[id].pdata_size =
> -			sizeof(struct cros_ec_sensor_platform);
> -		id++;
> -	}
> +	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
> +		ec->has_kb_wake_angle = true;
>  	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
>  		sensor_cells[id].name = "cros-ec-ring";
>  		id++;
> @@ -423,6 +415,15 @@ static int ec_device_probe(struct platform_device *pdev)
>  		goto failed;
>  	}
>  
> +	/* check whether this EC is a sensor hub. */
> +	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) {
> +		pr_err("has EC_FEATURE_MOTION_SENSE\n");
> +		cros_ec_sensors_register(ec);
> +	}
> +
> +	/* Take control of the lightbar from the EC. */
> +	lb_manual_suspend_ctrl(ec, 1);
> +
>  	retval = cdev_device_add(&ec->cdev, &ec->class_dev);
>  	if (retval) {
>  		dev_err(dev, "cdev_device_add failed => %d\n", retval);
> @@ -432,13 +433,6 @@ static int ec_device_probe(struct platform_device *pdev)
>  	if (cros_ec_debugfs_init(ec))
>  		dev_warn(dev, "failed to create debugfs directory\n");
>  
> -	/* check whether this EC is a sensor hub. */
> -	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> -		cros_ec_sensors_register(ec);
> -
> -	/* Take control of the lightbar from the EC. */
> -	lb_manual_suspend_ctrl(ec, 1);
> -
>  	return 0;
>  
>  failed:
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index f3baf99..60ff122 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -278,20 +278,100 @@ static ssize_t show_ec_flashinfo(struct device *dev,
>  	return ret;
>  }
>  
> +/* Keyboard wake angle control */
> +
> +static ssize_t show_kb_wake_angle(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct ec_response_motion_sense *resp;
> +	struct ec_params_motion_sense *param;
> +	struct cros_ec_command *msg;
> +	int ret;
> +	struct cros_ec_dev *ec = container_of(
> +			dev, struct cros_ec_dev, class_dev);
> +
> +	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	param = (struct ec_params_motion_sense *)msg->data;
> +	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
> +	msg->version = 2;
> +	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
> +	param->kb_wake_angle.data = EC_MOTION_SENSE_NO_VALUE;
> +	msg->outsize = sizeof(*param);
> +	msg->insize = sizeof(*resp);
> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);

Original CHROMIUM commit uses cros_ec_cmd_xfer here.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/changes/15/456415/5/drivers/platform/chrome/cros_ec_sysfs.c#303


> +	if (ret < 0)
> +		return ret;

Original commit also has this check:
	if (msg->result != EC_RES_SUCCESS)
		return scnprintf(buf, PAGE_SIZE, "ERROR: EC returned %d\n",
				 msg->result);

Now, in the CHROMIUM commits, Gwendal removes this in the next patch, which
is "[PATCH 8/8] platform/chrome: cros_ec: sysfs: Modify error handling" in your
series, but can we try to keep these as close to the originals as possible?

> +	resp = (struct ec_response_motion_sense *)msg->data;
> +	return scnprintf(buf, PAGE_SIZE, "%d\n",
> +			resp->kb_wake_angle.ret);
> +}
> +
> +static ssize_t store_kb_wake_angle(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct ec_params_motion_sense *param;
> +	struct cros_ec_command *msg;
> +	int ret;
> +	struct cros_ec_dev *ec = container_of(
> +			dev, struct cros_ec_dev, class_dev);
> +	uint16_t angle;
> +
> +	ret = kstrtou16(buf, 0, &angle);
> +	if (ret)
> +		return ret;
> +
> +	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	param = (struct ec_params_motion_sense *)msg->data;
> +	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
> +	msg->version = 2;
> +	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
> +	param->kb_wake_angle.data = angle;
> +	msg->outsize = sizeof(*param);
> +	msg->insize = sizeof(struct ec_response_motion_sense);
> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> +	if (ret < 0)
> +		return ret;
> +	return count;
> +}
> +
>  /* Module initialization */
>  
>  static DEVICE_ATTR(reboot, S_IWUSR | S_IRUGO, show_ec_reboot, store_ec_reboot);
>  static DEVICE_ATTR(version, S_IRUGO, show_ec_version, NULL);
>  static DEVICE_ATTR(flashinfo, S_IRUGO, show_ec_flashinfo, NULL);
> +static DEVICE_ATTR(kb_wake_angle, S_IWUSR | S_IRUGO, show_kb_wake_angle,
> +		   store_kb_wake_angle);
>  
>  static struct attribute *__ec_attrs[] = {
> +	&dev_attr_kb_wake_angle.attr,
>  	&dev_attr_reboot.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_flashinfo.attr,
>  	NULL,
>  };
>  
> +static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
> +				    struct attribute *a, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
> +					      class_dev);
> +
> +	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
> +		return 0;
> +
> +	return a->mode;
> +}
> +
>  struct attribute_group cros_ec_attr_group = {
>  	.attrs = __ec_attrs,
> +	.is_visible = cros_ec_ctrl_visible,
>  };
>  
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 4e887ba..350b8a4 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -191,6 +191,7 @@ struct cros_ec_dev {
>  	struct cros_ec_device *ec_dev;
>  	struct device *dev;
>  	struct cros_ec_debugfs *debug_info;
> +	bool has_kb_wake_angle;
>  	u16 cmd_offset;
>  	u32 features[2];
>  };
> -- 
> 2.7.4
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 7/8] platform/chrome: cros_ec: Add sysfs entry to set keyboard wake lid angle

[Thierry Escande]

Hi Benson,

On 11/08/2017 07:29, Benson Leung wrote:
<snip>
>> +/* Keyboard wake angle control */
>> +
>> +static ssize_t show_kb_wake_angle(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct ec_response_motion_sense *resp;
>> +	struct ec_params_motion_sense *param;
>> +	struct cros_ec_command *msg;
>> +	int ret;
>> +	struct cros_ec_dev *ec = container_of(
>> +			dev, struct cros_ec_dev, class_dev);
>> +
>> +	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> +	if (!msg)
>> +		return -ENOMEM;
>> +
>> +	param = (struct ec_params_motion_sense *)msg->data;
>> +	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>> +	msg->version = 2;
>> +	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
>> +	param->kb_wake_angle.data = EC_MOTION_SENSE_NO_VALUE;
>> +	msg->outsize = sizeof(*param);
>> +	msg->insize = sizeof(*resp);
>> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> 
> Original CHROMIUM commit uses cros_ec_cmd_xfer here.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/changes/15/456415/5/drivers/platform/chrome/cros_ec_sysfs.c#303
> 
> 
>> +	if (ret < 0)
>> +		return ret;
> 
> Original commit also has this check:
> 	if (msg->result != EC_RES_SUCCESS)
> 		return scnprintf(buf, PAGE_SIZE, "ERROR: EC returned %d\n",
> 				 msg->result);
> 
> Now, in the CHROMIUM commits, Gwendal removes this in the next patch, which
> is "[PATCH 8/8] platform/chrome: cros_ec: sysfs: Modify error handling" in your
> series, but can we try to keep these as close to the originals as possible?

Ok, sure. With the original patches I was pretty sure to get something 
like "hey, you modify something you've just added on the previous patch!" :)

I'll repost the whole series even if you've applied 2 of them already.

Regards,
  Thierry

> 
>> +	resp = (struct ec_response_motion_sense *)msg->data;
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n",
>> +			resp->kb_wake_angle.ret);
>> +}
>> +
>> +static ssize_t store_kb_wake_angle(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   const char *buf, size_t count)
>> +{
>> +	struct ec_params_motion_sense *param;
>> +	struct cros_ec_command *msg;
>> +	int ret;
>> +	struct cros_ec_dev *ec = container_of(
>> +			dev, struct cros_ec_dev, class_dev);
>> +	uint16_t angle;
>> +
>> +	ret = kstrtou16(buf, 0, &angle);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> +	if (!msg)
>> +		return -ENOMEM;
>> +
>> +	param = (struct ec_params_motion_sense *)msg->data;
>> +	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>> +	msg->version = 2;
>> +	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
>> +	param->kb_wake_angle.data = angle;
>> +	msg->outsize = sizeof(*param);
>> +	msg->insize = sizeof(struct ec_response_motion_sense);
>> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> +	if (ret < 0)
>> +		return ret;
>> +	return count;
>> +}
>> +
>>   /* Module initialization */
>>   
>>   static DEVICE_ATTR(reboot, S_IWUSR | S_IRUGO, show_ec_reboot, store_ec_reboot);
>>   static DEVICE_ATTR(version, S_IRUGO, show_ec_version, NULL);
>>   static DEVICE_ATTR(flashinfo, S_IRUGO, show_ec_flashinfo, NULL);
>> +static DEVICE_ATTR(kb_wake_angle, S_IWUSR | S_IRUGO, show_kb_wake_angle,
>> +		   store_kb_wake_angle);
>>   
>>   static struct attribute *__ec_attrs[] = {
>> +	&dev_attr_kb_wake_angle.attr,
>>   	&dev_attr_reboot.attr,
>>   	&dev_attr_version.attr,
>>   	&dev_attr_flashinfo.attr,
>>   	NULL,
>>   };
>>   
>> +static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
>> +				    struct attribute *a, int n)
>> +{
>> +	struct device *dev = container_of(kobj, struct device, kobj);
>> +	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
>> +					      class_dev);
>> +
>> +	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
>> +		return 0;
>> +
>> +	return a->mode;
>> +}
>> +
>>   struct attribute_group cros_ec_attr_group = {
>>   	.attrs = __ec_attrs,
>> +	.is_visible = cros_ec_ctrl_visible,
>>   };
>>   
>> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
>> index 4e887ba..350b8a4 100644
>> --- a/include/linux/mfd/cros_ec.h
>> +++ b/include/linux/mfd/cros_ec.h
>> @@ -191,6 +191,7 @@ struct cros_ec_dev {
>>   	struct cros_ec_device *ec_dev;
>>   	struct device *dev;
>>   	struct cros_ec_debugfs *debug_info;
>> +	bool has_kb_wake_angle;
>>   	u16 cmd_offset;
>>   	u32 features[2];
>>   };
>> -- 
>> 2.7.4
>>
> 

[PATCH 8/8] platform/chrome: cros_ec: sysfs: Modify error handling

[Thierry Escande]

From: Gwendal Grignou <gwendal@chromium.org>

When accessing a sysfs attribute, if the EC command fails, -EPROTO is
now returned instead of an error message as it is unlikely an app is
parsing the error message to do something meaningful.
Also, this patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer() so an error message is printed in the syslog.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/platform/chrome/cros_ec_sysfs.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 60ff122..ba69fe9 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -116,15 +116,9 @@ static ssize_t store_ec_reboot(struct device *dev,
 	msg->command = EC_CMD_REBOOT_EC + ec->cmd_offset;
 	msg->outsize = sizeof(*param);
 	msg->insize = 0;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
-	if (ret < 0) {
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0)
 		count = ret;
-		goto exit;
-	}
-	if (msg->result != EC_RES_SUCCESS) {
-		dev_dbg(ec->dev, "EC result %d\n", msg->result);
-		count = -EINVAL;
-	}
 exit:
 	kfree(msg);
 	return count;
@@ -152,17 +146,11 @@ static ssize_t show_ec_version(struct device *dev,
 	msg->command = EC_CMD_GET_VERSION + ec->cmd_offset;
 	msg->insize = sizeof(*r_ver);
 	msg->outsize = 0;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0) {
 		count = ret;
 		goto exit;
 	}
-	if (msg->result != EC_RES_SUCCESS) {
-		count = scnprintf(buf, PAGE_SIZE,
-				  "ERROR: EC returned %d\n", msg->result);
-		goto exit;
-	}
-
 	r_ver = (struct ec_response_get_version *)msg->data;
 	/* Strings should be null-terminated, but let's be sure. */
 	r_ver->version_string_ro[sizeof(r_ver->version_string_ro) - 1] = '\0';
@@ -257,14 +245,9 @@ static ssize_t show_ec_flashinfo(struct device *dev,
 	msg->command = EC_CMD_FLASH_INFO + ec->cmd_offset;
 	msg->insize = sizeof(*resp);
 	msg->outsize = 0;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto exit;
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = scnprintf(buf, PAGE_SIZE,
-				"ERROR: EC returned %d\n", msg->result);
-		goto exit;
-	}
 
 	resp = (struct ec_response_flash_info *)msg->data;
 
-- 
2.7.4