[PATCH v4 0/8] mfd: cros_ec: add subdevices and fixes

[PATCH v4 0/8] mfd: cros_ec: add subdevices and fixes

[Enric Balletbo i Serra]

Hi,

This is the four 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. This patchset contains some improvements
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 v4:
- [2/8] Add Acked-by Lee Jones.
- [3/8] Add Acked-by Lee Jones.
- [4/8] Nit: check -> Check (Lee Jones)
- [4/8] Use ARRAY_SIZE() instead of hardcoded number (Lee Jones)
- [5/8] Nit: EC -> ECs (Lee Jones)
- [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
- [6/8] Add Acked-by Lee Jones.
- [7/8] Add Acked-by Lee Jones
- [7/8] Remove comma in terminator.
- [8/8] Add Acked-by Lee Jones.

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                 | 103 +++++++++++++++++++++++++++++-
 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, 150 insertions(+), 18 deletions(-)

-- 
2.16.2

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

[Enric Balletbo i Serra]

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>
Acked-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4: None
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.2

[PATCH v4 2/8] mfd: cros_ec: free IRQ automatically

[Enric Balletbo i Serra]

From: Vincent Palatin <vpalatin@chromium.org>

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 <vpalatin@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Vincent Palatin <vpalatin@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4:
- [2/8] Add Acked-by Lee Jones.

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

[PATCH v4 3/8] mfd: cros_ec: Don't try to grab log when suspended

[Enric Balletbo i Serra]

From: Douglas Anderson <dianders@chromium.org>

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 <dianders@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4:
- [3/8] Add Acked-by Lee Jones.

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

[PATCH v4 4/8] mfd: cros_ec_dev: Register cros-ec-rtc driver as a subdevice.

[Enric Balletbo i Serra]

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 <enric.balletbo@collabora.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v4:
- [4/8] Nit: check -> Check (Lee Jones)
- [4/8] Use ARRAY_SIZE() instead of hardcoded number (Lee Jones)

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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index f98e5beffca6..f60a53f11942 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -389,6 +389,12 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 	kfree(msg);
 }
 
+static const struct mfd_cell cros_ec_rtc_cells[] = {
+	{
+		.name = "cros-ec-rtc",
+	}
+};
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -437,6 +443,18 @@ 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_cells,
+					 ARRAY_SIZE(cros_ec_rtc_cells),
+					 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.2

[PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Enric Balletbo i Serra]

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 <gwendal@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v4:
- [5/8] Nit: EC -> ECs (Lee Jones)
- [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)

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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index f60a53f11942..0d541d59d6f5 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 	kfree(msg);
 }
 
+static struct cros_ec_sensor_platform sensor_platforms[] = {
+	{
+		.sensor_num = 0,
+	},
+	{
+		.sensor_num = 1,
+	}
+};
+
+static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
+	{
+		.name = "cros-ec-accel-legacy",
+		.id = 0,
+		.platform_data = &sensor_platforms[0],
+		.pdata_size = sizeof(struct cros_ec_sensor_platform),
+	},
+	{
+		.name = "cros-ec-accel-legacy",
+		.id = 1,
+		.platform_data = &sensor_platforms[1],
+		.pdata_size = sizeof(struct cros_ec_sensor_platform),
+	}
+};
+
+static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
+{
+	struct cros_ec_device *ec_dev = ec->ec_dev;
+	u8 status;
+	int ret;
+
+	/*
+	 * ECs 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
+	 */
+	ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+			      cros_ec_accel_legacy_cells,
+			      ARRAY_SIZE(cros_ec_accel_legacy_cells),
+			      NULL, 0, NULL);
+	if (ret)
+		dev_err(ec_dev->dev, "failed to add EC sensors\n");
+}
+
 static const struct mfd_cell cros_ec_rtc_cells[] = {
 	{
 		.name = "cros-ec-rtc",
@@ -442,6 +509,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.2

[PATCH v4 6/8] mfd: cros_ec_dev: register shutdown function for debugfs

[Enric Balletbo i Serra]

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: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4:
- [6/8] Add Acked-by Lee Jones.

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 0d541d59d6f5..db27743cee8c 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -549,6 +549,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 */ },
@@ -591,6 +599,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.2

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

[Enric Balletbo i Serra]

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>
Acked-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4:
- [7/8] Add Acked-by Lee Jones
- [7/8] Remove comma in terminator.

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_dev.c |  2 +-
 drivers/mfd/cros_ec_i2c.c | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index db27743cee8c..bf1141bb1f61 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -559,7 +559,7 @@ static void ec_device_shutdown(struct platform_device *pdev)
 
 static const struct platform_device_id cros_ec_id[] = {
 	{ DRV_NAME, 0 },
-	{ /* sentinel */ },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, cros_ec_id);
 
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.2

[PATCH v4 8/8] mfd: cros_ec_i2c: moving the system sleep pm ops to late

[Enric Balletbo i Serra]

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 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 <josephl@nvidia.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes in v4:
- [8/8] Add Acked-by Lee Jones.

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

Re: [PATCH v4 4/8] mfd: cros_ec_dev: Register cros-ec-rtc driver as a subdevice.

[Lee Jones]

On Tue, 20 Mar 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 <enric.balletbo@collabora.com>
> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> 
> Changes in v4:
> - [4/8] Nit: check -> Check (Lee Jones)
> - [4/8] Use ARRAY_SIZE() instead of hardcoded number (Lee Jones)
> 
> 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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index f98e5beffca6..f60a53f11942 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -389,6 +389,12 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  	kfree(msg);
>  }
>  
> +static const struct mfd_cell cros_ec_rtc_cells[] = {
> +	{
> +		.name = "cros-ec-rtc",
> +	}

You're going to hate me, but ...

One line and no need for the comma.

	{ .name = "cros-ec-rtc" }

> +};
> +
>  static int ec_device_probe(struct platform_device *pdev)
>  {
>  	int retval = -ENOMEM;
> @@ -437,6 +443,18 @@ 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_cells,
> +					 ARRAY_SIZE(cros_ec_rtc_cells),
> +					 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);
>  

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 4/8] mfd: cros_ec_dev: Register cros-ec-rtc driver as a subdevice.

[Enric Balletbo i Serra]

Hi,

On 28/03/18 12:54, Lee Jones wrote:
> On Tue, 20 Mar 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 <enric.balletbo@collabora.com>
>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> ---
>>
>> Changes in v4:
>> - [4/8] Nit: check -> Check (Lee Jones)
>> - [4/8] Use ARRAY_SIZE() instead of hardcoded number (Lee Jones)
>>
>> 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 | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index f98e5beffca6..f60a53f11942 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -389,6 +389,12 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>>  	kfree(msg);
>>  }
>>  
>> +static const struct mfd_cell cros_ec_rtc_cells[] = {
>> +	{
>> +		.name = "cros-ec-rtc",
>> +	}
> 
> You're going to hate me, but ...
> 

Nope, it's fine :)

I am creating a note with all these things, so hopefully next time I'll do better.

> One line and no need for the comma.
> 
> 	{ .name = "cros-ec-rtc" }
> 
>> +};
>> +

Ok, I'll wait today for if something else arises and send v5 tomorrow.

Thanks,
 Enric

>>  static int ec_device_probe(struct platform_device *pdev)
>>  {
>>  	int retval = -ENOMEM;
>> @@ -437,6 +443,18 @@ 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_cells,
>> +					 ARRAY_SIZE(cros_ec_rtc_cells),
>> +					 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);
>>  
> 

Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Lee Jones]

On Tue, 20 Mar 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 <gwendal@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> 
> Changes in v4:
> - [5/8] Nit: EC -> ECs (Lee Jones)
> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> 
> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index f60a53f11942..0d541d59d6f5 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  	kfree(msg);
>  }
>  
> +static struct cros_ec_sensor_platform sensor_platforms[] = {
> +	{
> +		.sensor_num = 0,
> +	},
> +	{
> +		.sensor_num = 1,
> +	}
> +};

Also no need to be so many  lines.

Each one of these entries can be placed on a single line.

And there's no need for a comma if there is nothing to separate.

	{ .sensor_num = 0 },
	{ .sensor_num = 1 }

Also, this seems like a pretty pointless struct.

What is the sensor_num property used for?

Why does it care what sensor number it is?

> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 0,

What are you using this for?

> +		.platform_data = &sensor_platforms[0],
> +		.pdata_size = sizeof(struct cros_ec_sensor_platform),
> +	},
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 1,

And this?

