From: Moritz Fischer <[email protected]>
The ChromeOS EC has mapped memory regions where things like temperature
sensors and fan speed are stored. Provide access to those from the
cros-ec mfd device.
Signed-off-by: Moritz Fischer <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 55 +++++++++++++++++++++++++++++++++
include/linux/mfd/cros_ec.h | 39 +++++++++++++++++++++++
2 files changed, 94 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ed5dee7..28063de 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -494,3 +494,58 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
return get_keyboard_state_event(ec_dev);
}
EXPORT_SYMBOL(cros_ec_get_next_event);
+
+static int __cros_ec_read_mapped_mem(struct cros_ec_device *ec, uint8_t offset,
+ void *buf, size_t size)
+{
+ int ret;
+ struct ec_params_read_memmap *params;
+ struct cros_ec_command *msg;
+
+ msg = kzalloc(sizeof(*msg) + max(sizeof(*params), size), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->version = 0;
+ msg->command = EC_CMD_READ_MEMMAP;
+ msg->insize = size;
+ msg->outsize = sizeof(*params);
+
+ params = (struct ec_params_read_memmap *)msg->data;
+ params->offset = offset;
+ params->size = size;
+
+ ret = cros_ec_cmd_xfer(ec, msg);
+ if (ret < 0 || msg->result != EC_RES_SUCCESS) {
+ dev_warn(ec->dev, "cannot read mapped reg: %d/%d\n",
+ ret, msg->result);
+ goto out_free;
+ }
+
+ memcpy(buf, msg->data, size);
+
+out_free:
+ kfree(msg);
+ return ret;
+}
+
+int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
+ uint32_t *data)
+{
+ return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
+}
+EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem32);
+
+int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
+ uint16_t *data)
+{
+ return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
+}
+EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem16);
+
+int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
+ uint8_t *data)
+{
+ return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
+}
+EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem8);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index b3d04de..c2de878 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -190,6 +190,45 @@ struct cros_ec_dev {
};
/**
+ * cros_ec_read_mapped_mem8 - Read mapped memory in the ChromeOS EC
+ *
+ * This can be called by drivers to access the mapped memory in the EC
+ *
+ * @ec_dev: Device to read from
+ * @offset: Offset to read
+ * @data: Return data
+ * @return: 0 if Ok, -ve on error
+ */
+int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
+ uint8_t *data);
+
+/**
+ * cros_ec_read_mapped_mem16 - Read mapped memory in the ChromeOS EC
+ *
+ * This can be called by drivers to access the mapped memory in the EC
+ *
+ * @ec_dev: Device to read from
+ * @offset: Offset to read
+ * @data: Return data
+ * @return: 0 if Ok, -ve on error
+ */
+int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
+ uint16_t *data);
+
+/**
+ * cros_ec_read_mapped_mem32 - Read mapped memory in the ChromeOS EC
+ *
+ * This can be called by drivers to access the mapped memory in the EC
+ *
+ * @ec_dev: Device to read from
+ * @offset: Offset to read
+ * @data: Return data
+ * @return: 0 if Ok, -ve on error
+ */
+int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
+ uint32_t *data);
+
+/**
* cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
*
* This can be called by drivers to handle a suspend event.
--
2.7.4
From: Moritz Fischer <[email protected]>
This adds a hwmon driver for the Chromium EC's fans
and temperature sensors.
Signed-off-by: Moritz Fischer <[email protected]>
---
This one still needs some work, but I figured some early feedback might not hurt.
Specifically I was wondering if using the devm_hwmon_register_with_info() is
preferable to the devm_hwmon_register_with_groups().
The EC has a bunch of additional features such as setting thermal limits etc,
which I'd still like to add but I figured I'll get some feedback on what I got so far.
Thanks,
Moritz
---
drivers/hwmon/Kconfig | 8 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/cros-ec-hwmon.c | 244 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 253 insertions(+)
create mode 100644 drivers/hwmon/cros-ec-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3..3b9155f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1254,6 +1254,14 @@ config SENSORS_PCF8591
These devices are hard to detect and rarely found on mainstream
hardware. If unsure, say N.
+config SENSORS_CROS_EC
+ tristate "ChromeOS EC hwmon"
+ depends on MFD_CROS_EC
+ help
+ If you say yes here you get hwmon support that will expose the
+ ChromeOS internal sensors for fanspeed and temperature to the
+ Linux hwmon subsystem.
+
source drivers/hwmon/pmbus/Kconfig
config SENSORS_PWM_FAN
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 5509edf..e59b5da 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
+obj-$(CONFIG_SENSORS_CROS_EC) += cros-ec-hwmon.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
diff --git a/drivers/hwmon/cros-ec-hwmon.c b/drivers/hwmon/cros-ec-hwmon.c
new file mode 100644
index 0000000..29d8b06
--- /dev/null
+++ b/drivers/hwmon/cros-ec-hwmon.c
@@ -0,0 +1,244 @@
+/*
+ * Copyright (c) 2017, National Instruments Corp.
+ *
+ * Chromium EC Fan speed and temperature sensor driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/of_platform.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/bitops.h>
+#include <linux/mfd/cros_ec.h>
+
+struct cros_ec_hwmon_priv {
+ struct cros_ec_device *ec;
+ struct device *hwmon_dev;
+
+ struct attribute **attrs;
+
+ struct attribute_group attr_group;
+ const struct attribute_group *groups[2];
+};
+
+#define KELVIN_TO_MILLICELSIUS(x) (((x) - 273) * 1000)
+
+static int __cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
+{
+ int err, idx;
+ uint16_t data;
+
+ for (idx = 0; idx < EC_FAN_SPEED_ENTRIES; idx++) {
+ err = cros_ec_read_mapped_mem16(priv->ec,
+ EC_MEMMAP_FAN + 2 * idx,
+ &data);
+ if (err)
+ return err;
+
+ if (data == EC_FAN_SPEED_NOT_PRESENT)
+ break;
+ }
+
+ return idx;
+}
+
+static int __cros_ec_hwmon_probe_temps(struct cros_ec_hwmon_priv *priv)
+{
+ uint8_t data;
+ int err, idx;
+
+ err = cros_ec_read_mapped_mem8(priv->ec, EC_MEMMAP_THERMAL_VERSION,
+ &data);
+
+ /* if we have a read error, or EC_MEMMAP_THERMAL_VERSION is not set,
+ * most likely we don't have temperature sensors ...
+ */
+ if (err || !data)
+ return 0;
+
+ for (idx = 0; idx < EC_TEMP_SENSOR_ENTRIES; idx++) {
+ err = cros_ec_read_mapped_mem8(priv->ec,
+ EC_MEMMAP_TEMP_SENSOR + idx,
+ &data);
+ if (err)
+ return idx;
+
+ /* this assumes that they're all good up to idx */
+ switch (data) {
+ case EC_TEMP_SENSOR_NOT_PRESENT:
+ case EC_TEMP_SENSOR_ERROR:
+ case EC_TEMP_SENSOR_NOT_POWERED:
+ case EC_TEMP_SENSOR_NOT_CALIBRATED:
+ return idx;
+ default:
+ continue;
+ };
+ }
+
+ return idx;
+}
+
+static ssize_t cros_ec_hwmon_read_fan_rpm(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ uint16_t data;
+ int err;
+ struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+ struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+ err = cros_ec_read_mapped_mem16(priv->ec,
+ EC_MEMMAP_FAN + 2 * sattr->index,
+ &data);
+ if (err)
+ return err;
+
+ return sprintf(buf, "%d\n", data);
+}
+
+static ssize_t cros_ec_hwmon_read_temp(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ uint8_t data;
+ int err, tmp;
+
+ struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+ struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+ err = cros_ec_read_mapped_mem8(priv->ec,
+ EC_MEMMAP_TEMP_SENSOR + 1 * sattr->index,
+ &data);
+ if (err)
+ return err;
+
+ switch (data) {
+ case EC_TEMP_SENSOR_NOT_PRESENT:
+ case EC_TEMP_SENSOR_ERROR:
+ case EC_TEMP_SENSOR_NOT_POWERED:
+ case EC_TEMP_SENSOR_NOT_CALIBRATED:
+ dev_info(priv->ec->dev, "Failure: result=%d\n", data);
+ return -EIO;
+ }
+
+ /* make sure we don't overflow when adding offset*/
+ tmp = data + EC_TEMP_SENSOR_OFFSET;
+
+ return sprintf(buf, "%d\n", KELVIN_TO_MILLICELSIUS(tmp));
+}
+
+static int cros_ec_hwmon_probe(struct platform_device *pdev)
+{
+ struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+ struct cros_ec_hwmon_priv *ec_hwmon;
+ struct sensor_device_attribute *attr;
+ int num_fans, num_temps, i;
+
+ ec_hwmon = devm_kzalloc(&pdev->dev, sizeof(*ec_hwmon), GFP_KERNEL);
+ if (!ec_hwmon)
+ return -ENOMEM;
+ ec_hwmon->ec = ec;
+
+ num_fans = __cros_ec_hwmon_probe_fans(ec_hwmon);
+ if (num_fans < 0)
+ return num_fans;
+
+ num_temps = __cros_ec_hwmon_probe_temps(ec_hwmon);
+ if (num_fans < 0)
+ return num_temps;
+
+ ec_hwmon->attrs = devm_kzalloc(&pdev->dev,
+ sizeof(*ec_hwmon->attrs) *
+ (num_fans + num_temps + 1),
+ GFP_KERNEL);
+ if (!ec_hwmon->attrs)
+ return -ENOMEM;
+
+ for (i = 0; i < num_fans; i++) {
+ attr = devm_kzalloc(&pdev->dev, sizeof(*attr), GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+ sysfs_attr_init(&attr->dev_attr.attr);
+ attr->dev_attr.attr.name = devm_kasprintf(&pdev->dev,
+ GFP_KERNEL,
+ "fan%d_input",
+ i);
+ if (!attr->dev_attr.attr.name)
+ return -ENOMEM;
+
+ attr->dev_attr.show = cros_ec_hwmon_read_fan_rpm;
+ attr->dev_attr.attr.mode = S_IRUGO;
+ attr->index = i;
+ ec_hwmon->attrs[i] = &attr->dev_attr.attr;
+
+ }
+
+ for (i = 0; i < num_temps; i++) {
+ attr = devm_kzalloc(&pdev->dev, sizeof(*attr), GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+ sysfs_attr_init(&attr->dev_attr.attr);
+ attr->dev_attr.attr.name = devm_kasprintf(&pdev->dev,
+ GFP_KERNEL,
+ "temp%d_input",
+ i);
+ if (!attr->dev_attr.attr.name)
+ return -ENOMEM;
+
+ attr->dev_attr.show = cros_ec_hwmon_read_temp;
+ attr->dev_attr.attr.mode = S_IRUGO;
+ attr->index = i;
+ ec_hwmon->attrs[i + num_fans] = &attr->dev_attr.attr;
+
+ }
+
+ ec_hwmon->attr_group.attrs = ec_hwmon->attrs;
+ ec_hwmon->groups[0] = &ec_hwmon->attr_group;
+
+ ec_hwmon->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+ "ec_hwmon", ec_hwmon, ec_hwmon->groups);
+
+ if (IS_ERR(ec_hwmon->hwmon_dev))
+ return PTR_ERR(ec_hwmon->hwmon_dev);
+
+ platform_set_drvdata(pdev, ec_hwmon);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_hwmon_of_match[] = {
+ { .compatible = "google,cros-ec-hwmon" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_hwmon_of_match);
+#endif
+
+static struct platform_driver cros_ec_hwmon_driver = {
+ .probe = cros_ec_hwmon_probe,
+ .driver = {
+ .name = "cros-ec-hwmon",
+ .of_match_table = of_match_ptr(cros_ec_hwmon_of_match),
+ },
+};
+module_platform_driver(cros_ec_hwmon_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS EC Hardware Monitor driver");
+MODULE_ALIAS("platform:cros-ec-hwmon");
+MODULE_AUTHOR("Moritz Fischer <[email protected]>");
--
2.7.4
From: Moritz Fischer <[email protected]>
Add bindings for the Chromium EC HWMON. The Chromium EC HWMON
allows monitoring of temperature sensors and fans attached to the
EC.
Signed-off-by: Moritz Fischer <[email protected]>
---
.../devicetree/bindings/hwmon/cros-ec-hwmon.txt | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
diff --git a/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
new file mode 100644
index 0000000..4c94869
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
@@ -0,0 +1,25 @@
+Chromium Embedded Controller EC temperature and fan control
+-----------------------------------------------------------
+
+Google's Chromium EC HWMON is a hwmon implemented byimplemented by the Chromium EC
+firmware attached to the Embedded Controller (EC) and controlled via a host-command
+interface.
+
+An EC HWMON node should be only found as a sub-node of the EC node (see
+Documentation/devicetree/bindings/mfd/cros-ec.txt).
+
+Required properties:
+- compatible: Must contain "google,cros-ec-hwmon"
+
+Example:
+ embedded-controller@1e {
+ reg = <0x1e>;
+ compatible = "google,cros-ec-i2c";
+ interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-parent = <&gpio0>;
+
+ hwmon {
+ compatible = "google,cros-ec-hwmon";
+ };
+};
+
--
2.7.4
On 04/07/2017 03:00 PM, Moritz Fischer wrote:
> From: Moritz Fischer <[email protected]>
>
> The ChromeOS EC has mapped memory regions where things like temperature
> sensors and fan speed are stored. Provide access to those from the
> cros-ec mfd device.
>
> Signed-off-by: Moritz Fischer <[email protected]>
I'll have to consult with others at Google if this is a good idea.
Benson, can you comment ?
> ---
> drivers/platform/chrome/cros_ec_proto.c | 55 +++++++++++++++++++++++++++++++++
> include/linux/mfd/cros_ec.h | 39 +++++++++++++++++++++++
> 2 files changed, 94 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index ed5dee7..28063de 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -494,3 +494,58 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
> return get_keyboard_state_event(ec_dev);
> }
> EXPORT_SYMBOL(cros_ec_get_next_event);
> +
> +static int __cros_ec_read_mapped_mem(struct cros_ec_device *ec, uint8_t offset,
> + void *buf, size_t size)
> +{
> + int ret;
> + struct ec_params_read_memmap *params;
> + struct cros_ec_command *msg;
> +
> + msg = kzalloc(sizeof(*msg) + max(sizeof(*params), size), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
I don't think using kzalloc here makes much sense. It is well known
that size is <= 4, so using a local buffer should not be a problem.
> + msg->version = 0;
> + msg->command = EC_CMD_READ_MEMMAP;
> + msg->insize = size;
> + msg->outsize = sizeof(*params);
> +
> + params = (struct ec_params_read_memmap *)msg->data;
> + params->offset = offset;
> + params->size = size;
> +
> + ret = cros_ec_cmd_xfer(ec, msg);
> + if (ret < 0 || msg->result != EC_RES_SUCCESS) {
cros_ec_cmd_xfer_status() was introduced to be able to avoid the second check.
> + dev_warn(ec->dev, "cannot read mapped reg: %d/%d\n",
> + ret, msg->result);
> + goto out_free;
> + }
> +
> + memcpy(buf, msg->data, size);
> +
> +out_free:
> + kfree(msg);
> + return ret;
> +}
> +
> +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> + uint32_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem32);
> +
> +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> + uint16_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem16);
> +
Either case, this assumes that EC endianness matches host endianness. I don't
think we can just assume that this is the case.
> +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> + uint8_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem8);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index b3d04de..c2de878 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -190,6 +190,45 @@ struct cros_ec_dev {
> };
>
> /**
> + * cros_ec_read_mapped_mem8 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> + uint8_t *data);
> +
> +/**
> + * cros_ec_read_mapped_mem16 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> + uint16_t *data);
> +
> +/**
> + * cros_ec_read_mapped_mem32 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> + uint32_t *data);
> +
> +/**
> * cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
> *
> * This can be called by drivers to handle a suspend event.
>
On Sun, Apr 09, 2017 at 04:02:04PM -0700, Guenter Roeck wrote:
> On 04/07/2017 03:00 PM, Moritz Fischer wrote:
> > From: Moritz Fischer <[email protected]>
> >
> > The ChromeOS EC has mapped memory regions where things like temperature
> > sensors and fan speed are stored. Provide access to those from the
> > cros-ec mfd device.
> >
> > Signed-off-by: Moritz Fischer <[email protected]>
>
> I'll have to consult with others at Google if this is a good idea.
> Benson, can you comment ?
Well to my knowledge the only other way to get to it is the 'ectool'
from userland
via ioctl calls. The other option would be IIO ...
>
> > ---
> > drivers/platform/chrome/cros_ec_proto.c | 55 +++++++++++++++++++++++++++++++++
> > include/linux/mfd/cros_ec.h | 39 +++++++++++++++++++++++
> > 2 files changed, 94 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index ed5dee7..28063de 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -494,3 +494,58 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
> > return get_keyboard_state_event(ec_dev);
> > }
> > EXPORT_SYMBOL(cros_ec_get_next_event);
> > +
> > +static int __cros_ec_read_mapped_mem(struct cros_ec_device *ec, uint8_t offset,
> > + void *buf, size_t size)
> > +{
> > + int ret;
> > + struct ec_params_read_memmap *params;
> > + struct cros_ec_command *msg;
> > +
> > + msg = kzalloc(sizeof(*msg) + max(sizeof(*params), size), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
>
> I don't think using kzalloc here makes much sense. It is well known
> that size is <= 4, so using a local buffer should not be a problem.
Good point, that was basically copy & paste from other cros-ec code ;-)
I'll fix this.
>
> > + msg->version = 0;
> > + msg->command = EC_CMD_READ_MEMMAP;
> > + msg->insize = size;
> > + msg->outsize = sizeof(*params);
> > +
> > + params = (struct ec_params_read_memmap *)msg->data;
> > + params->offset = offset;
> > + params->size = size;
> > +
> > + ret = cros_ec_cmd_xfer(ec, msg);
> > + if (ret < 0 || msg->result != EC_RES_SUCCESS) {
>
> cros_ec_cmd_xfer_status() was introduced to be able to avoid the second check.
>
Alright, cool. Will fix this.
> > + dev_warn(ec->dev, "cannot read mapped reg: %d/%d\n",
> > + ret, msg->result);
> > + goto out_free;
> > + }
> > +
> > + memcpy(buf, msg->data, size);
> > +
> > +out_free:
> > + kfree(msg);
> > + return ret;
> > +}
> > +
> > +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> > + uint32_t *data)
> > +{
> > + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem32);
> > +
> > +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> > + uint16_t *data)
> > +{
> > + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem16);
> > +
>
> Either case, this assumes that EC endianness matches host endianness. I don't
> think we can just assume that this is the case.
Huh, yeah. Will need to figure out how to detect the EC endianness in
that case.
>
> > +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> > + uint8_t *data)
> > +{
> > + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem8);
> > diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> > index b3d04de..c2de878 100644
> > --- a/include/linux/mfd/cros_ec.h
> > +++ b/include/linux/mfd/cros_ec.h
> > @@ -190,6 +190,45 @@ struct cros_ec_dev {
> > };
> >
> > /**
> > + * cros_ec_read_mapped_mem8 - Read mapped memory in the ChromeOS EC
> > + *
> > + * This can be called by drivers to access the mapped memory in the EC
> > + *
> > + * @ec_dev: Device to read from
> > + * @offset: Offset to read
> > + * @data: Return data
> > + * @return: 0 if Ok, -ve on error
> > + */
> > +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> > + uint8_t *data);
> > +
> > +/**
> > + * cros_ec_read_mapped_mem16 - Read mapped memory in the ChromeOS EC
> > + *
> > + * This can be called by drivers to access the mapped memory in the EC
> > + *
> > + * @ec_dev: Device to read from
> > + * @offset: Offset to read
> > + * @data: Return data
> > + * @return: 0 if Ok, -ve on error
> > + */
> > +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> > + uint16_t *data);
> > +
> > +/**
> > + * cros_ec_read_mapped_mem32 - Read mapped memory in the ChromeOS EC
> > + *
> > + * This can be called by drivers to access the mapped memory in the EC
> > + *
> > + * @ec_dev: Device to read from
> > + * @offset: Offset to read
> > + * @data: Return data
> > + * @return: 0 if Ok, -ve on error
> > + */
> > +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> > + uint32_t *data);
> > +
> > +/**
> > * cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
> > *
> > * This can be called by drivers to handle a suspend event.
> >
>
Thanks for the feedback,
Moritz
On Fri, Apr 07, 2017 at 03:00:09PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <[email protected]>
>
> Add bindings for the Chromium EC HWMON. The Chromium EC HWMON
> allows monitoring of temperature sensors and fans attached to the
> EC.
>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> .../devicetree/bindings/hwmon/cros-ec-hwmon.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
> new file mode 100644
> index 0000000..4c94869
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
> @@ -0,0 +1,25 @@
> +Chromium Embedded Controller EC temperature and fan control
> +-----------------------------------------------------------
> +
> +Google's Chromium EC HWMON is a hwmon implemented byimplemented by the Chromium EC
> +firmware attached to the Embedded Controller (EC) and controlled via a host-command
> +interface.
> +
> +An EC HWMON node should be only found as a sub-node of the EC node (see
> +Documentation/devicetree/bindings/mfd/cros-ec.txt).
> +
> +Required properties:
> +- compatible: Must contain "google,cros-ec-hwmon"
> +
> +Example:
> + embedded-controller@1e {
> + reg = <0x1e>;
> + compatible = "google,cros-ec-i2c";
> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-parent = <&gpio0>;
> +
> + hwmon {
> + compatible = "google,cros-ec-hwmon";
This is sufficient for all devices? I don't see that DT provides
anything here other than instantiating a device, but the parent device
can just as easily do that.
Rob
On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <[email protected]>
>
> The ChromeOS EC has mapped memory regions where things like temperature
> sensors and fan speed are stored. Provide access to those from the
> cros-ec mfd device.
>
Turns out struct cros_ec_device already provides a cmd_readmem callback,
which is widely used by other drivers. Why don't you just use it ?
Thanks,
Guenter
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 55 +++++++++++++++++++++++++++++++++
> include/linux/mfd/cros_ec.h | 39 +++++++++++++++++++++++
> 2 files changed, 94 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index ed5dee7..28063de 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -494,3 +494,58 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
> return get_keyboard_state_event(ec_dev);
> }
> EXPORT_SYMBOL(cros_ec_get_next_event);
> +
> +static int __cros_ec_read_mapped_mem(struct cros_ec_device *ec, uint8_t offset,
> + void *buf, size_t size)
> +{
> + int ret;
> + struct ec_params_read_memmap *params;
> + struct cros_ec_command *msg;
> +
> + msg = kzalloc(sizeof(*msg) + max(sizeof(*params), size), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + msg->version = 0;
> + msg->command = EC_CMD_READ_MEMMAP;
> + msg->insize = size;
> + msg->outsize = sizeof(*params);
> +
> + params = (struct ec_params_read_memmap *)msg->data;
> + params->offset = offset;
> + params->size = size;
> +
> + ret = cros_ec_cmd_xfer(ec, msg);
> + if (ret < 0 || msg->result != EC_RES_SUCCESS) {
> + dev_warn(ec->dev, "cannot read mapped reg: %d/%d\n",
> + ret, msg->result);
> + goto out_free;
> + }
> +
> + memcpy(buf, msg->data, size);
> +
> +out_free:
> + kfree(msg);
> + return ret;
> +}
> +
> +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> + uint32_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem32);
> +
> +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> + uint16_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem16);
> +
> +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> + uint8_t *data)
> +{
> + return __cros_ec_read_mapped_mem(ec, offset, data, sizeof(*data));
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_read_mapped_mem8);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index b3d04de..c2de878 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -190,6 +190,45 @@ struct cros_ec_dev {
> };
>
> /**
> + * cros_ec_read_mapped_mem8 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem8(struct cros_ec_device *ec, const uint8_t offset,
> + uint8_t *data);
> +
> +/**
> + * cros_ec_read_mapped_mem16 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem16(struct cros_ec_device *ec, const uint8_t offset,
> + uint16_t *data);
> +
> +/**
> + * cros_ec_read_mapped_mem32 - Read mapped memory in the ChromeOS EC
> + *
> + * This can be called by drivers to access the mapped memory in the EC
> + *
> + * @ec_dev: Device to read from
> + * @offset: Offset to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_read_mapped_mem32(struct cros_ec_device *ec, const uint8_t offset,
> + uint32_t *data);
> +
> +/**
> * cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
> *
> * This can be called by drivers to handle a suspend event.
> --
> 2.7.4
>
On Thu, Apr 13, 2017 at 03:01:40PM -0500, Rob Herring wrote:
> On Fri, Apr 07, 2017 at 03:00:09PM -0700, Moritz Fischer wrote:
> > From: Moritz Fischer <[email protected]>
> >
> > Add bindings for the Chromium EC HWMON. The Chromium EC HWMON
> > allows monitoring of temperature sensors and fans attached to the
> > EC.
> >
> > Signed-off-by: Moritz Fischer <[email protected]>
> > ---
> > .../devicetree/bindings/hwmon/cros-ec-hwmon.txt | 25 ++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
> > new file mode 100644
> > index 0000000..4c94869
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
> > @@ -0,0 +1,25 @@
> > +Chromium Embedded Controller EC temperature and fan control
> > +-----------------------------------------------------------
> > +
> > +Google's Chromium EC HWMON is a hwmon implemented byimplemented by the Chromium EC
> > +firmware attached to the Embedded Controller (EC) and controlled via a host-command
> > +interface.
> > +
> > +An EC HWMON node should be only found as a sub-node of the EC node (see
> > +Documentation/devicetree/bindings/mfd/cros-ec.txt).
> > +
> > +Required properties:
> > +- compatible: Must contain "google,cros-ec-hwmon"
> > +
> > +Example:
> > + embedded-controller@1e {
> > + reg = <0x1e>;
> > + compatible = "google,cros-ec-i2c";
> > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-parent = <&gpio0>;
> > +
> > + hwmon {
> > + compatible = "google,cros-ec-hwmon";
>
> This is sufficient for all devices? I don't see that DT provides
> anything here other than instantiating a device, but the parent device
> can just as easily do that.
>
The parent driver (drivers/mfd/cros_ec_i2c.c) calls cros_ec_register(),
which uses uses of_platform_populate() to populate all sub-devices.
There are various examples in the dts files (look for "google,cros-ec").
Does it really make sense to start a second method for instantiating
sub-devices ?
Thanks,
Guenter
On Fri, Apr 07, 2017 at 03:00:10PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <[email protected]>
>
> This adds a hwmon driver for the Chromium EC's fans
> and temperature sensors.
>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
>
> This one still needs some work, but I figured some early feedback might not hurt.
> Specifically I was wondering if using the devm_hwmon_register_with_info() is
> preferable to the devm_hwmon_register_with_groups().
>
Please use devm_hwmon_register_with_info().
> The EC has a bunch of additional features such as setting thermal limits etc,
> which I'd still like to add but I figured I'll get some feedback on what I got so far.
>
Those would probably be more appropriate for a thermal driver.
> Thanks,
>
> Moritz
>
> ---
> drivers/hwmon/Kconfig | 8 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/cros-ec-hwmon.c | 244 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+)
> create mode 100644 drivers/hwmon/cros-ec-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0649d53f3..3b9155f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1254,6 +1254,14 @@ config SENSORS_PCF8591
> These devices are hard to detect and rarely found on mainstream
> hardware. If unsure, say N.
>
> +config SENSORS_CROS_EC
> + tristate "ChromeOS EC hwmon"
> + depends on MFD_CROS_EC
> + help
> + If you say yes here you get hwmon support that will expose the
> + ChromeOS internal sensors for fanspeed and temperature to the
> + Linux hwmon subsystem.
> +
> source drivers/hwmon/pmbus/Kconfig
>
> config SENSORS_PWM_FAN
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5509edf..e59b5da 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> +obj-$(CONFIG_SENSORS_CROS_EC) += cros-ec-hwmon.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> diff --git a/drivers/hwmon/cros-ec-hwmon.c b/drivers/hwmon/cros-ec-hwmon.c
> new file mode 100644
> index 0000000..29d8b06
> --- /dev/null
> +++ b/drivers/hwmon/cros-ec-hwmon.c
> @@ -0,0 +1,244 @@
> +/*
> + * Copyright (c) 2017, National Instruments Corp.
> + *
> + * Chromium EC Fan speed and temperature sensor driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_platform.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/bitops.h>
> +#include <linux/mfd/cros_ec.h>
> +
> +struct cros_ec_hwmon_priv {
> + struct cros_ec_device *ec;
> + struct device *hwmon_dev;
> +
> + struct attribute **attrs;
> +
> + struct attribute_group attr_group;
> + const struct attribute_group *groups[2];
> +};
> +
> +#define KELVIN_TO_MILLICELSIUS(x) (((x) - 273) * 1000)
> +
> +static int __cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
> +{
> + int err, idx;
> + uint16_t data;
> +
> + for (idx = 0; idx < EC_FAN_SPEED_ENTRIES; idx++) {
> + err = cros_ec_read_mapped_mem16(priv->ec,
> + EC_MEMMAP_FAN + 2 * idx,
> + &data);
Kind of interesting. ectool and ec code all assume that there is an
endianness match. Guess we can do the same. Just add a respective comment.
> + if (err)
> + return err;
> +
> + if (data == EC_FAN_SPEED_NOT_PRESENT)
> + break;
> + }
> +
> + return idx;
> +}
> +
> +static int __cros_ec_hwmon_probe_temps(struct cros_ec_hwmon_priv *priv)
> +{
> + uint8_t data;
> + int err, idx;
> +
> + err = cros_ec_read_mapped_mem8(priv->ec, EC_MEMMAP_THERMAL_VERSION,
> + &data);
> +
> + /* if we have a read error, or EC_MEMMAP_THERMAL_VERSION is not set,
> + * most likely we don't have temperature sensors ...
> + */
> + if (err || !data)
> + return 0;
> +
> + for (idx = 0; idx < EC_TEMP_SENSOR_ENTRIES; idx++) {
This doesn't cover EC_MEMMAP_THERMAL_VERSION >= 2 which supports
an additional set of thermal sensors.
> + err = cros_ec_read_mapped_mem8(priv->ec,
> + EC_MEMMAP_TEMP_SENSOR + idx,
> + &data);
You can read all data in one go using the provided callback function.
> + if (err)
> + return idx;
> +
> + /* this assumes that they're all good up to idx */
> + switch (data) {
> + case EC_TEMP_SENSOR_NOT_PRESENT:
> + case EC_TEMP_SENSOR_ERROR:
> + case EC_TEMP_SENSOR_NOT_POWERED:
> + case EC_TEMP_SENSOR_NOT_CALIBRATED:
Not sure if we can assume sensors to be in sequential order.
ectool doesn't make that assumption (it only does it for fans).
It might be safer to generate attributes for all possible sensors
and then use the is_visible function to determine which ones are
enabled.
> + return idx;
> + default:
> + continue;
> + };
> + }
> +
> + return idx;
> +}
> +
> +static ssize_t cros_ec_hwmon_read_fan_rpm(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + uint16_t data;
> + int err;
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> + err = cros_ec_read_mapped_mem16(priv->ec,
> + EC_MEMMAP_FAN + 2 * sattr->index,
> + &data);
> + if (err)
> + return err;
> +
Watch out for EC_FAN_SPEED_STALLED.
> + return sprintf(buf, "%d\n", data);
> +}
> +
> +static ssize_t cros_ec_hwmon_read_temp(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + uint8_t data;
> + int err, tmp;
> +
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> + err = cros_ec_read_mapped_mem8(priv->ec,
> + EC_MEMMAP_TEMP_SENSOR + 1 * sattr->index,
> + &data);
> + if (err)
> + return err;
> +
> + switch (data) {
> + case EC_TEMP_SENSOR_NOT_PRESENT:
> + case EC_TEMP_SENSOR_ERROR:
> + case EC_TEMP_SENSOR_NOT_POWERED:
> + case EC_TEMP_SENSOR_NOT_CALIBRATED:
> + dev_info(priv->ec->dev, "Failure: result=%d\n", data);
Please no logging noise.
> + return -EIO;
> + }
> +
> + /* make sure we don't overflow when adding offset*/
> + tmp = data + EC_TEMP_SENSOR_OFFSET;
> +
> + return sprintf(buf, "%d\n", KELVIN_TO_MILLICELSIUS(tmp));
Overall, a REG_TO_MILLICELSIUS() macro might be easier.
> +}
> +
> +static int cros_ec_hwmon_probe(struct platform_device *pdev)
> +{
> + struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> + struct cros_ec_hwmon_priv *ec_hwmon;
> + struct sensor_device_attribute *attr;
> + int num_fans, num_temps, i;
> +
> + ec_hwmon = devm_kzalloc(&pdev->dev, sizeof(*ec_hwmon), GFP_KERNEL);
> + if (!ec_hwmon)
> + return -ENOMEM;
> + ec_hwmon->ec = ec;
> +
> + num_fans = __cros_ec_hwmon_probe_fans(ec_hwmon);
> + if (num_fans < 0)
> + return num_fans;
> +
> + num_temps = __cros_ec_hwmon_probe_temps(ec_hwmon);
> + if (num_fans < 0)
> + return num_temps;
> +
> + ec_hwmon->attrs = devm_kzalloc(&pdev->dev,
> + sizeof(*ec_hwmon->attrs) *
> + (num_fans + num_temps + 1),
> + GFP_KERNEL);
> + if (!ec_hwmon->attrs)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_fans; i++) {
> + attr = devm_kzalloc(&pdev->dev, sizeof(*attr), GFP_KERNEL);
> + if (!attr)
> + return -ENOMEM;
> + sysfs_attr_init(&attr->dev_attr.attr);
> + attr->dev_attr.attr.name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL,
> + "fan%d_input",
> + i);
> + if (!attr->dev_attr.attr.name)
> + return -ENOMEM;
> +
> + attr->dev_attr.show = cros_ec_hwmon_read_fan_rpm;
> + attr->dev_attr.attr.mode = S_IRUGO;
> + attr->index = i;
> + ec_hwmon->attrs[i] = &attr->dev_attr.attr;
> +
> + }
> +
> + for (i = 0; i < num_temps; i++) {
> + attr = devm_kzalloc(&pdev->dev, sizeof(*attr), GFP_KERNEL);
> + if (!attr)
> + return -ENOMEM;
> + sysfs_attr_init(&attr->dev_attr.attr);
> + attr->dev_attr.attr.name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL,
> + "temp%d_input",
> + i);
> + if (!attr->dev_attr.attr.name)
> + return -ENOMEM;
> +
> + attr->dev_attr.show = cros_ec_hwmon_read_temp;
> + attr->dev_attr.attr.mode = S_IRUGO;
> + attr->index = i;
> + ec_hwmon->attrs[i + num_fans] = &attr->dev_attr.attr;
> +
> + }
> +
> + ec_hwmon->attr_group.attrs = ec_hwmon->attrs;
> + ec_hwmon->groups[0] = &ec_hwmon->attr_group;
> +
> + ec_hwmon->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> + "ec_hwmon", ec_hwmon, ec_hwmon->groups);
> +
hwmon_dev is only used in this function. There is no need to store it in
ec_hwmon.
> + if (IS_ERR(ec_hwmon->hwmon_dev))
> + return PTR_ERR(ec_hwmon->hwmon_dev);
> +
return PTR_ERR_OR_ZERO(hwmon_dev);
> + platform_set_drvdata(pdev, ec_hwmon);
> +
I don't see where this is used.
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_ec_hwmon_of_match[] = {
> + { .compatible = "google,cros-ec-hwmon" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_hwmon_of_match);
> +#endif
> +
> +static struct platform_driver cros_ec_hwmon_driver = {
> + .probe = cros_ec_hwmon_probe,
> + .driver = {
> + .name = "cros-ec-hwmon",
> + .of_match_table = of_match_ptr(cros_ec_hwmon_of_match),
> + },
> +};
> +module_platform_driver(cros_ec_hwmon_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitor driver");
> +MODULE_ALIAS("platform:cros-ec-hwmon");
> +MODULE_AUTHOR("Moritz Fischer <[email protected]>");
> --
> 2.7.4
>
Hi Guenter,
On Thu, Apr 13, 2017 at 2:03 PM, Guenter Roeck <[email protected]> wrote:
> On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
>> From: Moritz Fischer <[email protected]>
>>
>> The ChromeOS EC has mapped memory regions where things like temperature
>> sensors and fan speed are stored. Provide access to those from the
>> cros-ec mfd device.
>>
>
> Turns out struct cros_ec_device already provides a cmd_readmem callback,
> which is widely used by other drivers. Why don't you just use it ?
This is only actually set by the lpc version of the cros_ec. I2C and
SPI connected ECs
emulate it. I can most certainly hook it up in the (spi,i2c) drivers,
but the implementation
for SPI and I2C needs to live somewhere. drivers/platform/chrome/cros_ec_proto.c
seemed to be a good place.
Thanks for the feedback!
Moritz
On 04/13/2017 03:53 PM, Moritz Fischer wrote:
> Hi Guenter,
>
> On Thu, Apr 13, 2017 at 2:03 PM, Guenter Roeck <[email protected]> wrote:
>> On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
>>> From: Moritz Fischer <[email protected]>
>>>
>>> The ChromeOS EC has mapped memory regions where things like temperature
>>> sensors and fan speed are stored. Provide access to those from the
>>> cros-ec mfd device.
>>>
>>
>> Turns out struct cros_ec_device already provides a cmd_readmem callback,
>> which is widely used by other drivers. Why don't you just use it ?
>
> This is only actually set by the lpc version of the cros_ec. I2C and
> SPI connected ECs
Hmm - weird. I thought I saw it implemented for those, but I must have been
struck by lightning or something. Let me check with Gwendal to see how
this (ie its use from iio) is supposed to work on non-LPC systems.
Guenter
> emulate it. I can most certainly hook it up in the (spi,i2c) drivers,
> but the implementation
> for SPI and I2C needs to live somewhere. drivers/platform/chrome/cros_ec_proto.c
> seemed to be a good place.
>
> Thanks for the feedback!
>
> Moritz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, Apr 13, 2017 at 4:07 PM, Guenter Roeck <[email protected]> wrote:
> On Thu, Apr 13, 2017 at 03:01:40PM -0500, Rob Herring wrote:
>> On Fri, Apr 07, 2017 at 03:00:09PM -0700, Moritz Fischer wrote:
>> > From: Moritz Fischer <[email protected]>
>> >
>> > Add bindings for the Chromium EC HWMON. The Chromium EC HWMON
>> > allows monitoring of temperature sensors and fans attached to the
>> > EC.
>> >
>> > Signed-off-by: Moritz Fischer <[email protected]>
>> > ---
>> > .../devicetree/bindings/hwmon/cros-ec-hwmon.txt | 25 ++++++++++++++++++++++
>> > 1 file changed, 25 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>> > new file mode 100644
>> > index 0000000..4c94869
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>> > @@ -0,0 +1,25 @@
>> > +Chromium Embedded Controller EC temperature and fan control
>> > +-----------------------------------------------------------
>> > +
>> > +Google's Chromium EC HWMON is a hwmon implemented byimplemented by the Chromium EC
>> > +firmware attached to the Embedded Controller (EC) and controlled via a host-command
>> > +interface.
>> > +
>> > +An EC HWMON node should be only found as a sub-node of the EC node (see
>> > +Documentation/devicetree/bindings/mfd/cros-ec.txt).
>> > +
>> > +Required properties:
>> > +- compatible: Must contain "google,cros-ec-hwmon"
>> > +
>> > +Example:
>> > + embedded-controller@1e {
>> > + reg = <0x1e>;
>> > + compatible = "google,cros-ec-i2c";
>> > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> > + interrupt-parent = <&gpio0>;
>> > +
>> > + hwmon {
>> > + compatible = "google,cros-ec-hwmon";
>>
>> This is sufficient for all devices? I don't see that DT provides
>> anything here other than instantiating a device, but the parent device
>> can just as easily do that.
>>
> The parent driver (drivers/mfd/cros_ec_i2c.c) calls cros_ec_register(),
> which uses uses of_platform_populate() to populate all sub-devices.
> There are various examples in the dts files (look for "google,cros-ec").
> Does it really make sense to start a second method for instantiating
> sub-devices ?
Okay, I suppose not. That wasn't clear from the example.
Rob
On Fri, Apr 14, 2017 at 5:48 AM, Rob Herring <[email protected]> wrote:
> On Thu, Apr 13, 2017 at 4:07 PM, Guenter Roeck <[email protected]> wrote:
>> On Thu, Apr 13, 2017 at 03:01:40PM -0500, Rob Herring wrote:
>>> On Fri, Apr 07, 2017 at 03:00:09PM -0700, Moritz Fischer wrote:
>>> > From: Moritz Fischer <[email protected]>
>>> >
>>> > Add bindings for the Chromium EC HWMON. The Chromium EC HWMON
>>> > allows monitoring of temperature sensors and fans attached to the
>>> > EC.
>>> >
>>> > Signed-off-by: Moritz Fischer <[email protected]>
>>> > ---
>>> > .../devicetree/bindings/hwmon/cros-ec-hwmon.txt | 25 ++++++++++++++++++++++
>>> > 1 file changed, 25 insertions(+)
>>> > create mode 100644 Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>>> >
>>> > diff --git a/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>>> > new file mode 100644
>>> > index 0000000..4c94869
>>> > --- /dev/null
>>> > +++ b/Documentation/devicetree/bindings/hwmon/cros-ec-hwmon.txt
>>> > @@ -0,0 +1,25 @@
>>> > +Chromium Embedded Controller EC temperature and fan control
>>> > +-----------------------------------------------------------
>>> > +
>>> > +Google's Chromium EC HWMON is a hwmon implemented byimplemented by the Chromium EC
>>> > +firmware attached to the Embedded Controller (EC) and controlled via a host-command
>>> > +interface.
>>> > +
>>> > +An EC HWMON node should be only found as a sub-node of the EC node (see
>>> > +Documentation/devicetree/bindings/mfd/cros-ec.txt).
>>> > +
>>> > +Required properties:
>>> > +- compatible: Must contain "google,cros-ec-hwmon"
>>> > +
>>> > +Example:
>>> > + embedded-controller@1e {
>>> > + reg = <0x1e>;
>>> > + compatible = "google,cros-ec-i2c";
>>> > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>>> > + interrupt-parent = <&gpio0>;
>>> > +
>>> > + hwmon {
>>> > + compatible = "google,cros-ec-hwmon";
>>>
>>> This is sufficient for all devices? I don't see that DT provides
>>> anything here other than instantiating a device, but the parent device
>>> can just as easily do that.
>>>
>> The parent driver (drivers/mfd/cros_ec_i2c.c) calls cros_ec_register(),
>> which uses uses of_platform_populate() to populate all sub-devices.
>> There are various examples in the dts files (look for "google,cros-ec").
>> Does it really make sense to start a second method for instantiating
>> sub-devices ?
>
> Okay, I suppose not. That wasn't clear from the example.
Do you want me to clarify that in the example somehow?
Moritz