Hi,
This is a third version of a patchset that collects some patches
already send but that needed some rework due the patchset to split the
cros_ec_devs modules in 2 parts [1]. This patchset contains some of
imrpovements and also some fixes. They can be applied independently
and I think that all can go through the MFD tree.
Best regards,
Enric
Changes in v3:
- [2/8] Add Acked-by Vincent Palatin.
- [2/8] Add Reviewed-by Andy Shevchenko.
- [3/8] Add Reviewed-by Andy Shevchenko.
- [4/8] Add the Reviewed-by Andy Shevchenko.
- [5/8] Add the Reviewed-by Andy Shevchenko.
- [6/8] Add the Reviewed-by Andy Shevchenko.
- [7/8] Add the Reviewed-by Andy Shevchenko.
- [7/8] Avoid commas in terminator entries.
- [8/8] Add static to cros_ec_i2c_pm_ops.
- [8/8] Add the Reviewed-by Andy Shevchenko.
Changes in v2:
- [2/8] Remove the free_irq in cros_ec_remove.
- [3/8] That patch is new in this series.
- [4/8] Add the Reviewed-by Gwendal.
- [5/8] Add the Reviewed-by Gwendal.
- [6/8] This patch is new in this series.
- [7/8] Add the Reviewed-by Gwendal.
- [8/8] This patch is new in this series replacing [5/6] of v1.
Daniel Hung-yu Wu (1):
mfd: cros_ec_dev: register shutdown function for debugfs
Douglas Anderson (1):
mfd: cros_ec: Don't try to grab log when suspended
Enric Balletbo i Serra (2):
mfd: cros_ec_dev: Register cros-ec-rtc driver as a subdevice.
mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.
Joseph Lo (1):
mfd: cros_ec_i2c: moving the system sleep pm ops to late
Vincent Palatin (2):
mfd: cros_ec: fail early if we cannot identify the EC
mfd: cros_ec: free IRQ automatically
Wei-Ning Huang (1):
mfd: cros_ec_i2c: add ACPI module device table
drivers/mfd/cros_ec.c | 26 ++++-----
drivers/mfd/cros_ec_dev.c | 87 +++++++++++++++++++++++++++++++
drivers/mfd/cros_ec_i2c.c | 17 +++++-
drivers/platform/chrome/cros_ec_debugfs.c | 20 +++++++
include/linux/mfd/cros_ec.h | 2 +
5 files changed, 135 insertions(+), 17 deletions(-)
--
2.16.1
From: Wei-Ning Huang <[email protected]>
Add ACPI module device table for matching cros-ec devices to load the
cros_ec_i2c driver automatically.
Signed-off-by: Wei-Ning Huang <[email protected]>
Acked-by: Benson Leung <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Gwendal Grignou <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [7/8] Add the Reviewed-by Andy Shevchenko.
- [7/8] Avoid commas in terminator entries.
Changes in v2:
- [7/8] Add the Reviewed-by Gwendal.
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 9f70de1e4c70..02a7bacdc056 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>
@@ -344,11 +345,13 @@ static int cros_ec_i2c_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(cros_ec_i2c_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 },
@@ -356,9 +359,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 },
+ { /* sentinel */ }
+};
+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.16.1
From: Daniel Hung-yu Wu <[email protected]>
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 <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [6/8] Add the Reviewed-by Andy Shevchenko.
Changes in v2:
- [6/8] This patch is new in this series.
drivers/mfd/cros_ec_dev.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index fd7f0068fb45..354eb9e01d52 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -535,6 +535,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[] = {
{ DRV_NAME, 0 },
{ /* sentinel */ },
@@ -577,6 +585,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.16.1
From: Douglas Anderson <[email protected]>
We should stop our worker thread while we're suspended. If we don't
then we'll get 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 <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [3/8] Add Reviewed-by Andy Shevchenko.
Changes in v2:
- [3/8] That patch is new in this series.
drivers/mfd/cros_ec_dev.c | 4 ++++
drivers/platform/chrome/cros_ec_debugfs.c | 20 ++++++++++++++++++++
include/linux/mfd/cros_ec.h | 2 ++
3 files changed, 26 insertions(+)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index e4fafdd96e5e..f98e5beffca6 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -471,6 +471,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;
@@ -480,6 +482,8 @@ static __maybe_unused int ec_device_resume(struct device *dev)
{
struct cros_ec_dev *ec = dev_get_drvdata(dev);
+ cros_ec_debugfs_resume(ec);
+
lb_resume(ec);
return 0;
diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index 0e88e18362c1..0328856ec3a2 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -398,3 +398,23 @@ void cros_ec_debugfs_remove(struct cros_ec_dev *ec)
cros_ec_cleanup_console_log(ec->debug_info);
}
EXPORT_SYMBOL(cros_ec_debugfs_remove);
+
+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);
+}
+EXPORT_SYMBOL(cros_ec_debugfs_suspend);
+
+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);
+}
+EXPORT_SYMBOL(cros_ec_debugfs_resume);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index c61535979b8f..804b3ddbf819 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -325,6 +325,8 @@ extern struct attribute_group cros_ec_vbc_attr_group;
/* 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);
/* ACPI GPE handler */
#ifdef CONFIG_ACPI
--
2.16.1
From: Vincent Palatin <[email protected]>
Free the IRQ we might have requested when removing the cros_ec device,
so we can unload and reload the driver properly.
Signed-off-by: Vincent Palatin <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Acked-by: Vincent Palatin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [2/8] Add Acked-by Vincent Palatin.
- [2/8] Add Reviewed-by Andy Shevchenko.
Changes in v2:
- [2/8] Remove the free_irq in cros_ec_remove.
drivers/mfd/cros_ec.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 74780f2964a1..ea8aa704b61e 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -119,9 +119,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
}
if (ec_dev->irq) {
- err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
- IRQF_TRIGGER_LOW | IRQF_ONESHOT,
- "chromeos-ec", ec_dev);
+ err = devm_request_threaded_irq(dev, ec_dev->irq, NULL,
+ ec_irq_thread, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "chromeos-ec", ec_dev);
if (err) {
dev_err(dev, "Failed to request IRQ %d: %d",
ec_dev->irq, err);
@@ -135,7 +135,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
dev_err(dev,
"Failed to register Embedded Controller subdevice %d\n",
err);
- goto fail_mfd;
+ return err;
}
if (ec_dev->max_passthru) {
@@ -153,7 +153,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
dev_err(dev,
"Failed to register Power Delivery subdevice %d\n",
err);
- goto fail_mfd;
+ return err;
}
}
@@ -162,7 +162,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (err) {
mfd_remove_devices(dev);
dev_err(dev, "Failed to register sub-devices\n");
- goto fail_mfd;
+ return err;
}
}
@@ -180,11 +180,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
cros_ec_acpi_install_gpe_handler(dev);
return 0;
-
-fail_mfd:
- if (ec_dev->irq)
- free_irq(ec_dev->irq, ec_dev);
- return err;
}
EXPORT_SYMBOL(cros_ec_register);
@@ -194,9 +189,6 @@ int cros_ec_remove(struct cros_ec_device *ec_dev)
cros_ec_acpi_remove_gpe_handler();
- if (ec_dev->irq)
- free_irq(ec_dev->irq, ec_dev);
-
return 0;
}
EXPORT_SYMBOL(cros_ec_remove);
--
2.16.1
From: Vincent Palatin <[email protected]>
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 <[email protected]>
Acked-by: Benson Leung <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Gwendal Grignou <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3: None
Changes in v2: None
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 d61024141e2b..74780f2964a1 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.16.1
With this patch, the cros_ec_ctl driver will register the legacy
accelerometer driver (named cros_ec_accel_legacy) if it fails to
register sensors through the usual path cros_ec_sensors_register().
This legacy device is present on Chromebook devices with older EC
firmware only supporting deprecated EC commands (Glimmer based devices).
Tested-by: Gwendal Grignou <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Gwendal Grignou <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [5/8] Add the Reviewed-by Andy Shevchenko.
Changes in v2:
- [5/8] Add the Reviewed-by Gwendal.
drivers/mfd/cros_ec_dev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 33fe1b439ee2..fd7f0068fb45 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -389,6 +389,63 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
kfree(msg);
}
+#define CROS_EC_SENSOR_LEGACY_NUM 2
+static struct mfd_cell cros_ec_accel_legacy_cells[CROS_EC_SENSOR_LEGACY_NUM];
+
+static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
+{
+ struct cros_ec_device *ec_dev = ec->ec_dev;
+ u8 status;
+ int i, ret;
+ struct cros_ec_sensor_platform
+ sensor_platforms[CROS_EC_SENSOR_LEGACY_NUM];
+
+ /*
+ * EC that need legacy support are the main EC, directly connected to
+ * the AP.
+ */
+ if (ec->cmd_offset != 0)
+ return;
+
+ /*
+ * Check if EC supports direct memory reads and if EC has
+ * accelerometers.
+ */
+ if (!ec_dev->cmd_readmem)
+ return;
+
+ ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1, &status);
+ if (ret < 0) {
+ dev_warn(ec->dev, "EC does not support direct reads.\n");
+ return;
+ }
+
+ /* Check if EC has accelerometers. */
+ if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
+ dev_info(ec->dev, "EC does not have accelerometers.\n");
+ return;
+ }
+
+ /*
+ * Register 2 accelerometers
+ */
+ for (i = 0; i < CROS_EC_SENSOR_LEGACY_NUM; i++) {
+ cros_ec_accel_legacy_cells[i].name = "cros-ec-accel-legacy";
+ sensor_platforms[i].sensor_num = i;
+ cros_ec_accel_legacy_cells[i].id = i;
+ cros_ec_accel_legacy_cells[i].platform_data =
+ &sensor_platforms[i];
+ cros_ec_accel_legacy_cells[i].pdata_size =
+ sizeof(struct cros_ec_sensor_platform);
+ }
+ ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+ cros_ec_accel_legacy_cells,
+ CROS_EC_SENSOR_LEGACY_NUM,
+ NULL, 0, NULL);
+ if (ret)
+ dev_err(ec_dev->dev, "failed to add EC sensors\n");
+}
+
static const struct mfd_cell cros_ec_rtc_cell = {
.name = "cros-ec-rtc",
};
@@ -440,6 +497,9 @@ static int ec_device_probe(struct platform_device *pdev)
/* check whether this EC is a sensor hub. */
if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
cros_ec_sensors_register(ec);
+ else
+ /* Workaroud for older EC firmware */
+ cros_ec_accel_legacy_register(ec);
/* check whether this EC instance has RTC host command support */
if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
--
2.16.1
Check whether this EC instance has RTC host command support and instatiate
the RTC driver as a subdevice in such case.
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Gwendal Grignou <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [4/8] Add the Reviewed-by Andy Shevchenko.
Changes in v2:
- [4/8] Add the Reviewed-by Gwendal.
drivers/mfd/cros_ec_dev.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index f98e5beffca6..33fe1b439ee2 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -389,6 +389,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
kfree(msg);
}
+static const struct mfd_cell cros_ec_rtc_cell = {
+ .name = "cros-ec-rtc",
+};
+
static int ec_device_probe(struct platform_device *pdev)
{
int retval = -ENOMEM;
@@ -437,6 +441,16 @@ static int ec_device_probe(struct platform_device *pdev)
if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
cros_ec_sensors_register(ec);
+ /* check whether this EC instance has RTC host command support */
+ if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
+ retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+ &cros_ec_rtc_cell, 1, NULL, 0, NULL);
+ if (retval)
+ dev_err(ec->dev,
+ "failed to add cros-ec-rtc device: %d\n",
+ retval);
+ }
+
/* Take control of the lightbar from the EC. */
lb_manual_suspend_ctrl(ec, 1);
--
2.16.1
From: Joseph Lo <[email protected]>
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 will lead the system to crash.
During the test, we also observe that the EC needs to be resumed earlier
due to some status polling from the EC FW (e.g. battery status). So we
move the PM ops to late stage to make it work normally.
Signed-off-by: Joseph Lo <[email protected]>
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v3:
- [8/8] Add static to cros_ec_i2c_pm_ops.
- [8/8] Add the Reviewed-by Andy Shevchenko.
Changes in v2:
- [8/8] This patch is new in this series replacing [5/6] of v1.
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 02a7bacdc056..ef9b4763356f 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -342,8 +342,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);
+static 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[] = {
--
2.16.1
On Mon, Feb 26, 2018 at 12:26 PM, Enric Balletbo i Serra
<[email protected]> wrote:
> From: Wei-Ning Huang <[email protected]>
>
> Add ACPI module device table for matching cros-ec devices to load the
> cros_ec_i2c driver automatically.
> - [7/8] Avoid commas in terminator entries.
> { /* sentinel */ },
Still comma.
Though this is minor and old code, I guess no need to resend b/c of this one.
--
With Best Regards,
Andy Shevchenko
Hi Enric,
Thanks for sending this.
On Mon, Feb 26, 2018 at 11:26:01AM +0100, Enric Balletbo i Serra wrote:
> From: Douglas Anderson <[email protected]>
>
> We should stop our worker thread while we're suspended. If we don't
> then we'll get 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 <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [3/8] Add Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [3/8] That patch is new in this series.
>
> drivers/mfd/cros_ec_dev.c | 4 ++++
> drivers/platform/chrome/cros_ec_debugfs.c | 20 ++++++++++++++++++++
> include/linux/mfd/cros_ec.h | 2 ++
Looks like this will need an immutable branch between mfd and chrome-platform
again, Lee.
Do you want to do one, or should I?
Thanks,
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> Check whether this EC instance has RTC host command support and instatiate
> the RTC driver as a subdevice in such case.
>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Gwendal Grignou <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [4/8] Add the Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [4/8] Add the Reviewed-by Gwendal.
>
> drivers/mfd/cros_ec_dev.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index f98e5beffca6..33fe1b439ee2 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -389,6 +389,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> kfree(msg);
> }
>
> +static const struct mfd_cell cros_ec_rtc_cell = {
> + .name = "cros-ec-rtc",
> +};
> +
> static int ec_device_probe(struct platform_device *pdev)
> {
> int retval = -ENOMEM;
> @@ -437,6 +441,16 @@ static int ec_device_probe(struct platform_device *pdev)
> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> cros_ec_sensors_register(ec);
>
> + /* check whether this EC instance has RTC host command support */
Nit: "Check ..."
> + if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> + retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> + &cros_ec_rtc_cell, 1, NULL, 0, NULL);
Please use ARRAY_SIZE().
> + if (retval)
> + dev_err(ec->dev,
> + "failed to add cros-ec-rtc device: %d\n",
> + retval);
> + }
> +
> /* Take control of the lightbar from the EC. */
> lb_manual_suspend_ctrl(ec, 1);
>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Vincent Palatin <[email protected]>
>
> Free the IRQ we might have requested when removing the cros_ec device,
> so we can unload and reload the driver properly.
>
> Signed-off-by: Vincent Palatin <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Acked-by: Vincent Palatin <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [2/8] Add Acked-by Vincent Palatin.
> - [2/8] Add Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [2/8] Remove the free_irq in cros_ec_remove.
>
> drivers/mfd/cros_ec.c | 20 ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Douglas Anderson <[email protected]>
>
> We should stop our worker thread while we're suspended. If we don't
> then we'll get 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 <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [3/8] Add Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [3/8] That patch is new in this series.
>
> drivers/mfd/cros_ec_dev.c | 4 ++++
> drivers/platform/chrome/cros_ec_debugfs.c | 20 ++++++++++++++++++++
> include/linux/mfd/cros_ec.h | 2 ++
> 3 files changed, 26 insertions(+)
I plan to make this patch 1/X and send it out as a pull-request.
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Vincent Palatin <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Benson Leung <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Gwendal Grignou <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/mfd/cros_ec.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 07 Mar 2018, Lee Jones wrote:
> On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
>
> > From: Douglas Anderson <[email protected]>
> >
> > We should stop our worker thread while we're suspended. If we don't
> > then we'll get 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 <[email protected]>
> > Signed-off-by: Enric Balletbo i Serra <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > ---
> >
> > Changes in v3:
> > - [3/8] Add Reviewed-by Andy Shevchenko.
> >
> > Changes in v2:
> > - [3/8] That patch is new in this series.
> >
> > drivers/mfd/cros_ec_dev.c | 4 ++++
> > drivers/platform/chrome/cros_ec_debugfs.c | 20 ++++++++++++++++++++
> > include/linux/mfd/cros_ec.h | 2 ++
> > 3 files changed, 26 insertions(+)
>
> I plan to make this patch 1/X and send it out as a pull-request.
>
> Acked-by: Lee Jones <[email protected]>
In fact better still, could you do that and add a note below the ---
to that affect please?
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> With this patch, the cros_ec_ctl driver will register the legacy
> accelerometer driver (named cros_ec_accel_legacy) if it fails to
> register sensors through the usual path cros_ec_sensors_register().
> This legacy device is present on Chromebook devices with older EC
> firmware only supporting deprecated EC commands (Glimmer based devices).
>
> Tested-by: Gwendal Grignou <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Gwendal Grignou <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [5/8] Add the Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [5/8] Add the Reviewed-by Gwendal.
>
> drivers/mfd/cros_ec_dev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 33fe1b439ee2..fd7f0068fb45 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -389,6 +389,63 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> kfree(msg);
> }
>
> +#define CROS_EC_SENSOR_LEGACY_NUM 2
> +static struct mfd_cell cros_ec_accel_legacy_cells[CROS_EC_SENSOR_LEGACY_NUM];
> +
> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> +{
> + struct cros_ec_device *ec_dev = ec->ec_dev;
> + u8 status;
> + int i, ret;
> + struct cros_ec_sensor_platform
> + sensor_platforms[CROS_EC_SENSOR_LEGACY_NUM];
I wish I had seen this struct before. Yuk!
Why do you even need to know what 'number' the sensor is?
> + /*
> + * EC that need legacy support are the main EC, directly connected to
Nit: "ECs" or "needs". If you choose "needs" you need to s/are/is/ as
well.
> + * the AP.
> + */
> + if (ec->cmd_offset != 0)
> + return;
> +
> + /*
> + * Check if EC supports direct memory reads and if EC has
> + * accelerometers.
> + */
> + if (!ec_dev->cmd_readmem)
> + return;
> +
> + ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1, &status);
> + if (ret < 0) {
Is ret > 0 valid?
> + dev_warn(ec->dev, "EC does not support direct reads.\n");
> + return;
> + }
> +
> + /* Check if EC has accelerometers. */
> + if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
> + dev_info(ec->dev, "EC does not have accelerometers.\n");
> + return;
> + }
> +
> + /*
> + * Register 2 accelerometers
> + */
> + for (i = 0; i < CROS_EC_SENSOR_LEGACY_NUM; i++) {
> + cros_ec_accel_legacy_cells[i].name = "cros-ec-accel-legacy";
> + sensor_platforms[i].sensor_num = i;
> + cros_ec_accel_legacy_cells[i].id = i;
> + cros_ec_accel_legacy_cells[i].platform_data =
> + &sensor_platforms[i];
> + cros_ec_accel_legacy_cells[i].pdata_size =
> + sizeof(struct cros_ec_sensor_platform);
There is no dynamic information here.
I'd rather you did this all statically.
> + }
> + ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> + cros_ec_accel_legacy_cells,
> + CROS_EC_SENSOR_LEGACY_NUM,
ARRAY_SIZE() is less fragile.
> + NULL, 0, NULL);
> + if (ret)
> + dev_err(ec_dev->dev, "failed to add EC sensors\n");
> +}
> +
> static const struct mfd_cell cros_ec_rtc_cell = {
> .name = "cros-ec-rtc",
> };
> @@ -440,6 +497,9 @@ static int ec_device_probe(struct platform_device *pdev)
> /* check whether this EC is a sensor hub. */
> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> cros_ec_sensors_register(ec);
> + else
> + /* Workaroud for older EC firmware */
> + cros_ec_accel_legacy_register(ec);
>
> /* check whether this EC instance has RTC host command support */
> if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Daniel Hung-yu Wu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [6/8] Add the Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [6/8] This patch is new in this series.
>
> drivers/mfd/cros_ec_dev.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Wei-Ning Huang <[email protected]>
>
> Add ACPI module device table for matching cros-ec devices to load the
> cros_ec_i2c driver automatically.
>
> Signed-off-by: Wei-Ning Huang <[email protected]>
> Acked-by: Benson Leung <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Gwendal Grignou <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [7/8] Add the Reviewed-by Andy Shevchenko.
> - [7/8] Avoid commas in terminator entries.
>
> Changes in v2:
> - [7/8] Add the Reviewed-by Gwendal.
>
> drivers/mfd/cros_ec_i2c.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
> From: Joseph Lo <[email protected]>
>
> 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 will lead the system to crash.
>
> During the test, we also observe that the EC needs to be resumed earlier
> due to some status polling from the EC FW (e.g. battery status). So we
> move the PM ops to late stage to make it work normally.
>
> Signed-off-by: Joseph Lo <[email protected]>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
>
> Changes in v3:
> - [8/8] Add static to cros_ec_i2c_pm_ops.
> - [8/8] Add the Reviewed-by Andy Shevchenko.
>
> Changes in v2:
> - [8/8] This patch is new in this series replacing [5/6] of v1.
>
> drivers/mfd/cros_ec_i2c.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
2018-03-07 17:04 GMT+01:00 Lee Jones <[email protected]>:
> On Mon, 26 Feb 2018, Enric Balletbo i Serra wrote:
>
>> With this patch, the cros_ec_ctl driver will register the legacy
>> accelerometer driver (named cros_ec_accel_legacy) if it fails to
>> register sensors through the usual path cros_ec_sensors_register().
>> This legacy device is present on Chromebook devices with older EC
>> firmware only supporting deprecated EC commands (Glimmer based devices).
>>
>> Tested-by: Gwendal Grignou <[email protected]>
>> Signed-off-by: Enric Balletbo i Serra <[email protected]>
>> Reviewed-by: Gwendal Grignou <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> ---
>>
>> Changes in v3:
>> - [5/8] Add the Reviewed-by Andy Shevchenko.
>>
>> Changes in v2:
>> - [5/8] Add the Reviewed-by Gwendal.
>>
>> drivers/mfd/cros_ec_dev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index 33fe1b439ee2..fd7f0068fb45 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -389,6 +389,63 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>> kfree(msg);
>> }
>>
>> +#define CROS_EC_SENSOR_LEGACY_NUM 2
>> +static struct mfd_cell cros_ec_accel_legacy_cells[CROS_EC_SENSOR_LEGACY_NUM];
>> +
>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
>> +{
>> + struct cros_ec_device *ec_dev = ec->ec_dev;
>> + u8 status;
>> + int i, ret;
>> + struct cros_ec_sensor_platform
>> + sensor_platforms[CROS_EC_SENSOR_LEGACY_NUM];
>
> I wish I had seen this struct before. Yuk!
>
> Why do you even need to know what 'number' the sensor is?
>
>> + /*
>> + * EC that need legacy support are the main EC, directly connected to
>
> Nit: "ECs" or "needs". If you choose "needs" you need to s/are/is/ as
> well.
>
>> + * the AP.
>> + */
>> + if (ec->cmd_offset != 0)
>> + return;
>> +
>> + /*
>> + * Check if EC supports direct memory reads and if EC has
>> + * accelerometers.
>> + */
>> + if (!ec_dev->cmd_readmem)
>> + return;
>> +
>> + ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1, &status);
>> + if (ret < 0) {
>
> Is ret > 0 valid?
>
Yes, ret > 0 is valid.
>> + dev_warn(ec->dev, "EC does not support direct reads.\n");
>> + return;
>> + }
>> +
>> + /* Check if EC has accelerometers. */
>> + if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
>> + dev_info(ec->dev, "EC does not have accelerometers.\n");
>> + return;
>> + }
>> +
>> + /*
>> + * Register 2 accelerometers
>> + */
>> + for (i = 0; i < CROS_EC_SENSOR_LEGACY_NUM; i++) {
>> + cros_ec_accel_legacy_cells[i].name = "cros-ec-accel-legacy";
>> + sensor_platforms[i].sensor_num = i;
>> + cros_ec_accel_legacy_cells[i].id = i;
>> + cros_ec_accel_legacy_cells[i].platform_data =
>> + &sensor_platforms[i];
>> + cros_ec_accel_legacy_cells[i].pdata_size =
>> + sizeof(struct cros_ec_sensor_platform);
>
> There is no dynamic information here.
>
> I'd rather you did this all statically.
>
Ok, I will create a static mfd_cell struct.
>> + }
>> + ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
>> + cros_ec_accel_legacy_cells,
>> + CROS_EC_SENSOR_LEGACY_NUM,
>
> ARRAY_SIZE() is less fragile.
>
Ack.
>> + NULL, 0, NULL);
>> + if (ret)
>> + dev_err(ec_dev->dev, "failed to add EC sensors\n");
>> +}
>> +
>> static const struct mfd_cell cros_ec_rtc_cell = {
>> .name = "cros-ec-rtc",
>> };
>> @@ -440,6 +497,9 @@ static int ec_device_probe(struct platform_device *pdev)
>> /* check whether this EC is a sensor hub. */
>> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
>> cros_ec_sensors_register(ec);
>> + else
>> + /* Workaroud for older EC firmware */
>> + cros_ec_accel_legacy_register(ec);
>>
>> /* check whether this EC instance has RTC host command support */
>> if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
>
> --
> Lee Jones
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
Thanks for the review. I'll prepare a new version of this patchset.
Enric