> +		.platform_data = &sensor_platforms[1],
> +		.pdata_size = sizeof(struct cros_ec_sensor_platform),
> +	}
> +};
> +
> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> +{
> +	struct cros_ec_device *ec_dev = ec->ec_dev;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * ECs 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
> +	 */
> +	ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> +			      cros_ec_accel_legacy_cells,
> +			      ARRAY_SIZE(cros_ec_accel_legacy_cells),
> +			      NULL, 0, NULL);
> +	if (ret)
> +		dev_err(ec_dev->dev, "failed to add EC sensors\n");
> +}
> +
>  static const struct mfd_cell cros_ec_rtc_cells[] = {
>  	{
>  		.name = "cros-ec-rtc",
> @@ -442,6 +509,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

Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Enric Balletbo Serra]

Hi Lee,

2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 20 Mar 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 <gwendal@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> ---
>>
>> Changes in v4:
>> - [5/8] Nit: EC -> ECs (Lee Jones)
>> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
>>
>> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index f60a53f11942..0d541d59d6f5 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>>       kfree(msg);
>>  }
>>
>> +static struct cros_ec_sensor_platform sensor_platforms[] = {
>> +     {
>> +             .sensor_num = 0,
>> +     },
>> +     {
>> +             .sensor_num = 1,
>> +     }
>> +};
>
> Also no need to be so many  lines.
>
> Each one of these entries can be placed on a single line.
>
> And there's no need for a comma if there is nothing to separate.
>
>         { .sensor_num = 0 },
>         { .sensor_num = 1 }
>
> Also, this seems like a pretty pointless struct.
>
> What is the sensor_num property used for?
>
> Why does it care what sensor number it is?
>

I thought that was used but after look again I didn't see where, so
seems that you have reason and this struct is pointless. I'll remove
this and the .id in the cells, we can always send a patch later
introducing this if I am missing something. I'll send another version.

Thanks,
  Enric

>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
>> +     {
>> +             .name = "cros-ec-accel-legacy",
>> +             .id = 0,
>
> What are you using this for?
>
>> +             .platform_data = &sensor_platforms[0],
>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
>> +     },
>> +     {
>> +             .name = "cros-ec-accel-legacy",
>> +             .id = 1,
>
> And this?
>
>> +             .platform_data = &sensor_platforms[1],
>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
>> +     }
>> +};
>> +
>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
>> +{
>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
>> +     u8 status;
>> +     int ret;
>> +
>> +     /*
>> +      * ECs 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
>> +      */
>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
>> +                           cros_ec_accel_legacy_cells,
>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
>> +                           NULL, 0, NULL);
>> +     if (ret)
>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
>> +}
>> +
>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
>>       {
>>               .name = "cros-ec-rtc",
>> @@ -442,6 +509,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

Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Enric Balletbo Serra]

Hi again,

2018-04-04 10:03 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> Hi Lee,
>
> 2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
>> On Tue, 20 Mar 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 <gwendal@chromium.org>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> ---
>>>
>>> Changes in v4:
>>> - [5/8] Nit: EC -> ECs (Lee Jones)
>>> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
>>>
>>> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 70 insertions(+)
>>>
>>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>>> index f60a53f11942..0d541d59d6f5 100644
>>> --- a/drivers/mfd/cros_ec_dev.c
>>> +++ b/drivers/mfd/cros_ec_dev.c
>>> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>>>       kfree(msg);
>>>  }
>>>
>>> +static struct cros_ec_sensor_platform sensor_platforms[] = {
>>> +     {
>>> +             .sensor_num = 0,
>>> +     },
>>> +     {
>>> +             .sensor_num = 1,
>>> +     }
>>> +};
>>
>> Also no need to be so many  lines.
>>
>> Each one of these entries can be placed on a single line.
>>
>> And there's no need for a comma if there is nothing to separate.
>>
>>         { .sensor_num = 0 },
>>         { .sensor_num = 1 }
>>
>> Also, this seems like a pretty pointless struct.
>>
>> What is the sensor_num property used for?
>>
>> Why does it care what sensor number it is?
>>
>
> I thought that was used but after look again I didn't see where, so
> seems that you have reason and this struct is pointless. I'll remove
> this and the .id in the cells, we can always send a patch later
> introducing this if I am missing something. I'll send another version.
>

Ok, forget what I said.

Actually, sensor_num is used by the cros_ec_legacy driver to get the
right sensor properties from the EC and to get the sensor data from
the EC, sensor_num is the offset passed to the EC read command.

.id is not used but as there are two accelerometers that use the same
driver, shouldn't we set the id (or I am missing something)?

+static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
+     { .name = "cros-ec-accel-legacy", .id = 0 },
+     { .name = "cros-ec-accel-legacy", .id = 1 },
+ };

???

Thanks,
  Enric

> Thanks,
>   Enric
>
>>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
>>> +     {
>>> +             .name = "cros-ec-accel-legacy",
>>> +             .id = 0,
>>
>> What are you using this for?
>>
>>> +             .platform_data = &sensor_platforms[0],
>>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
>>> +     },
>>> +     {
>>> +             .name = "cros-ec-accel-legacy",
>>> +             .id = 1,
>>
>> And this?
>>
>>> +             .platform_data = &sensor_platforms[1],
>>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
>>> +     }
>>> +};
>>> +
>>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
>>> +{
>>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
>>> +     u8 status;
>>> +     int ret;
>>> +
>>> +     /*
>>> +      * ECs 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
>>> +      */
>>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
>>> +                           cros_ec_accel_legacy_cells,
>>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
>>> +                           NULL, 0, NULL);
>>> +     if (ret)
>>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
>>> +}
>>> +
>>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
>>>       {
>>>               .name = "cros-ec-rtc",
>>> @@ -442,6 +509,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

Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Lee Jones]

On Wed, 04 Apr 2018, Enric Balletbo Serra wrote:

> Hi again,
> 
> 2018-04-04 10:03 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> > Hi Lee,
> >
> > 2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> >> On Tue, 20 Mar 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 <gwendal@chromium.org>
> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >>> ---
> >>>
> >>> Changes in v4:
> >>> - [5/8] Nit: EC -> ECs (Lee Jones)
> >>> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> >>>
> >>> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 70 insertions(+)
> >>>
> >>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> >>> index f60a53f11942..0d541d59d6f5 100644
> >>> --- a/drivers/mfd/cros_ec_dev.c
> >>> +++ b/drivers/mfd/cros_ec_dev.c
> >>> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> >>>       kfree(msg);
> >>>  }
> >>>
> >>> +static struct cros_ec_sensor_platform sensor_platforms[] = {
> >>> +     {
> >>> +             .sensor_num = 0,
> >>> +     },
> >>> +     {
> >>> +             .sensor_num = 1,
> >>> +     }
> >>> +};
> >>
> >> Also no need to be so many  lines.
> >>
> >> Each one of these entries can be placed on a single line.
> >>
> >> And there's no need for a comma if there is nothing to separate.
> >>
> >>         { .sensor_num = 0 },
> >>         { .sensor_num = 1 }
> >>
> >> Also, this seems like a pretty pointless struct.
> >>
> >> What is the sensor_num property used for?
> >>
> >> Why does it care what sensor number it is?
> >>
> >
> > I thought that was used but after look again I didn't see where, so
> > seems that you have reason and this struct is pointless. I'll remove
> > this and the .id in the cells, we can always send a patch later
> > introducing this if I am missing something. I'll send another version.
> >
> 
> Ok, forget what I said.
> 
> Actually, sensor_num is used by the cros_ec_legacy driver to get the
> right sensor properties from the EC and to get the sensor data from
> the EC, sensor_num is the offset passed to the EC read command.

I'm sure that it is being used, but my question is, why?

Passing a 0 and a 1 to a child driver seems like a pointless exercise
to me.  We do not usually enumerate devices like this with platform
data.  Do these values ever change, or are they simply used to
enumerate 2 devices which are always called 0 and 1?

> .id is not used but as there are two accelerometers that use the same
> driver, shouldn't we set the id (or I am missing something)?
> 
> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> +     { .name = "cros-ec-accel-legacy", .id = 0 },
> +     { .name = "cros-ec-accel-legacy", .id = 1 },
> + };

I think you need to understand what MFD/Platform code does with these
IDs.  Take a look at the MFD/Platform core code to enlighten
yourself.  If there's anything you then do not understand, please
ask and I'll try to fill in the gaps.

> >>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> >>> +     {
> >>> +             .name = "cros-ec-accel-legacy",
> >>> +             .id = 0,
> >>
> >> What are you using this for?
> >>
> >>> +             .platform_data = &sensor_platforms[0],
> >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> >>> +     },
> >>> +     {
> >>> +             .name = "cros-ec-accel-legacy",
> >>> +             .id = 1,
> >>
> >> And this?
> >>
> >>> +             .platform_data = &sensor_platforms[1],
> >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> >>> +     }
> >>> +};
> >>> +
> >>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> >>> +{
> >>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
> >>> +     u8 status;
> >>> +     int ret;
> >>> +
> >>> +     /*
> >>> +      * ECs 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
> >>> +      */
> >>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> >>> +                           cros_ec_accel_legacy_cells,
> >>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
> >>> +                           NULL, 0, NULL);
> >>> +     if (ret)
> >>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
> >>> +}
> >>> +
> >>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> >>>       {
> >>>               .name = "cros-ec-rtc",
> >>> @@ -442,6 +509,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

Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Gwendal Grignou]

