linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
@ 2017-04-07 22:00 Moritz Fischer
  2017-04-07 22:00 ` [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON Moritz Fischer
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Moritz Fischer @ 2017-04-07 22:00 UTC (permalink / raw)
  To: linux-hwmon
  Cc: moritz.fischer, linux-kernel, devicetree, lee.jones, olof, linux,
	jdelvare, robh+dt, mark.rutland, Moritz Fischer

From: Moritz Fischer <mdf@kernel.org>

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 <mdf@kernel.org>
---
 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON
  2017-04-07 22:00 [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Moritz Fischer
@ 2017-04-07 22:00 ` Moritz Fischer
  2017-04-13 20:01   ` Rob Herring
  2017-04-07 22:00 ` [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver Moritz Fischer
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Moritz Fischer @ 2017-04-07 22:00 UTC (permalink / raw)
  To: linux-hwmon
  Cc: moritz.fischer, linux-kernel, devicetree, lee.jones, olof, linux,
	jdelvare, robh+dt, mark.rutland, Moritz Fischer

From: Moritz Fischer <mdf@kernel.org>

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 <mdf@kernel.org>
---
 .../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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver
  2017-04-07 22:00 [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Moritz Fischer
  2017-04-07 22:00 ` [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON Moritz Fischer
@ 2017-04-07 22:00 ` Moritz Fischer
  2017-04-13 21:34   ` Guenter Roeck
  2017-04-09 23:02 ` [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Guenter Roeck
  2017-04-13 21:03 ` Guenter Roeck
  3 siblings, 1 reply; 13+ messages in thread
From: Moritz Fischer @ 2017-04-07 22:00 UTC (permalink / raw)
  To: linux-hwmon
  Cc: moritz.fischer, linux-kernel, devicetree, lee.jones, olof, linux,
	jdelvare, robh+dt, mark.rutland, Moritz Fischer

From: Moritz Fischer <mdf@kernel.org>

This adds a hwmon driver for the Chromium EC's fans
and temperature sensors.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

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 <mdf@kernel.org>");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
  2017-04-07 22:00 [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Moritz Fischer
  2017-04-07 22:00 ` [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON Moritz Fischer
  2017-04-07 22:00 ` [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver Moritz Fischer
@ 2017-04-09 23:02 ` Guenter Roeck
  2017-04-10  0:40   ` Moritz Fischer
  2017-04-13 21:03 ` Guenter Roeck
  3 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2017-04-09 23:02 UTC (permalink / raw)
  To: Moritz Fischer, linux-hwmon
  Cc: linux-kernel, devicetree, lee.jones, olof, jdelvare, robh+dt,
	mark.rutland, Moritz Fischer, Benson Leung

On 04/07/2017 03:00 PM, Moritz Fischer wrote:
> From: Moritz Fischer <mdf@kernel.org>
>
> 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 <mdf@kernel.org>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
  2017-04-09 23:02 ` [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Guenter Roeck
@ 2017-04-10  0:40   ` Moritz Fischer
  0 siblings, 0 replies; 13+ messages in thread
From: Moritz Fischer @ 2017-04-10  0:40 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Moritz Fischer, linux-hwmon, Linux Kernel Mailing List,
	Devicetree List, lee.jones, olof, jdelvare, Rob Herring,
	Mark Rutland, Moritz Fischer, Benson Leung

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 <mdf@kernel.org>
> >
> > 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 <mdf@kernel.org>
>
> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON
  2017-04-07 22:00 ` [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON Moritz Fischer
@ 2017-04-13 20:01   ` Rob Herring
  2017-04-13 21:07     ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2017-04-13 20:01 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: linux-hwmon, linux-kernel, devicetree, lee.jones, olof, linux,
	jdelvare, mark.rutland, Moritz Fischer

On Fri, Apr 07, 2017 at 03:00:09PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <mdf@kernel.org>
> 
> 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 <mdf@kernel.org>
> ---
>  .../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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
  2017-04-07 22:00 [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Moritz Fischer
                   ` (2 preceding siblings ...)
  2017-04-09 23:02 ` [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Guenter Roeck
@ 2017-04-13 21:03 ` Guenter Roeck
  2017-04-13 22:53   ` Moritz Fischer
  3 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2017-04-13 21:03 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: linux-hwmon, linux-kernel, devicetree, lee.jones, olof, jdelvare,
	robh+dt, mark.rutland, Moritz Fischer

On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <mdf@kernel.org>
> 
> 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 <mdf@kernel.org>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON
  2017-04-13 20:01   ` Rob Herring
@ 2017-04-13 21:07     ` Guenter Roeck
  2017-04-14 12:48       ` Rob Herring
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2017-04-13 21:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: Moritz Fischer, linux-hwmon, linux-kernel, devicetree, lee.jones,
	olof, jdelvare, mark.rutland, Moritz Fischer

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 <mdf@kernel.org>
> > 
> > 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 <mdf@kernel.org>
> > ---
> >  .../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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver
  2017-04-07 22:00 ` [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver Moritz Fischer
@ 2017-04-13 21:34   ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2017-04-13 21:34 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: linux-hwmon, linux-kernel, devicetree, lee.jones, olof, jdelvare,
	robh+dt, mark.rutland, Moritz Fischer

On Fri, Apr 07, 2017 at 03:00:10PM -0700, Moritz Fischer wrote:
> From: Moritz Fischer <mdf@kernel.org>
> 
> This adds a hwmon driver for the Chromium EC's fans
> and temperature sensors.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> 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 <mdf@kernel.org>");
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
  2017-04-13 21:03 ` Guenter Roeck
@ 2017-04-13 22:53   ` Moritz Fischer
  2017-04-14  2:49     ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Moritz Fischer @ 2017-04-13 22:53 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Moritz Fischer, linux-hwmon, Linux Kernel Mailing List,
	Devicetree List, Lee Jones, olof, jdelvare, Rob Herring,
	Mark Rutland

Hi Guenter,

On Thu, Apr 13, 2017 at 2:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
>> From: Moritz Fischer <mdf@kernel.org>
>>
>> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory
  2017-04-13 22:53   ` Moritz Fischer
@ 2017-04-14  2:49     ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2017-04-14  2:49 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Moritz Fischer, linux-hwmon, Linux Kernel Mailing List,
	Devicetree List, Lee Jones, olof, jdelvare, Rob Herring,
	Mark Rutland

On 04/13/2017 03:53 PM, Moritz Fischer wrote:
> Hi Guenter,
>
> On Thu, Apr 13, 2017 at 2:03 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Fri, Apr 07, 2017 at 03:00:08PM -0700, Moritz Fischer wrote:
>>> From: Moritz Fischer <mdf@kernel.org>
>>>
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON
  2017-04-13 21:07     ` Guenter Roeck
@ 2017-04-14 12:48       ` Rob Herring
  2017-04-14 17:47         ` Moritz Fischer
  0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2017-04-14 12:48 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Moritz Fischer, linux-hwmon, linux-kernel, devicetree, Lee Jones,
	Olof Johansson, Jean Delvare, Mark Rutland, Moritz Fischer

On Thu, Apr 13, 2017 at 4:07 PM, Guenter Roeck <linux@roeck-us.net> 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 <mdf@kernel.org>
>> >
>> > 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 <mdf@kernel.org>
>> > ---
>> >  .../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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON
  2017-04-14 12:48       ` Rob Herring
@ 2017-04-14 17:47         ` Moritz Fischer
  0 siblings, 0 replies; 13+ messages in thread
From: Moritz Fischer @ 2017-04-14 17:47 UTC (permalink / raw)
  To: Rob Herring
  Cc: Guenter Roeck, Moritz Fischer, linux-hwmon, linux-kernel,
	devicetree, Lee Jones, Olof Johansson, Jean Delvare,
	Mark Rutland

On Fri, Apr 14, 2017 at 5:48 AM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Apr 13, 2017 at 4:07 PM, Guenter Roeck <linux@roeck-us.net> 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 <mdf@kernel.org>
>>> >
>>> > 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 <mdf@kernel.org>
>>> > ---
>>> >  .../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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-04-14 17:47 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 22:00 [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Moritz Fischer
2017-04-07 22:00 ` [PATCH 2/3] dt-bindings: hwmon: Add bindings for Google Chromium EC HWMON Moritz Fischer
2017-04-13 20:01   ` Rob Herring
2017-04-13 21:07     ` Guenter Roeck
2017-04-14 12:48       ` Rob Herring
2017-04-14 17:47         ` Moritz Fischer
2017-04-07 22:00 ` [PATCH 3/3] hwmon: cros-ec-hwmon: Add Chromium-EC HWMON driver Moritz Fischer
2017-04-13 21:34   ` Guenter Roeck
2017-04-09 23:02 ` [PATCH 1/3] mfd: cros-ec: Add functions to read mapped memory Guenter Roeck
2017-04-10  0:40   ` Moritz Fischer
2017-04-13 21:03 ` Guenter Roeck
2017-04-13 22:53   ` Moritz Fischer
2017-04-14  2:49     ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).