On Mon, Apr 16, 2018 at 6:21 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 04 Apr 2018, Enric Balletbo Serra wrote:
>
> > Hi again,
> >
> > 2018-04-04 10:03 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> > > Hi Lee,
> > >
> > > 2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> > >> On Tue, 20 Mar 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 <gwendal@chromium.org>
> > >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > >>> ---
> > >>>
> > >>> Changes in v4:
> > >>> - [5/8] Nit: EC -> ECs (Lee Jones)
> > >>> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > >>>
> > >>> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 70 insertions(+)
> > >>>
> > >>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > >>> index f60a53f11942..0d541d59d6f5 100644
> > >>> --- a/drivers/mfd/cros_ec_dev.c
> > >>> +++ b/drivers/mfd/cros_ec_dev.c
> > >>> @@ -389,6 +389,73 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > >>>       kfree(msg);
> > >>>  }
> > >>>
> > >>> +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > >>> +     {
> > >>> +             .sensor_num = 0,
> > >>> +     },
> > >>> +     {
> > >>> +             .sensor_num = 1,
> > >>> +     }
> > >>> +};
> > >>
> > >> Also no need to be so many  lines.
> > >>
> > >> Each one of these entries can be placed on a single line.
> > >>
> > >> And there's no need for a comma if there is nothing to separate.
> > >>
> > >>         { .sensor_num = 0 },
> > >>         { .sensor_num = 1 }
> > >>
> > >> Also, this seems like a pretty pointless struct.
> > >>
> > >> What is the sensor_num property used for?
> > >>
> > >> Why does it care what sensor number it is?
This patch add support for device with older embedded controller
firmware that do not support the command MOTIONSENSE_CMD_DUMP.
Contrary to cros_ec_sensors_register(), we need to guess what
MOTIONSENSE_CMD_DUMP would have returned when only 2 accelerometers
are present.

The IIO CrosEC sensors driver needs these id created by the EC to
reference a particular sensor: this is sensor_num in
cros_ec_sensor_platform.
It happens to be the same as .id in mfd_cell.
> > >>
> > >
> > > I thought that was used but after look again I didn't see where, so
> > > seems that you have reason and this struct is pointless. I'll remove
> > > this and the .id in the cells, we can always send a patch later
> > > introducing this if I am missing something. I'll send another version.
> > >
> >
> > Ok, forget what I said.
> >
> > Actually, sensor_num is used by the cros_ec_legacy driver to get the
> > right sensor properties from the EC and to get the sensor data from
> > the EC, sensor_num is the offset passed to the EC read command.
>
> I'm sure that it is being used, but my question is, why?
>
> Passing a 0 and a 1 to a child driver seems like a pointless exercise
> to me.  We do not usually enumerate devices like this with platform
> data.  Do these values ever change, or are they simply used to
> enumerate 2 devices which are always called 0 and 1?
The later. The IIO driver needs to use the EC assigned names 0 and 1.
>
> > .id is not used but as there are two accelerometers that use the same
> > driver, shouldn't we set the id (or I am missing something)?
It is just a coincidence that .id and .sensor_num are identical.
> >
> > +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> > +     { .name = "cros-ec-accel-legacy", .id = 0 },
> > +     { .name = "cros-ec-accel-legacy", .id = 1 },
> > + };
I repost a more concise patch.

Thanks,

Gwendal.
>
> I think you need to understand what MFD/Platform code does with these
> IDs.  Take a look at the MFD/Platform core code to enlighten
> yourself.  If there's anything you then do not understand, please
> ask and I'll try to fill in the gaps.
>
> > >>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> > >>> +     {
> > >>> +             .name = "cros-ec-accel-legacy",
> > >>> +             .id = 0,
> > >>
> > >> What are you using this for?
> > >>
> > >>> +             .platform_data = &sensor_platforms[0],
> > >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > >>> +     },
> > >>> +     {
> > >>> +             .name = "cros-ec-accel-legacy",
> > >>> +             .id = 1,
> > >>
> > >> And this?
> > >>
> > >>> +             .platform_data = &sensor_platforms[1],
> > >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > >>> +     }
> > >>> +};
> > >>> +
> > >>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> > >>> +{
> > >>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
> > >>> +     u8 status;
> > >>> +     int ret;
> > >>> +
> > >>> +     /*
> > >>> +      * ECs 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
> > >>> +      */
> > >>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> > >>> +                           cros_ec_accel_legacy_cells,
> > >>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
> > >>> +                           NULL, 0, NULL);
> > >>> +     if (ret)
> > >>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
> > >>> +}
> > >>> +
> > >>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> > >>>       {
> > >>>               .name = "cros-ec-rtc",
> > >>> @@ -442,6 +509,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

[PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>

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 <gwendal@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v5:
- Remove unnecessary white lines.

Changes in v4:
- [5/8] Nit: EC -> ECs (Lee Jones)
- [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)

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 | 66 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index d275deaecb12..64567bd0a081 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 	kfree(msg);
 }
 
+static struct cros_ec_sensor_platform sensor_platforms[] = {
+	{ .sensor_num = 0 },
+	{ .sensor_num = 1 }
+};
+
+static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
+	{
+		.name = "cros-ec-accel-legacy",
+		.id = 0,
+		.platform_data = &sensor_platforms[0],
+		.pdata_size = sizeof(struct cros_ec_sensor_platform),
+	},
+	{
+		.name = "cros-ec-accel-legacy",
+		.id = 1,
+		.platform_data = &sensor_platforms[1],
+		.pdata_size = sizeof(struct cros_ec_sensor_platform),
+	}
+};
+
+static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
+{
+	struct cros_ec_device *ec_dev = ec->ec_dev;
+	u8 status;
+	int ret;
+
+	/*
+	 * ECs 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
+	 */
+	ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+			      cros_ec_accel_legacy_cells,
+			      ARRAY_SIZE(cros_ec_accel_legacy_cells),
+			      NULL, 0, NULL);
+	if (ret)
+		dev_err(ec_dev->dev, "failed to add EC sensors\n");
+}
+
 static const struct mfd_cell cros_ec_cec_cells[] = {
 	{ .name = "cros-ec-cec" }
 };
@@ -437,6 +500,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 CEC host command support */
 	if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
-- 
2.21.0.rc2.261.ga7da99ff1b-goog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones]

On Wed, 27 Feb 2019, Gwendal Grignou wrote:

> From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> 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 <gwendal@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> Changes in v5:
> - Remove unnecessary white lines.
> 
> Changes in v4:
> - [5/8] Nit: EC -> ECs (Lee Jones)
> - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> 
> 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 | 66 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index d275deaecb12..64567bd0a081 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  	kfree(msg);
>  }
>  
> +static struct cros_ec_sensor_platform sensor_platforms[] = {
> +	{ .sensor_num = 0 },
> +	{ .sensor_num = 1 }
> +};

I'm still very uncomfortable with this struct.

Other than these indices, the sensors have no other distinguishing
features, thus there should be no need to identify or distinguish
between them in this way.

> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 0,
> +		.platform_data = &sensor_platforms[0],
> +		.pdata_size = sizeof(struct cros_ec_sensor_platform),
> +	},
> +	{
> +		.name = "cros-ec-accel-legacy",
> +		.id = 1,
> +		.platform_data = &sensor_platforms[1],
> +		.pdata_size = sizeof(struct cros_ec_sensor_platform),
> +	}
> +};
> +
> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> +{
> +	struct cros_ec_device *ec_dev = ec->ec_dev;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * ECs 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
> +	 */
> +	ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,

Using PLATFORM_DEVID_AUTO whilst providing the MFD cells with IDs
doesn't make any sense.  Please remove the IDs from the cells.

> +			      cros_ec_accel_legacy_cells,
> +			      ARRAY_SIZE(cros_ec_accel_legacy_cells),
> +			      NULL, 0, NULL);
> +	if (ret)
> +		dev_err(ec_dev->dev, "failed to add EC sensors\n");
> +}
> +
>  static const struct mfd_cell cros_ec_cec_cells[] = {
>  	{ .name = "cros-ec-cec" }
>  };
> @@ -437,6 +500,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 CEC host command support */
>  	if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 27 Feb 2019, Gwendal Grignou wrote:
>
> > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >
> > 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 <gwendal@chromium.org>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> > Changes in v5:
> > - Remove unnecessary white lines.
> >
> > Changes in v4:
> > - [5/8] Nit: EC -> ECs (Lee Jones)
> > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> >
> > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index d275deaecb12..64567bd0a081 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> >       kfree(msg);
> >  }
> >
> > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > +     { .sensor_num = 0 },
> > +     { .sensor_num = 1 }
> > +};
>
> I'm still very uncomfortable with this struct.
>
> Other than these indices, the sensors have no other distinguishing
> features, thus there should be no need to identify or distinguish
> between them in this way.
When initializing the sensors, the IIO driver expect to find in the
data  structure pointed by dev_get_platdata(dev), in field sensor_num
is stored the index assigned by the embedded controller to talk to a
given sensor.
cros_ec_sensors_register() use the same mechanism; in that function,
the sensor_num field is populated from the output of an EC command
MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
available and in any case we know the EC has only either 2
accelerometers present or nothing.

For instance, let's compare a legacy device with a more recent one:

legacy:
type                  |   id          | sensor_num   | device name
accelerometer  |   0           |   0                  | cros-ec-accel.0
accelerometer  |   1           |   1                  | cros-ec-accel.1

Modern:
type                  |   id          | sensor_num   | device name
accelerometer  |   0           |   0                  | cros-ec-accel.0
accelerometer  |   1           |   1                  | cros-ec-accel.1
gyroscope        |    0          |    2                 | cros-ec-gyro.0
magnetometer |    0          |   3                  | cros-ec-mag.0
light                  |    0          |   4                  | cros-ec-light.0
...

Gwendal.

>
> > +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> > +     {
> > +             .name = "cros-ec-accel-legacy",
> > +             .id = 0,
> > +             .platform_data = &sensor_platforms[0],
> > +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > +     },
> > +     {
> > +             .name = "cros-ec-accel-legacy",
> > +             .id = 1,
> > +             .platform_data = &sensor_platforms[1],
> > +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > +     }
> > +};
> > +
> > +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> > +{
> > +     struct cros_ec_device *ec_dev = ec->ec_dev;
> > +     u8 status;
> > +     int ret;
> > +
> > +     /*
> > +      * ECs 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
> > +      */
> > +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
>
> Using PLATFORM_DEVID_AUTO whilst providing the MFD cells with IDs
> doesn't make any sense.  Please remove the IDs from the cells.
Removed in V6.
>
> > +                           cros_ec_accel_legacy_cells,
> > +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
> > +                           NULL, 0, NULL);
> > +     if (ret)
> > +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
> > +}
> > +
> >  static const struct mfd_cell cros_ec_cec_cells[] = {
> >       { .name = "cros-ec-cec" }
> >  };
> > @@ -437,6 +500,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 CEC host command support */
> >       if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones]

On Tue, 28 May 2019, Gwendal Grignou wrote:

> On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> >
> > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >
> > > 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 <gwendal@chromium.org>
> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > ---
> > > Changes in v5:
> > > - Remove unnecessary white lines.
> > >
> > > Changes in v4:
> > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > >
> > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > index d275deaecb12..64567bd0a081 100644
> > > --- a/drivers/mfd/cros_ec_dev.c
> > > +++ b/drivers/mfd/cros_ec_dev.c
> > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > >       kfree(msg);
> > >  }
> > >
> > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > +     { .sensor_num = 0 },
> > > +     { .sensor_num = 1 }
> > > +};
> >
> > I'm still very uncomfortable with this struct.
> >
> > Other than these indices, the sensors have no other distinguishing
> > features, thus there should be no need to identify or distinguish
> > between them in this way.
> When initializing the sensors, the IIO driver expect to find in the
> data  structure pointed by dev_get_platdata(dev), in field sensor_num
> is stored the index assigned by the embedded controller to talk to a
> given sensor.
> cros_ec_sensors_register() use the same mechanism; in that function,
> the sensor_num field is populated from the output of an EC command
> MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> available and in any case we know the EC has only either 2
> accelerometers present or nothing.
> 
> For instance, let's compare a legacy device with a more recent one:
> 
> legacy:
> type                  |   id          | sensor_num   | device name
> accelerometer  |   0           |   0                  | cros-ec-accel.0
> accelerometer  |   1           |   1                  | cros-ec-accel.1
> 
> Modern:
> type                  |   id          | sensor_num   | device name
> accelerometer  |   0           |   0                  | cros-ec-accel.0
> accelerometer  |   1           |   1                  | cros-ec-accel.1
> gyroscope        |    0          |    2                 | cros-ec-gyro.0
> magnetometer |    0          |   3                  | cros-ec-mag.0
> light                  |    0          |   4                  | cros-ec-light.0
> ...

Why can't these numbers be assigned at runtime?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 28 May 2019, Gwendal Grignou wrote:
>
> > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > >
> > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > >
> > > > 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 <gwendal@chromium.org>
> > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > ---
> > > > Changes in v5:
> > > > - Remove unnecessary white lines.
> > > >
> > > > Changes in v4:
> > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > >
> > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > index d275deaecb12..64567bd0a081 100644
> > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > >       kfree(msg);
> > > >  }
> > > >
> > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > +     { .sensor_num = 0 },
> > > > +     { .sensor_num = 1 }
> > > > +};
> > >
> > > I'm still very uncomfortable with this struct.
> > >
> > > Other than these indices, the sensors have no other distinguishing
> > > features, thus there should be no need to identify or distinguish
> > > between them in this way.
> > When initializing the sensors, the IIO driver expect to find in the
> > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > is stored the index assigned by the embedded controller to talk to a
> > given sensor.
> > cros_ec_sensors_register() use the same mechanism; in that function,
> > the sensor_num field is populated from the output of an EC command
> > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > available and in any case we know the EC has only either 2
> > accelerometers present or nothing.
> >
> > For instance, let's compare a legacy device with a more recent one:
> >
> > legacy:
> > type                  |   id          | sensor_num   | device name
> > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > accelerometer  |   1           |   1                  | cros-ec-accel.1
> >
> > Modern:
> > type                  |   id          | sensor_num   | device name
> > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > magnetometer |    0          |   3                  | cros-ec-mag.0
> > light                  |    0          |   4                  | cros-ec-light.0
> > ...
>
> Why can't these numbers be assigned at runtime?
I assume you want to know why IIO drivers need to know "sensor_num"
ahead of time. It is because each IIO driver is independent from the
other.
Let assume there was 2 light sensors in the device:
type                  |   id          | sensor_num   | device name
 light                  |    0          |   4                  | cros-ec-light.0
 light                  |    1          |   5                  | cros-ec-light.1

In case of sensors of the same type without sensor_num, cros-ec-light
driver has no information at probe time if it should bind to sensors
named by the EC 4 or 5.

We could get away with cros-ec-accel, as EC always presents
accelerometers with sensor_num  0 and 1, but I don't want to rely on
this property in the general case.
Only cros_ec_dev MFD driver has the global view of all sensors available.

>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones]

On Wed, 29 May 2019, Gwendal Grignou wrote:

> On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 28 May 2019, Gwendal Grignou wrote:
> >
> > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > >
> > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > >
> > > > > 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 <gwendal@chromium.org>
> > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > ---
> > > > > Changes in v5:
> > > > > - Remove unnecessary white lines.
> > > > >
> > > > > Changes in v4:
> > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > >
> > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 66 insertions(+)
> > > > >
> > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > index d275deaecb12..64567bd0a081 100644
> > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > >       kfree(msg);
> > > > >  }
> > > > >
> > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > +     { .sensor_num = 0 },
> > > > > +     { .sensor_num = 1 }
> > > > > +};
> > > >
> > > > I'm still very uncomfortable with this struct.
> > > >
> > > > Other than these indices, the sensors have no other distinguishing
> > > > features, thus there should be no need to identify or distinguish
> > > > between them in this way.
> > > When initializing the sensors, the IIO driver expect to find in the
> > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > is stored the index assigned by the embedded controller to talk to a
> > > given sensor.
> > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > the sensor_num field is populated from the output of an EC command
> > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > available and in any case we know the EC has only either 2
> > > accelerometers present or nothing.
> > >
> > > For instance, let's compare a legacy device with a more recent one:
> > >
> > > legacy:
> > > type                  |   id          | sensor_num   | device name
> > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > >
> > > Modern:
> > > type                  |   id          | sensor_num   | device name
> > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > light                  |    0          |   4                  | cros-ec-light.0
> > > ...
> >
> > Why can't these numbers be assigned at runtime?
> I assume you want to know why IIO drivers need to know "sensor_num"
> ahead of time. It is because each IIO driver is independent from the
> other.
> Let assume there was 2 light sensors in the device:
> type                  |   id          | sensor_num   | device name
>  light                  |    0          |   4                  | cros-ec-light.0
>  light                  |    1          |   5                  | cros-ec-light.1
> 
> In case of sensors of the same type without sensor_num, cros-ec-light
> driver has no information at probe time if it should bind to sensors
> named by the EC 4 or 5.
> 
> We could get away with cros-ec-accel, as EC always presents
> accelerometers with sensor_num  0 and 1, but I don't want to rely on
> this property in the general case.
> Only cros_ec_dev MFD driver has the global view of all sensors available.

Well seeing as this implementation has already been accepted and you're
only *using* it, rather than creating it, I think this conversation is
moot.  It looks like the original implementation patch was not
reviewed by me, which is frustrating since I would have NACKed it.

Just so you know, pointlessly enumerating identical devices manually
is not a good practice.  It is one we reject all the time.  This
imp. should too have been rejected on submission.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 29 May 2019, Gwendal Grignou wrote:
>
> > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > >
> > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > >
> > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > >
> > > > > > 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 <gwendal@chromium.org>
> > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > ---
> > > > > > Changes in v5:
> > > > > > - Remove unnecessary white lines.
> > > > > >
> > > > > > Changes in v4:
> > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > >
> > > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 66 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > >       kfree(msg);
> > > > > >  }
> > > > > >
> > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > +     { .sensor_num = 0 },
> > > > > > +     { .sensor_num = 1 }
> > > > > > +};
> > > > >
> > > > > I'm still very uncomfortable with this struct.
> > > > >
> > > > > Other than these indices, the sensors have no other distinguishing
> > > > > features, thus there should be no need to identify or distinguish
> > > > > between them in this way.
> > > > When initializing the sensors, the IIO driver expect to find in the
> > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > is stored the index assigned by the embedded controller to talk to a
> > > > given sensor.
> > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > the sensor_num field is populated from the output of an EC command
> > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > available and in any case we know the EC has only either 2
> > > > accelerometers present or nothing.
> > > >
> > > > For instance, let's compare a legacy device with a more recent one:
> > > >
> > > > legacy:
> > > > type                  |   id          | sensor_num   | device name
> > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > >
> > > > Modern:
> > > > type                  |   id          | sensor_num   | device name
> > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > ...
> > >
> > > Why can't these numbers be assigned at runtime?
> > I assume you want to know why IIO drivers need to know "sensor_num"
> > ahead of time. It is because each IIO driver is independent from the
> > other.
> > Let assume there was 2 light sensors in the device:
> > type                  |   id          | sensor_num   | device name
> >  light                  |    0          |   4                  | cros-ec-light.0
> >  light                  |    1          |   5                  | cros-ec-light.1
> >
> > In case of sensors of the same type without sensor_num, cros-ec-light
> > driver has no information at probe time if it should bind to sensors
> > named by the EC 4 or 5.
> >
> > We could get away with cros-ec-accel, as EC always presents
> > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > this property in the general case.
> > Only cros_ec_dev MFD driver has the global view of all sensors available.
>
> Well seeing as this implementation has already been accepted and you're
> only *using* it, rather than creating it, I think this conversation is
> moot.  It looks like the original implementation patch was not
> reviewed by me, which is frustrating since I would have NACKed it.
>
> Just so you know, pointlessly enumerating identical devices manually
> is not a good practice.  It is one we reject all the time.  This
> imp. should too have been rejected on submission.
I wrote the original code, Enric submitted it, so I am not just using it.
We can work on implementing the right way. Which model should I follow?
The code function is similar to HID sensor hub code which is done in
driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
mfd_add_hotplug_devices() with an array of mfd_cell,
hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
structure that is shared between the iio driver and the hid sensor hub
driver. hsdev->usage information is sent back and forth between
specialized hid IIO device driver and the HID sensor hub driver, for
example when sensor_hub_input_attr_get_raw_value() is called.
hsdev->usage has the same usage a sensor_num I am using.

I am not enumerating identical devices twice: the embedded controller
manages a list of sensors:

For instance on pixelbook, it look like:
       +--------+
        | EC    |
       +--------+
   ( via several i2c/spi buses)
+--------------------+--------------+-------- ...
|                         |                  |
IMU (base)     light/prox    Accelrometer (lid)
|
Magnetometer

A given hardware sensor may be composed of multiple logical sensors
(IMU is a accelerometer and a gyroscope into one package).

The EC firmware list all the (logical) sensors in array, and that
unique index - sensor_num - points to a single logical sensor.

Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
assigning .id myself?
The topology will look like:
find . -type d -name \*auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-logger.8.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-accel.2.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.4.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-charger.7.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.3.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-mag.5.auto
./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-ring.6.auto
./devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-GOOG0008:00/cros-ec-dev.0.auto

Thank you for your support,

Gwendal.





>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones]

On Thu, 30 May 2019, Gwendal Grignou wrote:

> On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 29 May 2019, Gwendal Grignou wrote:
> >
> > > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > > >
> > > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > > >
> > > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > >
> > > > > > > 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 <gwendal@chromium.org>
> > > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > ---
> > > > > > > Changes in v5:
> > > > > > > - Remove unnecessary white lines.
> > > > > > >
> > > > > > > Changes in v4:
> > > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > > >
> > > > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 66 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > > >       kfree(msg);
> > > > > > >  }
> > > > > > >
> > > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > > +     { .sensor_num = 0 },
> > > > > > > +     { .sensor_num = 1 }
> > > > > > > +};
> > > > > >
> > > > > > I'm still very uncomfortable with this struct.
> > > > > >
> > > > > > Other than these indices, the sensors have no other distinguishing
> > > > > > features, thus there should be no need to identify or distinguish
> > > > > > between them in this way.
> > > > > When initializing the sensors, the IIO driver expect to find in the
> > > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > > is stored the index assigned by the embedded controller to talk to a
> > > > > given sensor.
> > > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > > the sensor_num field is populated from the output of an EC command
> > > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > > available and in any case we know the EC has only either 2
> > > > > accelerometers present or nothing.
> > > > >
> > > > > For instance, let's compare a legacy device with a more recent one:
> > > > >
> > > > > legacy:
> > > > > type                  |   id          | sensor_num   | device name
> > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > >
> > > > > Modern:
> > > > > type                  |   id          | sensor_num   | device name
> > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > > ...
> > > >
> > > > Why can't these numbers be assigned at runtime?
> > > I assume you want to know why IIO drivers need to know "sensor_num"
> > > ahead of time. It is because each IIO driver is independent from the
> > > other.
> > > Let assume there was 2 light sensors in the device:
> > > type                  |   id          | sensor_num   | device name
> > >  light                  |    0          |   4                  | cros-ec-light.0
> > >  light                  |    1          |   5                  | cros-ec-light.1
> > >
> > > In case of sensors of the same type without sensor_num, cros-ec-light
> > > driver has no information at probe time if it should bind to sensors
> > > named by the EC 4 or 5.
> > >
> > > We could get away with cros-ec-accel, as EC always presents
> > > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > > this property in the general case.
> > > Only cros_ec_dev MFD driver has the global view of all sensors available.
> >
> > Well seeing as this implementation has already been accepted and you're
> > only *using* it, rather than creating it, I think this conversation is
> > moot.  It looks like the original implementation patch was not
> > reviewed by me, which is frustrating since I would have NACKed it.
> >
> > Just so you know, pointlessly enumerating identical devices manually
> > is not a good practice.  It is one we reject all the time.  This
> > imp. should too have been rejected on submission.

> I wrote the original code, Enric submitted it, so I am not just using it.

My point was, *this* patch is just using it.  The implementation has
already been applied to the mainline kernel.  Who wrote the initial
commit is not important at this point.

> We can work on implementing the right way. Which model should I follow?
> The code function is similar to HID sensor hub code which is done in
> driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
> mfd_add_hotplug_devices() with an array of mfd_cell,
> hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
> structure that is shared between the iio driver and the hid sensor hub
> driver. hsdev->usage information is sent back and forth between
> specialized hid IIO device driver and the HID sensor hub driver, for
> example when sensor_hub_input_attr_get_raw_value() is called.
> hsdev->usage has the same usage a sensor_num I am using.

It looks like the HID Usage implementation is using a set of
pre-defined values to identify sensor *types*:

  include/linux/hid-sensor-ids.h

Where as your implementation is confusing me.  In some instances you
are using it as what looks like an *index* into a register set:

  ec_cmd_read_u16(st->ec,
                EC_MEMMAP_ACC_DATA +
                sizeof(s16) *
                (1 + i + st->sensor_num * MAX_AXIS),
                data);

And at other times it is used for sensor *types*, but in a very
limited way:

  enum motionsensor_location {
          MOTIONSENSE_LOC_BASE = 0,
          MOTIONSENSE_LOC_LID = 1,
          MOTIONSENSE_LOC_MAX,
  };

  static char *cros_ec_accel_legacy_loc_strings[] = {
          [MOTIONSENSE_LOC_BASE] = "base",
          [MOTIONSENSE_LOC_LID] = "lid",
          [MOTIONSENSE_LOC_MAX] = "unknown",
  };

  return sprintf(buf, "%s\n",
                 cros_ec_accel_legacy_loc_strings[st->sensor_num +
                                                MOTIONSENSE_LOC_BASE]);

> I am not enumerating identical devices twice: the embedded controller
> manages a list of sensors:
> 
> For instance on pixelbook, it look like:
>        +--------+
>         | EC    |
>        +--------+
>    ( via several i2c/spi buses)
> +--------------------+--------------+-------- ...
> |                         |                  |
> IMU (base)     light/prox    Accelrometer (lid)
> |
> Magnetometer
> 
> A given hardware sensor may be composed of multiple logical sensors
> (IMU is a accelerometer and a gyroscope into one package).
> 
> The EC firmware list all the (logical) sensors in array, and that
> unique index - sensor_num - points to a single logical sensor.

What what is 'sensor_num'; is it a channel address/number similar to
what I2C HIDs use to communicate over a specific I2C line, or is it a
type, similar to what HID devices provide on request for
identification purposes? 

> Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
> assigning .id myself?

Is this a separate question, or can 'sensor_num' be any unique
arbitrary number?

> The topology will look like:
> find . -type d -name \*auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-logger.8.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-accel.2.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.4.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-charger.7.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.3.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-mag.5.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-ring.6.auto
> ./devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-GOOG0008:00/cros-ec-dev.0.auto
> 
> Thank you for your support,

No problem.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

On Fri, May 31, 2019 at 1:13 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 30 May 2019, Gwendal Grignou wrote:
>
> > On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 29 May 2019, Gwendal Grignou wrote:
> > >
> > > > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > > > >
> > > > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > > > >
> > > > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > >
> > > > > > > > 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 <gwendal@chromium.org>
> > > > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > > ---
> > > > > > > > Changes in v5:
> > > > > > > > - Remove unnecessary white lines.
> > > > > > > >
> > > > > > > > Changes in v4:
> > > > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > > > >
> > > > > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 66 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > > > >       kfree(msg);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > > > +     { .sensor_num = 0 },
> > > > > > > > +     { .sensor_num = 1 }
> > > > > > > > +};
> > > > > > >
> > > > > > > I'm still very uncomfortable with this struct.
> > > > > > >
> > > > > > > Other than these indices, the sensors have no other distinguishing
> > > > > > > features, thus there should be no need to identify or distinguish
> > > > > > > between them in this way.
> > > > > > When initializing the sensors, the IIO driver expect to find in the
> > > > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > > > is stored the index assigned by the embedded controller to talk to a
> > > > > > given sensor.
> > > > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > > > the sensor_num field is populated from the output of an EC command
> > > > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > > > available and in any case we know the EC has only either 2
> > > > > > accelerometers present or nothing.
> > > > > >
> > > > > > For instance, let's compare a legacy device with a more recent one:
> > > > > >
> > > > > > legacy:
> > > > > > type                  |   id          | sensor_num   | device name
> > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > >
> > > > > > Modern:
> > > > > > type                  |   id          | sensor_num   | device name
> > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > > > ...
> > > > >
> > > > > Why can't these numbers be assigned at runtime?
> > > > I assume you want to know why IIO drivers need to know "sensor_num"
> > > > ahead of time. It is because each IIO driver is independent from the
> > > > other.
> > > > Let assume there was 2 light sensors in the device:
> > > > type                  |   id          | sensor_num   | device name
> > > >  light                  |    0          |   4                  | cros-ec-light.0
> > > >  light                  |    1          |   5                  | cros-ec-light.1
> > > >
> > > > In case of sensors of the same type without sensor_num, cros-ec-light
> > > > driver has no information at probe time if it should bind to sensors
> > > > named by the EC 4 or 5.
> > > >
> > > > We could get away with cros-ec-accel, as EC always presents
> > > > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > > > this property in the general case.
> > > > Only cros_ec_dev MFD driver has the global view of all sensors available.
> > >
> > > Well seeing as this implementation has already been accepted and you're
> > > only *using* it, rather than creating it, I think this conversation is
> > > moot.  It looks like the original implementation patch was not
> > > reviewed by me, which is frustrating since I would have NACKed it.
> > >
> > > Just so you know, pointlessly enumerating identical devices manually
> > > is not a good practice.  It is one we reject all the time.  This
> > > imp. should too have been rejected on submission.
>
> > I wrote the original code, Enric submitted it, so I am not just using it.
>
> My point was, *this* patch is just using it.  The implementation has
> already been applied to the mainline kernel.  Who wrote the initial
> commit is not important at this point.
>
> > We can work on implementing the right way. Which model should I follow?
> > The code function is similar to HID sensor hub code which is done in
> > driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
> > mfd_add_hotplug_devices() with an array of mfd_cell,
> > hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
> > structure that is shared between the iio driver and the hid sensor hub
> > driver. hsdev->usage information is sent back and forth between
> > specialized hid IIO device driver and the HID sensor hub driver, for
> > example when sensor_hub_input_attr_get_raw_value() is called.
> > hsdev->usage has the same usage a sensor_num I am using.
>
> It looks like the HID Usage implementation is using a set of
> pre-defined values to identify sensor *types*:
>
>   include/linux/hid-sensor-ids.h
Yes, hsdev->usage, aka usage_id define the types between 0x00 and 0xFF.
For accessing a paritcualre fileds, we use attr_usage_id, between
0x100 and 0x7FF.
AFAIK, a sensorhub/collection can not contain to sensor of the same type.
>
> Where as your implementation is confusing me.  In some instances you
> are using it as what looks like an *index* into a register set:
>
>   ec_cmd_read_u16(st->ec,
>                 EC_MEMMAP_ACC_DATA +
>                 sizeof(s16) *
>                 (1 + i + st->sensor_num * MAX_AXIS),
>                 data);
>
> And at other times it is used for sensor *types*, but in a very
> limited way:
>
>   enum motionsensor_location {
>           MOTIONSENSE_LOC_BASE = 0,
>           MOTIONSENSE_LOC_LID = 1,
>           MOTIONSENSE_LOC_MAX,
>   };
>
>   static char *cros_ec_accel_legacy_loc_strings[] = {
>           [MOTIONSENSE_LOC_BASE] = "base",
>           [MOTIONSENSE_LOC_LID] = "lid",
>           [MOTIONSENSE_LOC_MAX] = "unknown",
>   };
>
>   return sprintf(buf, "%s\n",
>                  cros_ec_accel_legacy_loc_strings[st->sensor_num +
>                                                 MOTIONSENSE_LOC_BASE]);
In the legacy case, the location is hard-coded from sensor_num, the
index used by the EC: sensor 0 is in base, 1 is in the lid.
This limitation is removed from newer EC implementation, the EC
subcommand MOTIONSENSE_CMD_INFO provide that information.
>
> > I am not enumerating identical devices twice: the embedded controller
> > manages a list of sensors:
> >
> > For instance on pixelbook, it look like:
> >        +--------+
> >         | EC    |
> >        +--------+
> >    ( via several i2c/spi buses)
> > +--------------------+--------------+-------- ...
> > |                         |                  |
> > IMU (base)     light/prox    Accelrometer (lid)
> > |
> > Magnetometer
> >
> > A given hardware sensor may be composed of multiple logical sensors
> > (IMU is a accelerometer and a gyroscope into one package).
> >
> > The EC firmware list all the (logical) sensors in array, and that
> > unique index - sensor_num - points to a single logical sensor.
>
> What what is 'sensor_num'; is it a channel address/number similar to
> what I2C HIDs use to communicate over a specific I2C line, or is it a
> type, similar to what HID devices provide on request for
> identification purposes?
>
> > Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
> > assigning .id myself?
>
> Is this a separate question, or can 'sensor_num' be any unique
> arbitrary number?
No, it is assigned by the EC.

Thanks for your time,

Gwendal.
>
> > The topology will look like:
> > find . -type d -name \*auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-logger.8.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-accel.2.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.4.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-charger.7.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.3.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-mag.5.auto
> > ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-ring.6.auto
> > ./devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-GOOG0008:00/cros-ec-dev.0.auto
> >
> > Thank you for your support,
>
> No problem.
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones]

On Fri, 31 May 2019, Gwendal Grignou wrote:

> On Fri, May 31, 2019 at 1:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 30 May 2019, Gwendal Grignou wrote:
> >
> > > On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Wed, 29 May 2019, Gwendal Grignou wrote:
> > > >
> > > > > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > > > > >
> > > > > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > > > > >
> > > > > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > > >
> > > > > > > > > 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 <gwendal@chromium.org>
> > > > > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > > > ---
> > > > > > > > > Changes in v5:
> > > > > > > > > - Remove unnecessary white lines.
> > > > > > > > >
> > > > > > > > > Changes in v4:
> > > > > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > > > > >
> > > > > > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 66 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > > > > >       kfree(msg);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > > > > +     { .sensor_num = 0 },
> > > > > > > > > +     { .sensor_num = 1 }
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > I'm still very uncomfortable with this struct.
> > > > > > > >
> > > > > > > > Other than these indices, the sensors have no other distinguishing
> > > > > > > > features, thus there should be no need to identify or distinguish
> > > > > > > > between them in this way.
> > > > > > > When initializing the sensors, the IIO driver expect to find in the
> > > > > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > > > > is stored the index assigned by the embedded controller to talk to a
> > > > > > > given sensor.
> > > > > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > > > > the sensor_num field is populated from the output of an EC command
> > > > > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > > > > available and in any case we know the EC has only either 2
> > > > > > > accelerometers present or nothing.
> > > > > > >
> > > > > > > For instance, let's compare a legacy device with a more recent one:
> > > > > > >
> > > > > > > legacy:
> > > > > > > type                  |   id          | sensor_num   | device name
> > > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > > >
> > > > > > > Modern:
> > > > > > > type                  |   id          | sensor_num   | device name
> > > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > > > > ...
> > > > > >
> > > > > > Why can't these numbers be assigned at runtime?
> > > > > I assume you want to know why IIO drivers need to know "sensor_num"
> > > > > ahead of time. It is because each IIO driver is independent from the
> > > > > other.
> > > > > Let assume there was 2 light sensors in the device:
> > > > > type                  |   id          | sensor_num   | device name
> > > > >  light                  |    0          |   4                  | cros-ec-light.0
> > > > >  light                  |    1          |   5                  | cros-ec-light.1
> > > > >
> > > > > In case of sensors of the same type without sensor_num, cros-ec-light
> > > > > driver has no information at probe time if it should bind to sensors
> > > > > named by the EC 4 or 5.
> > > > >
> > > > > We could get away with cros-ec-accel, as EC always presents
> > > > > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > > > > this property in the general case.
> > > > > Only cros_ec_dev MFD driver has the global view of all sensors available.
> > > >
> > > > Well seeing as this implementation has already been accepted and you're
> > > > only *using* it, rather than creating it, I think this conversation is
> > > > moot.  It looks like the original implementation patch was not
> > > > reviewed by me, which is frustrating since I would have NACKed it.
> > > >
> > > > Just so you know, pointlessly enumerating identical devices manually
> > > > is not a good practice.  It is one we reject all the time.  This
> > > > imp. should too have been rejected on submission.
> >
> > > I wrote the original code, Enric submitted it, so I am not just using it.
> >
> > My point was, *this* patch is just using it.  The implementation has
> > already been applied to the mainline kernel.  Who wrote the initial
> > commit is not important at this point.
> >
> > > We can work on implementing the right way. Which model should I follow?
> > > The code function is similar to HID sensor hub code which is done in
> > > driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
> > > mfd_add_hotplug_devices() with an array of mfd_cell,
> > > hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
> > > structure that is shared between the iio driver and the hid sensor hub
> > > driver. hsdev->usage information is sent back and forth between
> > > specialized hid IIO device driver and the HID sensor hub driver, for
> > > example when sensor_hub_input_attr_get_raw_value() is called.
> > > hsdev->usage has the same usage a sensor_num I am using.
> >
> > It looks like the HID Usage implementation is using a set of
> > pre-defined values to identify sensor *types*:
> >
> >   include/linux/hid-sensor-ids.h
> Yes, hsdev->usage, aka usage_id define the types between 0x00 and 0xFF.
> For accessing a paritcualre fileds, we use attr_usage_id, between
> 0x100 and 0x7FF.
> AFAIK, a sensorhub/collection can not contain to sensor of the same type.
> >
> > Where as your implementation is confusing me.  In some instances you
> > are using it as what looks like an *index* into a register set:
> >
> >   ec_cmd_read_u16(st->ec,
> >                 EC_MEMMAP_ACC_DATA +
> >                 sizeof(s16) *
> >                 (1 + i + st->sensor_num * MAX_AXIS),
> >                 data);
> >
> > And at other times it is used for sensor *types*, but in a very
> > limited way:
> >
> >   enum motionsensor_location {
> >           MOTIONSENSE_LOC_BASE = 0,
> >           MOTIONSENSE_LOC_LID = 1,
> >           MOTIONSENSE_LOC_MAX,
> >   };
> >
> >   static char *cros_ec_accel_legacy_loc_strings[] = {
> >           [MOTIONSENSE_LOC_BASE] = "base",
> >           [MOTIONSENSE_LOC_LID] = "lid",
> >           [MOTIONSENSE_LOC_MAX] = "unknown",
> >   };
> >
> >   return sprintf(buf, "%s\n",
> >                  cros_ec_accel_legacy_loc_strings[st->sensor_num +
> >                                                 MOTIONSENSE_LOC_BASE]);
> In the legacy case, the location is hard-coded from sensor_num, the
> index used by the EC: sensor 0 is in base, 1 is in the lid.
> This limitation is removed from newer EC implementation, the EC
> subcommand MOTIONSENSE_CMD_INFO provide that information.
> >
> > > I am not enumerating identical devices twice: the embedded controller
> > > manages a list of sensors:
> > >
> > > For instance on pixelbook, it look like:
> > >        +--------+
> > >         | EC    |
> > >        +--------+
> > >    ( via several i2c/spi buses)
> > > +--------------------+--------------+-------- ...
> > > |                         |                  |
> > > IMU (base)     light/prox    Accelrometer (lid)
> > > |
> > > Magnetometer
> > >
> > > A given hardware sensor may be composed of multiple logical sensors
> > > (IMU is a accelerometer and a gyroscope into one package).
> > >
> > > The EC firmware list all the (logical) sensors in array, and that
> > > unique index - sensor_num - points to a single logical sensor.
> >
> > What what is 'sensor_num'; is it a channel address/number similar to
> > what I2C HIDs use to communicate over a specific I2C line, or is it a
> > type, similar to what HID devices provide on request for
> > identification purposes?
> >
> > > Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
> > > assigning .id myself?
> >
> > Is this a separate question, or can 'sensor_num' be any unique
> > arbitrary number?
> No, it is assigned by the EC.

Is it assigned *by* the EC or *to* the EC.

If it is assigned *by* the EC, can't you ask the EC for it?

I asked this before, but was not given the answer I wanted:

> > Why can't these numbers be assigned at runtime?
> I assume you want to know why IIO drivers need to know "sensor_num"
> ahead of time. It is because each IIO driver is independent from the
> other.

Is there a way to call into the EC and request the number?

int cros_ec_allocate_sensor_id(enum cros_sensor_type);

Which allocates the next available ID of the requested type.

OR

If this is not possible AND the sensors of the same type are identical
AND the sensors are always numbered sequentially from the same base
(i.e. 0, 1, 2 OR 1, 2, 3, etc) then you use the ida_ API
(include/linux/idr.h) to provide you with a unique ID for your
sensor(s).

###

The flip side is a situation where the devices of a same type are not
identical and are provided different platform data (by the parent MFD
in this case).  If this true then you may actually need to identify
the specific sensor ahead of time, in which case it's the chosen
nomenclature that is misleading.  What you might really be looking for
is a sensor_id and proper commenting/documentation. 

###

However, looking at this patch, I suspect the former situation to be
the case here, thus there is no requirement for the parent to
pre-allocate 'sensor_num's.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Gwendal Grignou]

On Sun, Jun 2, 2019 at 11:23 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 31 May 2019, Gwendal Grignou wrote:
>
> > On Fri, May 31, 2019 at 1:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Thu, 30 May 2019, Gwendal Grignou wrote:
> > >
> > > > On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Wed, 29 May 2019, Gwendal Grignou wrote:
> > > > >
> > > > > > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > > > > > >
> > > > > > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > > > > > >
> > > > > > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > > > >
> > > > > > > > > > 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 <gwendal@chromium.org>
> > > > > > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > > Changes in v5:
> > > > > > > > > > - Remove unnecessary white lines.
> > > > > > > > > >
> > > > > > > > > > Changes in v4:
> > > > > > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > > > > > >
> > > > > > > > > > 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 | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  1 file changed, 66 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > > > > > >       kfree(msg);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > > > > > +     { .sensor_num = 0 },
> > > > > > > > > > +     { .sensor_num = 1 }
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > I'm still very uncomfortable with this struct.
> > > > > > > > >
> > > > > > > > > Other than these indices, the sensors have no other distinguishing
> > > > > > > > > features, thus there should be no need to identify or distinguish
> > > > > > > > > between them in this way.
> > > > > > > > When initializing the sensors, the IIO driver expect to find in the
> > > > > > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > > > > > is stored the index assigned by the embedded controller to talk to a
> > > > > > > > given sensor.
> > > > > > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > > > > > the sensor_num field is populated from the output of an EC command
> > > > > > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > > > > > available and in any case we know the EC has only either 2
> > > > > > > > accelerometers present or nothing.
> > > > > > > >
> > > > > > > > For instance, let's compare a legacy device with a more recent one:
> > > > > > > >
> > > > > > > > legacy:
> > > > > > > > type                  |   id          | sensor_num   | device name
> > > > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > > > >
> > > > > > > > Modern:
> > > > > > > > type                  |   id          | sensor_num   | device name
> > > > > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > > > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > > > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > > > > > ...
> > > > > > >
> > > > > > > Why can't these numbers be assigned at runtime?
> > > > > > I assume you want to know why IIO drivers need to know "sensor_num"
> > > > > > ahead of time. It is because each IIO driver is independent from the
> > > > > > other.
> > > > > > Let assume there was 2 light sensors in the device:
> > > > > > type                  |   id          | sensor_num   | device name
> > > > > >  light                  |    0          |   4                  | cros-ec-light.0
> > > > > >  light                  |    1          |   5                  | cros-ec-light.1
> > > > > >
> > > > > > In case of sensors of the same type without sensor_num, cros-ec-light
> > > > > > driver has no information at probe time if it should bind to sensors
> > > > > > named by the EC 4 or 5.
> > > > > >
> > > > > > We could get away with cros-ec-accel, as EC always presents
> > > > > > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > > > > > this property in the general case.
> > > > > > Only cros_ec_dev MFD driver has the global view of all sensors available.
> > > > >
> > > > > Well seeing as this implementation has already been accepted and you're
> > > > > only *using* it, rather than creating it, I think this conversation is
> > > > > moot.  It looks like the original implementation patch was not
> > > > > reviewed by me, which is frustrating since I would have NACKed it.
> > > > >
> > > > > Just so you know, pointlessly enumerating identical devices manually
> > > > > is not a good practice.  It is one we reject all the time.  This
> > > > > imp. should too have been rejected on submission.
> > >
> > > > I wrote the original code, Enric submitted it, so I am not just using it.
> > >
> > > My point was, *this* patch is just using it.  The implementation has
> > > already been applied to the mainline kernel.  Who wrote the initial
> > > commit is not important at this point.
> > >
> > > > We can work on implementing the right way. Which model should I follow?
> > > > The code function is similar to HID sensor hub code which is done in
> > > > driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
> > > > mfd_add_hotplug_devices() with an array of mfd_cell,
> > > > hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
> > > > structure that is shared between the iio driver and the hid sensor hub
> > > > driver. hsdev->usage information is sent back and forth between
> > > > specialized hid IIO device driver and the HID sensor hub driver, for
> > > > example when sensor_hub_input_attr_get_raw_value() is called.
> > > > hsdev->usage has the same usage a sensor_num I am using.
> > >
> > > It looks like the HID Usage implementation is using a set of
> > > pre-defined values to identify sensor *types*:
> > >
> > >   include/linux/hid-sensor-ids.h
> > Yes, hsdev->usage, aka usage_id define the types between 0x00 and 0xFF.
> > For accessing a paritcualre fileds, we use attr_usage_id, between
> > 0x100 and 0x7FF.
> > AFAIK, a sensorhub/collection can not contain to sensor of the same type.
> > >
> > > Where as your implementation is confusing me.  In some instances you
> > > are using it as what looks like an *index* into a register set:
> > >
> > >   ec_cmd_read_u16(st->ec,
> > >                 EC_MEMMAP_ACC_DATA +
> > >                 sizeof(s16) *
> > >                 (1 + i + st->sensor_num * MAX_AXIS),
> > >                 data);
> > >
> > > And at other times it is used for sensor *types*, but in a very
> > > limited way:
> > >
> > >   enum motionsensor_location {
> > >           MOTIONSENSE_LOC_BASE = 0,
> > >           MOTIONSENSE_LOC_LID = 1,
> > >           MOTIONSENSE_LOC_MAX,
> > >   };
> > >
> > >   static char *cros_ec_accel_legacy_loc_strings[] = {
> > >           [MOTIONSENSE_LOC_BASE] = "base",
> > >           [MOTIONSENSE_LOC_LID] = "lid",
> > >           [MOTIONSENSE_LOC_MAX] = "unknown",
> > >   };
> > >
> > >   return sprintf(buf, "%s\n",
> > >                  cros_ec_accel_legacy_loc_strings[st->sensor_num +
> > >                                                 MOTIONSENSE_LOC_BASE]);
> > In the legacy case, the location is hard-coded from sensor_num, the
> > index used by the EC: sensor 0 is in base, 1 is in the lid.
> > This limitation is removed from newer EC implementation, the EC
> > subcommand MOTIONSENSE_CMD_INFO provide that information.
I will send a patch to remove that remove that code and use _INFO_
even in legacy mode, it is one of a few command that haven't changed
since inception.
> > >
> > > > I am not enumerating identical devices twice: the embedded controller
> > > > manages a list of sensors:
> > > >
> > > > For instance on pixelbook, it look like:
> > > >        +--------+
> > > >         | EC    |
> > > >        +--------+
> > > >    ( via several i2c/spi buses)
> > > > +--------------------+--------------+-------- ...
> > > > |                         |                  |
> > > > IMU (base)     light/prox    Accelrometer (lid)
> > > > |
> > > > Magnetometer
> > > >
> > > > A given hardware sensor may be composed of multiple logical sensors
> > > > (IMU is a accelerometer and a gyroscope into one package).
> > > >
> > > > The EC firmware list all the (logical) sensors in array, and that
> > > > unique index - sensor_num - points to a single logical sensor.
> > >
> > > What what is 'sensor_num'; is it a channel address/number similar to
> > > what I2C HIDs use to communicate over a specific I2C line, or is it a
> > > type, similar to what HID devices provide on request for
> > > identification purposes?
> > >
> > > > Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
> > > > assigning .id myself?
> > >
> > > Is this a separate question, or can 'sensor_num' be any unique
> > > arbitrary number?
> > No, it is assigned by the EC.
>
> Is it assigned *by* the EC or *to* the EC.
The sensor_num filed is set the EC.
>
> If it is assigned *by* the EC, can't you ask the EC for it?
If the IIO driver knows the cros_sensor_type type, it could retrieve
the sensor_num[s] for all the sensors of that type.
However, the IIO driver itself, does not know which one to use,
another piece of code (cros_ec_dev) must keep track of which sensors
are already take care of.
>
> I asked this before, but was not given the answer I wanted:
>
> > > Why can't these numbers be assigned at runtime?
> > I assume you want to know why IIO drivers need to know "sensor_num"
> > ahead of time. It is because each IIO driver is independent from the
> > other.
>
> Is there a way to call into the EC and request the number?
No, there is not.
>
> int cros_ec_allocate_sensor_id(enum cros_sensor_type);
>
> Which allocates the next available ID of the requested type.
That code would not be in the EC, but in cros_ec_dev, based on the
result from _DUMP_ and _INFO_ command, if supported by the EC, as you
suggest below.
>
> OR
>
> If this is not possible AND the sensors of the same type are identical
> AND the sensors are always numbered sequentially from the same base
> (i.e. 0, 1, 2 OR 1, 2, 3, etc) then you use the ida_ API
> (include/linux/idr.h) to provide you with a unique ID for your
> sensor(s).
That would work if we add the requirement on the EC that sensors of
the same type are next to each other in the EC sensor array:
cros_ec_dev would build an array of struct idr, one per type.
IIO driver will recompute the type for a given device based on the
name in the mfd_cell (for instance cros-ec-sensor.c driver handles
accel, gyro and compass), and call back cros_ec_dev for an unused
index.
The EC requirement above is true today, but I see case ti will not
always be true, when some sensors are removed from a BOM while using
the same EC firmware: we will put the optional sensors at the end of
the EC sensors array.
>
> ###
>
> The flip side is a situation where the devices of a same type are not
> identical and are provided different platform data (by the parent MFD
> in this case).  If this true then you may actually need to identify
> the specific sensor ahead of time, in which case it's the chosen
> nomenclature that is misleading.  What you might really be looking for
> is a sensor_id and proper commenting/documentation.
Indeed, I would prefer to keep cros_ec sensor_hub code functionally
equivalent to the hid sensor_hub code with the difference we can not
infer the sensor type from sensor_num (contrary to hid
sensor_id/usageusage_id) and the map {sensor_type, sensor_num] varies
from chromebook to chromebook.
>
> ###
>
> However, looking at this patch, I suspect the former situation to be
> the case here, thus there is no requirement for the parent to
> pre-allocate 'sensor_num's.
>
Thank you,
Gwndal.
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog