[PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC

[PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC

[Rick Altherr]

Signed-off-by: Rick Altherr <raltherr@google.com>
---
 .../devicetree/bindings/hwmon/aspeed_adc.txt       | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
new file mode 100644
index 000000000000..9e481668c4d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
@@ -0,0 +1,48 @@
+Aspeed AST2400/2500 ADC
+
+This device is a 10-bit converter for 16 voltage channels.  All inputs are
+single ended.  Each channel can be individually enabled to allow for use of
+alternate pin functions.
+
+1) adc node
+
+  Required properties:
+  - compatible : Should be one of
+	"aspeed,ast2400-adc"
+	"aspeed,ast2500-adc"
+  - reg : memory window mapping address and length
+  - #address-cells : must be <1> corresponding to the channel child binding
+  - #size-cells : must be <0> corresponding to the channel child binding
+  - clocks : Input clock used to derive the sample clock. Expected to be the
+    SoC's APB clock.
+  - update-interval-ms : initial time between updates on a channel
+
+  The node contains child nodes for each channel that the platform uses.
+
+  Example adc node:
+    adc@1e6e9000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "aspeed,ast2400-adc";
+	reg = <0x1e6e9000 0xB0>;
+	clocks = <&clk_apb>;
+	update-interval-ms = <100>;
+
+	[ child node definitions... ]
+    };
+
+2) channel nodes
+
+  Optional properties:
+  - status: indicates the operational status of the device.
+    Value must be either "disabled" or "okay".
+  - label : string describing the monitored value
+
+  Example channel node:
+  channel@1 {
+	status = "okay";
+	label = "3V3 rail";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_default>;
+  };
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Rick Altherr]

Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
driver implements reading the ADC values, enabling channels via device
tree, and optionally providing channel labels via device tree.  Low and
high threshold interrupts are supported by the hardware but not
implemented.

Signed-off-by: Rick Altherr <raltherr@google.com>
---
 drivers/hwmon/Kconfig      |  10 ++
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/aspeed_adc.c | 383 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 394 insertions(+)
 create mode 100644 drivers/hwmon/aspeed_adc.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3d16..c99a67b4afe4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -261,6 +261,16 @@ config SENSORS_ASC7621
 	  This driver can also be built as a module.  If so, the module
 	  will be called asc7621.
 
+config SENSORS_ASPEED_ADC
+	tristate "Aspeed AST2400/AST2500 ADC"
+	depends on ARCH_ASPEED
+	help
+	  If you say yes here you get support for the Aspeed AST2400/AST2500
+	  ADC.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called aspeed_adc.
+
 config SENSORS_K8TEMP
 	tristate "AMD Athlon64/FX or Opteron temperature sensor"
 	depends on X86 && PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 5509edf6186a..eede049c9d0d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
 obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
+obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c
new file mode 100644
index 000000000000..098e32315264
--- /dev/null
+++ b/drivers/hwmon/aspeed_adc.c
@@ -0,0 +1,383 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include <asm/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define ASPEED_ADC_NUM_CHANNELS	16
+#define ASPEED_ADC_REF_VOLTAGE	2500 /* millivolts */
+
+#define ASPEED_ADC_REG_ADC00 0x00
+#define ASPEED_ADC_REG_ADC04 0x04
+#define ASPEED_ADC_REG_ADC08 0x08
+#define ASPEED_ADC_REG_ADC0C 0x0C
+#define ASPEED_ADC_REG_ADC10 0x10
+#define ASPEED_ADC_REG_ADC14 0x14
+#define ASPEED_ADC_REG_ADC18 0x18
+#define ASPEED_ADC_REG_ADC1C 0x1C
+#define ASPEED_ADC_REG_ADC20 0x20
+#define ASPEED_ADC_REG_ADC24 0x24
+#define ASPEED_ADC_REG_ADC28 0x28
+#define ASPEED_ADC_REG_ADC2C 0x2C
+
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN	(0x0 << 1)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY	(0x1 << 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL	(0x7 << 1)
+
+#define ASPEED_ADC_ENGINE_ENABLE	BIT(0)
+
+struct aspeed_adc_data {
+	struct device	*dev;
+	void __iomem	*base;
+	struct clk	*pclk;
+	struct mutex	lock;
+	unsigned long	update_interval_ms;
+	u32		enabled_channel_mask;
+	const char*	channel_labels[ASPEED_ADC_NUM_CHANNELS];
+};
+
+static int aspeed_adc_set_adc_clock_frequency(
+		struct aspeed_adc_data *data,
+		unsigned long desired_update_interval_ms)
+{
+	long pclk_hz = clk_get_rate(data->pclk);
+	long adc_combined_divisor;
+	long adc_pre_divisor;
+	long adc_divisor;
+	long adc_clock_control_reg_val;
+        long num_enabled_channels = hweight32(data->enabled_channel_mask);
+
+	if (desired_update_interval_ms < 1)
+		return -EINVAL;
+
+	/* From the AST2400 datasheet:
+	 *   adc_period_s = pclk_period_s * 2 * (pre_divisor+1) * (divisor+1)
+	 *
+	 *   and
+	 *
+	 *   sample_period_s = 12 * adc_period_s
+	 *
+	 * Substitute pclk_period_s = (1 / pclk_hz)
+	 *
+	 * Solve for (pre-divisor+1) * (divisor+1) as those are settings we need
+	 * to program:
+	 *   (pre-divisor+1) * (divisor+1) = (sample_period_s * pclk_hz) / 24
+	 */
+
+	/* Assume PCLK runs at a fairly high frequency so dividing it first
+	 * loses little accuracy.  Also note that the above formulas have
+	 * sample_period in seconds while our desired_update_interval is in
+	 * milliseconds, hence the divide by 1000.
+	 */
+	adc_combined_divisor = desired_update_interval_ms *
+			(pclk_hz / 24 / 1000 / num_enabled_channels);
+
+	/* Prefer to use the ADC divisor over the ADC pre-divisor.  ADC divisor
+	 * is 10-bits wide so anything over 1024 must use the pre-divisor.
+	 */
+	adc_pre_divisor = 1;
+	while (adc_combined_divisor/adc_pre_divisor > (1<<10)) {
+		adc_pre_divisor += 1;
+	};
+	adc_divisor = adc_combined_divisor / adc_pre_divisor;
+
+	/* Since ADC divisor and pre-divisor are used in division, the register
+	 * value is implicitly incremented by one before use.  This means we
+	 * need to subtract one from our calculated values above.
+	 */
+	adc_pre_divisor -= 1;
+	adc_divisor -= 1;
+
+	adc_clock_control_reg_val = (adc_pre_divisor << 17) | adc_divisor;
+
+	mutex_lock(&data->lock);
+	data->update_interval_ms =
+			(adc_pre_divisor + 1) * (adc_divisor + 1)
+			/ (pclk_hz / 24 / 1000);
+	writel(adc_clock_control_reg_val, data->base + ASPEED_ADC_REG_ADC0C);
+	mutex_unlock(&data->lock);
+
+	return 0;
+}
+
+static int aspeed_adc_get_channel_reading(struct aspeed_adc_data *data,
+						int channel, long *val)
+{
+	u32 data_reg;
+	u32 data_reg_val;
+	long adc_val;
+
+	/* Each 32-bit data register contains 2 10-bit ADC values. */
+	data_reg = ASPEED_ADC_REG_ADC10 + (channel / 2) * 4;
+	data_reg_val = readl(data->base + data_reg);
+	if (channel % 2 == 0) {
+		adc_val = data_reg_val & 0x3FF;
+	} else {
+		adc_val = (data_reg_val >> 16) & 0x3FF;
+	}
+
+	/* Scale 10-bit input reading to millivolts. */
+	*val = adc_val * ASPEED_ADC_REF_VOLTAGE / 1024;
+
+	return 0;
+}
+
+static umode_t aspeed_adc_hwmon_is_visible(const void *_data,
+						enum hwmon_sensor_types type,
+						u32 attr, int channel)
+{
+	const struct aspeed_adc_data* data = _data;
+
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
+		return S_IRUGO | S_IWUSR;
+	} else if (type == hwmon_in) {
+		/* Only channels that are enabled should be visible. */
+		if (channel >= 0 && channel <= ASPEED_ADC_NUM_CHANNELS &&
+		    (data->enabled_channel_mask & BIT(channel))) {
+
+			/* All enabled channels have an input but labels are
+			 * optional in the device tree.
+			 */
+			if (attr == hwmon_in_input) {
+				return S_IRUGO;
+			} else if (attr == hwmon_in_label &&
+					data->channel_labels[channel] != NULL) {
+				return S_IRUGO;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int aspeed_adc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct aspeed_adc_data *data = dev_get_drvdata(dev);
+
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
+		*val = data->update_interval_ms;
+		return 0;
+	} else if (type == hwmon_in && attr == hwmon_in_input) {
+		return aspeed_adc_get_channel_reading(data, channel, val);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int aspeed_adc_hwmon_read_string(struct device *dev,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel, char **str)
+{
+	struct aspeed_adc_data *data = dev_get_drvdata(dev);
+
+	if (type == hwmon_in && attr == hwmon_in_label) {
+		*str = (char*)data->channel_labels[channel];
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int aspeed_adc_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long val)
+{
+	struct aspeed_adc_data *data = dev_get_drvdata(dev);
+
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
+		return aspeed_adc_set_adc_clock_frequency(data, val);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops aspeed_adc_hwmon_ops = {
+	.is_visible = aspeed_adc_hwmon_is_visible,
+	.read = aspeed_adc_hwmon_read,
+	.read_string = aspeed_adc_hwmon_read_string,
+	.write = aspeed_adc_hwmon_write,
+};
+
+static const u32 aspeed_adc_hwmon_chip_config[] = {
+	HWMON_C_UPDATE_INTERVAL,
+	0
+};
+
+static const struct hwmon_channel_info aspeed_adc_hwmon_chip_channel = {
+	.type = hwmon_chip,
+	.config = aspeed_adc_hwmon_chip_config,
+};
+
+static const u32 aspeed_adc_hwmon_in_config[] = {
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	0
+};
+
+static const struct hwmon_channel_info aspeed_adc_hwmon_in_channel = {
+	.type = hwmon_in,
+	.config = aspeed_adc_hwmon_in_config,
+};
+
+static const struct hwmon_channel_info *aspeed_adc_hwmon_channel_info[] = {
+	&aspeed_adc_hwmon_chip_channel,
+	&aspeed_adc_hwmon_in_channel,
+	NULL
+};
+
+static const struct hwmon_chip_info aspeed_adc_hwmon_chip_info = {
+	.ops = &aspeed_adc_hwmon_ops,
+	.info = aspeed_adc_hwmon_channel_info,
+};
+
+static int aspeed_adc_probe(struct platform_device *pdev)
+{
+	struct aspeed_adc_data *data;
+	struct resource *res;
+	int ret;
+	struct device *hwmon_dev;
+	u32 desired_update_interval_ms;
+	u32 adc_engine_control_reg_val;
+	struct device_node* node;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->pclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->pclk)) {
+		dev_err(&pdev->dev, "clk_get failed\n");
+		return PTR_ERR(data->pclk);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+			"update-interval-ms", &desired_update_interval_ms);
+	if (ret < 0 || desired_update_interval_ms == 0) {
+		dev_err(&pdev->dev,
+				"Missing or zero update-interval-ms property, "
+				"defaulting to 100ms\n");
+		desired_update_interval_ms = 100;
+	}
+
+	mutex_init(&data->lock);
+
+	ret = clk_prepare_enable(data->pclk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		mutex_destroy(&data->lock);
+                return ret;
+	}
+
+	/* Figure out which channels are marked available in the device tree. */
+	data->enabled_channel_mask = 0;
+	for_each_available_child_of_node(pdev->dev.of_node, node) {
+		u32 pval;
+		unsigned int channel;
+
+		if (of_property_read_u32(node, "reg", &pval)) {
+			dev_err(&pdev->dev, "invalid reg on %s\n",
+				node->full_name);
+			continue;
+		}
+
+		channel = pval;
+		if (channel >= ASPEED_ADC_NUM_CHANNELS) {
+			dev_err(&pdev->dev,
+				"invalid channel index %d on %s\n",
+				channel, node->full_name);
+			continue;
+		}
+
+		data->enabled_channel_mask |= BIT(channel);
+
+		/* Cache a pointer to the label if one is specified.  Ignore any
+		 * errors as the label property is optional.
+		 */
+		of_property_read_string(node, "label", &data->channel_labels[channel]);
+	}
+
+	platform_set_drvdata(pdev, data);
+	aspeed_adc_set_adc_clock_frequency(data, desired_update_interval_ms);
+
+	/* Start all the requested channels in normal mode. */
+	adc_engine_control_reg_val = (data->enabled_channel_mask << 16) |
+		ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
+	writel(adc_engine_control_reg_val, data->base + ASPEED_ADC_REG_ADC00);
+
+	/* Register sysfs hooks */
+	hwmon_dev = devm_hwmon_device_register_with_info(
+			&pdev->dev, "aspeed_adc", data,
+			&aspeed_adc_hwmon_chip_info, NULL);
+	if (IS_ERR(hwmon_dev)) {
+		clk_disable_unprepare(data->pclk);
+		mutex_destroy(&data->lock);
+		return PTR_ERR(hwmon_dev);
+	}
+
+	return 0;
+}
+
+static int aspeed_adc_remove(struct platform_device *pdev) {
+	struct aspeed_adc_data *data = dev_get_drvdata(&pdev->dev);
+	clk_disable_unprepare(data->pclk);
+	mutex_destroy(&data->lock);
+	return 0;
+}
+
+const struct of_device_id aspeed_adc_matches[] = {
+	{ .compatible = "aspeed,ast2400-adc" },
+	{ .compatible = "aspeed,ast2500-adc" },
+};
+MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
+
+static struct platform_driver aspeed_adc_driver = {
+	.probe = aspeed_adc_probe,
+	.remove = aspeed_adc_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = aspeed_adc_matches,
+	}
+};
+
+module_platform_driver(aspeed_adc_driver);
+
+MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
+MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Joel Stanley]

On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
> driver implements reading the ADC values, enabling channels via device
> tree, and optionally providing channel labels via device tree.  Low and
> high threshold interrupts are supported by the hardware but not
> implemented.
>
> Signed-off-by: Rick Altherr <raltherr@google.com>

Looks good. Some minor comments below.

Is there a reason you wrote a hwmon driver instead of an iio driver? I
wasn't sure what the recommended subsystem is.

> ---
>  drivers/hwmon/Kconfig      |  10 ++
>  drivers/hwmon/Makefile     |   1 +
>  drivers/hwmon/aspeed_adc.c | 383 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 394 insertions(+)
>  create mode 100644 drivers/hwmon/aspeed_adc.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0649d53f3d16..c99a67b4afe4 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -261,6 +261,16 @@ config SENSORS_ASC7621
>           This driver can also be built as a module.  If so, the module
>           will be called asc7621.
>
> +config SENSORS_ASPEED_ADC
> +       tristate "Aspeed AST2400/AST2500 ADC"
> +       depends on ARCH_ASPEED

depends on ARCH_ASPEED || COMPILE_TEST.

> +       help
> +         If you say yes here you get support for the Aspeed AST2400/AST2500
> +         ADC.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called aspeed_adc.
> +
>  config SENSORS_K8TEMP
>         tristate "AMD Athlon64/FX or Opteron temperature sensor"
>         depends on X86 && PCI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5509edf6186a..eede049c9d0d 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
>  obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)  += asc7621.o
> +obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o
>  obj-$(CONFIG_SENSORS_ATXP1)    += atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c
> new file mode 100644
> index 000000000000..098e32315264
> --- /dev/null
> +++ b/drivers/hwmon/aspeed_adc.c
> @@ -0,0 +1,383 @@
> +/*
> + * Aspeed AST2400/2500 ADC
> + *
> + * Copyright (C) 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + */
> +
> +#include <asm/io.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define ASPEED_ADC_NUM_CHANNELS        16
> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
> +
> +#define ASPEED_ADC_REG_ADC00 0x00
> +#define ASPEED_ADC_REG_ADC04 0x04
> +#define ASPEED_ADC_REG_ADC08 0x08
> +#define ASPEED_ADC_REG_ADC0C 0x0C
> +#define ASPEED_ADC_REG_ADC10 0x10
> +#define ASPEED_ADC_REG_ADC14 0x14
> +#define ASPEED_ADC_REG_ADC18 0x18
> +#define ASPEED_ADC_REG_ADC1C 0x1C
> +#define ASPEED_ADC_REG_ADC20 0x20
> +#define ASPEED_ADC_REG_ADC24 0x24
> +#define ASPEED_ADC_REG_ADC28 0x28
> +#define ASPEED_ADC_REG_ADC2C 0x2C

I'm not sure that these defines add any value. I'd either give them
names such as "ASPEED_ADC_ENGINE_CTRL". or just use the register
offset directly.

> +
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN   (0x0 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_STANDBY      (0x1 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL       (0x7 << 1)
> +
> +#define ASPEED_ADC_ENGINE_ENABLE       BIT(0)
> +
> +struct aspeed_adc_data {
> +       struct device   *dev;
> +       void __iomem    *base;
> +       struct clk      *pclk;
> +       struct mutex    lock;
> +       unsigned long   update_interval_ms;
> +       u32             enabled_channel_mask;
> +       const char*     channel_labels[ASPEED_ADC_NUM_CHANNELS];
> +};
> +
> +static int aspeed_adc_set_adc_clock_frequency(
> +               struct aspeed_adc_data *data,
> +               unsigned long desired_update_interval_ms)
> +{
> +       long pclk_hz = clk_get_rate(data->pclk);
> +       long adc_combined_divisor;
> +       long adc_pre_divisor;
> +       long adc_divisor;
> +       long adc_clock_control_reg_val;
> +        long num_enabled_channels = hweight32(data->enabled_channel_mask);

Some whitespace damage here.

> +
> +       if (desired_update_interval_ms < 1)
> +               return -EINVAL;
> +
> +       /* From the AST2400 datasheet:

Nit: kernel style is to leave a blank line on the first line of
multi-line comments:

 /*
  * Foo
  * etc

> +        *   adc_period_s = pclk_period_s * 2 * (pre_divisor+1) * (divisor+1)
> +        *
> +        *   and
> +        *
> +        *   sample_period_s = 12 * adc_period_s
> +        *
> +        * Substitute pclk_period_s = (1 / pclk_hz)
> +        *
> +        * Solve for (pre-divisor+1) * (divisor+1) as those are settings we need
> +        * to program:
> +        *   (pre-divisor+1) * (divisor+1) = (sample_period_s * pclk_hz) / 24
> +        */
> +
> +       /* Assume PCLK runs at a fairly high frequency so dividing it first
> +        * loses little accuracy.  Also note that the above formulas have
> +        * sample_period in seconds while our desired_update_interval is in
> +        * milliseconds, hence the divide by 1000.
> +        */
> +       adc_combined_divisor = desired_update_interval_ms *
> +                       (pclk_hz / 24 / 1000 / num_enabled_channels);
> +
> +       /* Prefer to use the ADC divisor over the ADC pre-divisor.  ADC divisor
> +        * is 10-bits wide so anything over 1024 must use the pre-divisor.
> +        */
> +       adc_pre_divisor = 1;
> +       while (adc_combined_divisor/adc_pre_divisor > (1<<10)) {
> +               adc_pre_divisor += 1;
> +       };
> +       adc_divisor = adc_combined_divisor / adc_pre_divisor;
> +
> +       /* Since ADC divisor and pre-divisor are used in division, the register
> +        * value is implicitly incremented by one before use.  This means we
> +        * need to subtract one from our calculated values above.
> +        */
> +       adc_pre_divisor -= 1;
> +       adc_divisor -= 1;
> +
> +       adc_clock_control_reg_val = (adc_pre_divisor << 17) | adc_divisor;
> +
> +       mutex_lock(&data->lock);
> +       data->update_interval_ms =
> +                       (adc_pre_divisor + 1) * (adc_divisor + 1)
> +                       / (pclk_hz / 24 / 1000);
> +       writel(adc_clock_control_reg_val, data->base + ASPEED_ADC_REG_ADC0C);
> +       mutex_unlock(&data->lock);
> +
> +       return 0;
> +}
> +
> +static int aspeed_adc_get_channel_reading(struct aspeed_adc_data *data,
> +                                               int channel, long *val)
> +{
> +       u32 data_reg;
> +       u32 data_reg_val;
> +       long adc_val;
> +
> +       /* Each 32-bit data register contains 2 10-bit ADC values. */
> +       data_reg = ASPEED_ADC_REG_ADC10 + (channel / 2) * 4;
> +       data_reg_val = readl(data->base + data_reg);
> +       if (channel % 2 == 0) {
> +               adc_val = data_reg_val & 0x3FF;
> +       } else {
> +               adc_val = (data_reg_val >> 16) & 0x3FF;
> +       }

I wonder if you could replace the above block with:

            adc_val = readw(data->base + ASPEED_ADC_REG_ADC10 + channel);

> +
> +       /* Scale 10-bit input reading to millivolts. */
> +       *val = adc_val * ASPEED_ADC_REF_VOLTAGE / 1024;
> +
> +       return 0;
> +}
> +
> +static umode_t aspeed_adc_hwmon_is_visible(const void *_data,
> +                                               enum hwmon_sensor_types type,
> +                                               u32 attr, int channel)
> +{
> +       const struct aspeed_adc_data* data = _data;
> +
> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +               return S_IRUGO | S_IWUSR;
> +       } else if (type == hwmon_in) {
> +               /* Only channels that are enabled should be visible. */
> +               if (channel >= 0 && channel <= ASPEED_ADC_NUM_CHANNELS &&
> +                   (data->enabled_channel_mask & BIT(channel))) {
> +
> +                       /* All enabled channels have an input but labels are
> +                        * optional in the device tree.
> +                        */
> +                       if (attr == hwmon_in_input) {
> +                               return S_IRUGO;
> +                       } else if (attr == hwmon_in_label &&
> +                                       data->channel_labels[channel] != NULL) {
> +                               return S_IRUGO;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int aspeed_adc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +                        u32 attr, int channel, long *val)
> +{
> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
> +
> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +               *val = data->update_interval_ms;
> +               return 0;
> +       } else if (type == hwmon_in && attr == hwmon_in_input) {
> +               return aspeed_adc_get_channel_reading(data, channel, val);
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static int aspeed_adc_hwmon_read_string(struct device *dev,
> +                                       enum hwmon_sensor_types type,
> +                                       u32 attr, int channel, char **str)
> +{
> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
> +
> +       if (type == hwmon_in && attr == hwmon_in_label) {
> +               *str = (char*)data->channel_labels[channel];
> +               return 0;
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static int aspeed_adc_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +                         u32 attr, int channel, long val)
> +{
> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
> +
> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +               return aspeed_adc_set_adc_clock_frequency(data, val);
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops aspeed_adc_hwmon_ops = {
> +       .is_visible = aspeed_adc_hwmon_is_visible,
> +       .read = aspeed_adc_hwmon_read,
> +       .read_string = aspeed_adc_hwmon_read_string,
> +       .write = aspeed_adc_hwmon_write,
> +};
> +
> +static const u32 aspeed_adc_hwmon_chip_config[] = {
> +       HWMON_C_UPDATE_INTERVAL,
> +       0
> +};
> +
> +static const struct hwmon_channel_info aspeed_adc_hwmon_chip_channel = {
> +       .type = hwmon_chip,
> +       .config = aspeed_adc_hwmon_chip_config,
> +};
> +
> +static const u32 aspeed_adc_hwmon_in_config[] = {
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       HWMON_I_INPUT | HWMON_I_LABEL,
> +       0
> +};
> +
> +static const struct hwmon_channel_info aspeed_adc_hwmon_in_channel = {
> +       .type = hwmon_in,
> +       .config = aspeed_adc_hwmon_in_config,
> +};
> +
> +static const struct hwmon_channel_info *aspeed_adc_hwmon_channel_info[] = {
> +       &aspeed_adc_hwmon_chip_channel,
> +       &aspeed_adc_hwmon_in_channel,
> +       NULL
> +};
> +
> +static const struct hwmon_chip_info aspeed_adc_hwmon_chip_info = {
> +       .ops = &aspeed_adc_hwmon_ops,
> +       .info = aspeed_adc_hwmon_channel_info,
> +};
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> +       struct aspeed_adc_data *data;
> +       struct resource *res;
> +       int ret;
> +       struct device *hwmon_dev;
> +       u32 desired_update_interval_ms;
> +       u32 adc_engine_control_reg_val;
> +       struct device_node* node;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->pclk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(data->pclk)) {
> +               dev_err(&pdev->dev, "clk_get failed\n");
> +               return PTR_ERR(data->pclk);
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       data->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(data->base))
> +               return PTR_ERR(data->base);
> +
> +       ret = of_property_read_u32(pdev->dev.of_node,
> +                       "update-interval-ms", &desired_update_interval_ms);
> +       if (ret < 0 || desired_update_interval_ms == 0) {
> +               dev_err(&pdev->dev,
> +                               "Missing or zero update-interval-ms property, "
> +                               "defaulting to 100ms\n");

Put the string on one line so it can be easily searched for.

> +               desired_update_interval_ms = 100;
> +       }
> +
> +       mutex_init(&data->lock);
> +
> +       ret = clk_prepare_enable(data->pclk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to enable clock\n");
> +               mutex_destroy(&data->lock);
> +                return ret;

Strange whitespace here.

> +       }
> +
> +       /* Figure out which channels are marked available in the device tree. */
> +       data->enabled_channel_mask = 0;
> +       for_each_available_child_of_node(pdev->dev.of_node, node) {
> +               u32 pval;
> +               unsigned int channel;
> +
> +               if (of_property_read_u32(node, "reg", &pval)) {
> +                       dev_err(&pdev->dev, "invalid reg on %s\n",
> +                               node->full_name);
> +                       continue;
> +               }
> +
> +               channel = pval;
> +               if (channel >= ASPEED_ADC_NUM_CHANNELS) {
> +                       dev_err(&pdev->dev,
> +                               "invalid channel index %d on %s\n",
> +                               channel, node->full_name);
> +                       continue;
> +               }
> +
> +               data->enabled_channel_mask |= BIT(channel);
> +
> +               /* Cache a pointer to the label if one is specified.  Ignore any
> +                * errors as the label property is optional.
> +                */
> +               of_property_read_string(node, "label", &data->channel_labels[channel]);

I was wondering how long we could keep that pointer around. I think we
are ok for the lifetime of the driver, as we hold a reference to the
node in dev.of_node.

> +       }
> +
> +       platform_set_drvdata(pdev, data);
> +       aspeed_adc_set_adc_clock_frequency(data, desired_update_interval_ms);

This reads funny. aspeed_adc_set_clock_frequency instead?

> +
> +       /* Start all the requested channels in normal mode. */
> +       adc_engine_control_reg_val = (data->enabled_channel_mask << 16) |
> +               ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> +       writel(adc_engine_control_reg_val, data->base + ASPEED_ADC_REG_ADC00);
> +
> +       /* Register sysfs hooks */
> +       hwmon_dev = devm_hwmon_device_register_with_info(
> +                       &pdev->dev, "aspeed_adc", data,
> +                       &aspeed_adc_hwmon_chip_info, NULL);
> +       if (IS_ERR(hwmon_dev)) {
> +               clk_disable_unprepare(data->pclk);
> +               mutex_destroy(&data->lock);
> +               return PTR_ERR(hwmon_dev);
> +       }
> +
> +       return 0;
> +}
> +
> +static int aspeed_adc_remove(struct platform_device *pdev) {
> +       struct aspeed_adc_data *data = dev_get_drvdata(&pdev->dev);
> +       clk_disable_unprepare(data->pclk);
> +       mutex_destroy(&data->lock);
> +       return 0;
> +}
> +
> +const struct of_device_id aspeed_adc_matches[] = {
> +       { .compatible = "aspeed,ast2400-adc" },
> +       { .compatible = "aspeed,ast2500-adc" },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> +       .probe = aspeed_adc_probe,
> +       .remove = aspeed_adc_remove,
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .of_match_table = aspeed_adc_matches,
> +       }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0.483.g087da7b7c-goog
>

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Guenter Roeck]

On 02/28/2017 04:49 PM, Joel Stanley wrote:
> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
>> driver implements reading the ADC values, enabling channels via device
>> tree, and optionally providing channel labels via device tree.  Low and
>> high threshold interrupts are supported by the hardware but not
>> implemented.
>>
>> Signed-off-by: Rick Altherr <raltherr@google.com>
>
> Looks good. Some minor comments below.
>
> Is there a reason you wrote a hwmon driver instead of an iio driver? I
> wasn't sure what the recommended subsystem is.

Excellent point. Question is really if there is a plan to add support for
thresholds. If not, an iio driver might be more appropriate.

Guenter

>
>> ---
>>  drivers/hwmon/Kconfig      |  10 ++
>>  drivers/hwmon/Makefile     |   1 +
>>  drivers/hwmon/aspeed_adc.c | 383 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 394 insertions(+)
>>  create mode 100644 drivers/hwmon/aspeed_adc.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 0649d53f3d16..c99a67b4afe4 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -261,6 +261,16 @@ config SENSORS_ASC7621
>>           This driver can also be built as a module.  If so, the module
>>           will be called asc7621.
>>
>> +config SENSORS_ASPEED_ADC
>> +       tristate "Aspeed AST2400/AST2500 ADC"
>> +       depends on ARCH_ASPEED
>
> depends on ARCH_ASPEED || COMPILE_TEST.
>
>> +       help
>> +         If you say yes here you get support for the Aspeed AST2400/AST2500
>> +         ADC.
>> +
>> +         This driver can also be built as a module.  If so, the module
>> +         will be called aspeed_adc.
>> +
>>  config SENSORS_K8TEMP
>>         tristate "AMD Athlon64/FX or Opteron temperature sensor"
>>         depends on X86 && PCI
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 5509edf6186a..eede049c9d0d 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
>>  obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
>>  obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
>>  obj-$(CONFIG_SENSORS_ASC7621)  += asc7621.o
>> +obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o
>>  obj-$(CONFIG_SENSORS_ATXP1)    += atxp1.o
>>  obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
>>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>> diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c
>> new file mode 100644
>> index 000000000000..098e32315264
>> --- /dev/null
>> +++ b/drivers/hwmon/aspeed_adc.c
>> @@ -0,0 +1,383 @@
>> +/*
>> + * Aspeed AST2400/2500 ADC
>> + *
>> + * Copyright (C) 2017 Google, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <asm/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +
>> +#define ASPEED_ADC_NUM_CHANNELS        16
>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>> +
>> +#define ASPEED_ADC_REG_ADC00 0x00
>> +#define ASPEED_ADC_REG_ADC04 0x04
>> +#define ASPEED_ADC_REG_ADC08 0x08
>> +#define ASPEED_ADC_REG_ADC0C 0x0C
>> +#define ASPEED_ADC_REG_ADC10 0x10
>> +#define ASPEED_ADC_REG_ADC14 0x14
>> +#define ASPEED_ADC_REG_ADC18 0x18
>> +#define ASPEED_ADC_REG_ADC1C 0x1C
>> +#define ASPEED_ADC_REG_ADC20 0x20
>> +#define ASPEED_ADC_REG_ADC24 0x24
>> +#define ASPEED_ADC_REG_ADC28 0x28
>> +#define ASPEED_ADC_REG_ADC2C 0x2C
>
> I'm not sure that these defines add any value. I'd either give them
> names such as "ASPEED_ADC_ENGINE_CTRL". or just use the register
> offset directly.
>
>> +
>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN   (0x0 << 1)
>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY      (0x1 << 1)
>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL       (0x7 << 1)
>> +
>> +#define ASPEED_ADC_ENGINE_ENABLE       BIT(0)
>> +
>> +struct aspeed_adc_data {
>> +       struct device   *dev;
>> +       void __iomem    *base;
>> +       struct clk      *pclk;
>> +       struct mutex    lock;
>> +       unsigned long   update_interval_ms;
>> +       u32             enabled_channel_mask;
>> +       const char*     channel_labels[ASPEED_ADC_NUM_CHANNELS];
>> +};
>> +
>> +static int aspeed_adc_set_adc_clock_frequency(
>> +               struct aspeed_adc_data *data,
>> +               unsigned long desired_update_interval_ms)
>> +{
>> +       long pclk_hz = clk_get_rate(data->pclk);
>> +       long adc_combined_divisor;
>> +       long adc_pre_divisor;
>> +       long adc_divisor;
>> +       long adc_clock_control_reg_val;
>> +        long num_enabled_channels = hweight32(data->enabled_channel_mask);
>
> Some whitespace damage here.
>
>> +
>> +       if (desired_update_interval_ms < 1)
>> +               return -EINVAL;
>> +
>> +       /* From the AST2400 datasheet:
>
> Nit: kernel style is to leave a blank line on the first line of
> multi-line comments:
>
>  /*
>   * Foo
>   * etc
>
>> +        *   adc_period_s = pclk_period_s * 2 * (pre_divisor+1) * (divisor+1)
>> +        *
>> +        *   and
>> +        *
>> +        *   sample_period_s = 12 * adc_period_s
>> +        *
>> +        * Substitute pclk_period_s = (1 / pclk_hz)
>> +        *
>> +        * Solve for (pre-divisor+1) * (divisor+1) as those are settings we need
>> +        * to program:
>> +        *   (pre-divisor+1) * (divisor+1) = (sample_period_s * pclk_hz) / 24
>> +        */
>> +
>> +       /* Assume PCLK runs at a fairly high frequency so dividing it first
>> +        * loses little accuracy.  Also note that the above formulas have
>> +        * sample_period in seconds while our desired_update_interval is in
>> +        * milliseconds, hence the divide by 1000.
>> +        */
>> +       adc_combined_divisor = desired_update_interval_ms *
>> +                       (pclk_hz / 24 / 1000 / num_enabled_channels);
>> +
>> +       /* Prefer to use the ADC divisor over the ADC pre-divisor.  ADC divisor
>> +        * is 10-bits wide so anything over 1024 must use the pre-divisor.
>> +        */
>> +       adc_pre_divisor = 1;
>> +       while (adc_combined_divisor/adc_pre_divisor > (1<<10)) {
>> +               adc_pre_divisor += 1;
>> +       };
>> +       adc_divisor = adc_combined_divisor / adc_pre_divisor;
>> +
>> +       /* Since ADC divisor and pre-divisor are used in division, the register
>> +        * value is implicitly incremented by one before use.  This means we
>> +        * need to subtract one from our calculated values above.
>> +        */
>> +       adc_pre_divisor -= 1;
>> +       adc_divisor -= 1;
>> +
>> +       adc_clock_control_reg_val = (adc_pre_divisor << 17) | adc_divisor;
>> +
>> +       mutex_lock(&data->lock);
>> +       data->update_interval_ms =
>> +                       (adc_pre_divisor + 1) * (adc_divisor + 1)
>> +                       / (pclk_hz / 24 / 1000);
>> +       writel(adc_clock_control_reg_val, data->base + ASPEED_ADC_REG_ADC0C);
>> +       mutex_unlock(&data->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_adc_get_channel_reading(struct aspeed_adc_data *data,
>> +                                               int channel, long *val)
>> +{
>> +       u32 data_reg;
>> +       u32 data_reg_val;
>> +       long adc_val;
>> +
>> +       /* Each 32-bit data register contains 2 10-bit ADC values. */
>> +       data_reg = ASPEED_ADC_REG_ADC10 + (channel / 2) * 4;
>> +       data_reg_val = readl(data->base + data_reg);
>> +       if (channel % 2 == 0) {
>> +               adc_val = data_reg_val & 0x3FF;
>> +       } else {
>> +               adc_val = (data_reg_val >> 16) & 0x3FF;
>> +       }
>
> I wonder if you could replace the above block with:
>
>             adc_val = readw(data->base + ASPEED_ADC_REG_ADC10 + channel);
>
>> +
>> +       /* Scale 10-bit input reading to millivolts. */
>> +       *val = adc_val * ASPEED_ADC_REF_VOLTAGE / 1024;
>> +
>> +       return 0;
>> +}
>> +
>> +static umode_t aspeed_adc_hwmon_is_visible(const void *_data,
>> +                                               enum hwmon_sensor_types type,
>> +                                               u32 attr, int channel)
>> +{
>> +       const struct aspeed_adc_data* data = _data;
>> +
>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>> +               return S_IRUGO | S_IWUSR;
>> +       } else if (type == hwmon_in) {
>> +               /* Only channels that are enabled should be visible. */
>> +               if (channel >= 0 && channel <= ASPEED_ADC_NUM_CHANNELS &&
>> +                   (data->enabled_channel_mask & BIT(channel))) {
>> +
>> +                       /* All enabled channels have an input but labels are
>> +                        * optional in the device tree.
>> +                        */
>> +                       if (attr == hwmon_in_input) {
>> +                               return S_IRUGO;
>> +                       } else if (attr == hwmon_in_label &&
>> +                                       data->channel_labels[channel] != NULL) {
>> +                               return S_IRUGO;
>> +                       }
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_adc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +                        u32 attr, int channel, long *val)
>> +{
>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>> +
>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>> +               *val = data->update_interval_ms;
>> +               return 0;
>> +       } else if (type == hwmon_in && attr == hwmon_in_input) {
>> +               return aspeed_adc_get_channel_reading(data, channel, val);
>> +       }
>> +
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static int aspeed_adc_hwmon_read_string(struct device *dev,
>> +                                       enum hwmon_sensor_types type,
>> +                                       u32 attr, int channel, char **str)
>> +{
>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>> +
>> +       if (type == hwmon_in && attr == hwmon_in_label) {
>> +               *str = (char*)data->channel_labels[channel];
>> +               return 0;
>> +       }
>> +
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static int aspeed_adc_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>> +                         u32 attr, int channel, long val)
>> +{
>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>> +
>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>> +               return aspeed_adc_set_adc_clock_frequency(data, val);
>> +       }
>> +
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops aspeed_adc_hwmon_ops = {
>> +       .is_visible = aspeed_adc_hwmon_is_visible,
>> +       .read = aspeed_adc_hwmon_read,
>> +       .read_string = aspeed_adc_hwmon_read_string,
>> +       .write = aspeed_adc_hwmon_write,
>> +};
>> +
>> +static const u32 aspeed_adc_hwmon_chip_config[] = {
>> +       HWMON_C_UPDATE_INTERVAL,
>> +       0
>> +};
>> +
>> +static const struct hwmon_channel_info aspeed_adc_hwmon_chip_channel = {
>> +       .type = hwmon_chip,
>> +       .config = aspeed_adc_hwmon_chip_config,
>> +};
>> +
>> +static const u32 aspeed_adc_hwmon_in_config[] = {
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>> +       0
>> +};
>> +
>> +static const struct hwmon_channel_info aspeed_adc_hwmon_in_channel = {
>> +       .type = hwmon_in,
>> +       .config = aspeed_adc_hwmon_in_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *aspeed_adc_hwmon_channel_info[] = {
>> +       &aspeed_adc_hwmon_chip_channel,
>> +       &aspeed_adc_hwmon_in_channel,
>> +       NULL
>> +};
>> +
>> +static const struct hwmon_chip_info aspeed_adc_hwmon_chip_info = {
>> +       .ops = &aspeed_adc_hwmon_ops,
>> +       .info = aspeed_adc_hwmon_channel_info,
>> +};
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> +       struct aspeed_adc_data *data;
>> +       struct resource *res;
>> +       int ret;
>> +       struct device *hwmon_dev;
>> +       u32 desired_update_interval_ms;
>> +       u32 adc_engine_control_reg_val;
>> +       struct device_node* node;
>> +
>> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       data->pclk = devm_clk_get(&pdev->dev, NULL);
>> +       if (IS_ERR(data->pclk)) {
>> +               dev_err(&pdev->dev, "clk_get failed\n");
>> +               return PTR_ERR(data->pclk);
>> +       }
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       data->base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(data->base))
>> +               return PTR_ERR(data->base);
>> +
>> +       ret = of_property_read_u32(pdev->dev.of_node,
>> +                       "update-interval-ms", &desired_update_interval_ms);
>> +       if (ret < 0 || desired_update_interval_ms == 0) {
>> +               dev_err(&pdev->dev,
>> +                               "Missing or zero update-interval-ms property, "
>> +                               "defaulting to 100ms\n");
>
> Put the string on one line so it can be easily searched for.
>
>> +               desired_update_interval_ms = 100;
>> +       }
>> +
>> +       mutex_init(&data->lock);
>> +
>> +       ret = clk_prepare_enable(data->pclk);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to enable clock\n");
>> +               mutex_destroy(&data->lock);
>> +                return ret;
>
> Strange whitespace here.
>
>> +       }
>> +
>> +       /* Figure out which channels are marked available in the device tree. */
>> +       data->enabled_channel_mask = 0;
>> +       for_each_available_child_of_node(pdev->dev.of_node, node) {
>> +               u32 pval;
>> +               unsigned int channel;
>> +
>> +               if (of_property_read_u32(node, "reg", &pval)) {
>> +                       dev_err(&pdev->dev, "invalid reg on %s\n",
>> +                               node->full_name);
>> +                       continue;
>> +               }
>> +
>> +               channel = pval;
>> +               if (channel >= ASPEED_ADC_NUM_CHANNELS) {
>> +                       dev_err(&pdev->dev,
>> +                               "invalid channel index %d on %s\n",
>> +                               channel, node->full_name);
>> +                       continue;
>> +               }
>> +
>> +               data->enabled_channel_mask |= BIT(channel);
>> +
>> +               /* Cache a pointer to the label if one is specified.  Ignore any
>> +                * errors as the label property is optional.
>> +                */
>> +               of_property_read_string(node, "label", &data->channel_labels[channel]);
>
> I was wondering how long we could keep that pointer around. I think we
> are ok for the lifetime of the driver, as we hold a reference to the
> node in dev.of_node.
>
>> +       }
>> +
>> +       platform_set_drvdata(pdev, data);
>> +       aspeed_adc_set_adc_clock_frequency(data, desired_update_interval_ms);
>
> This reads funny. aspeed_adc_set_clock_frequency instead?
>
>> +
>> +       /* Start all the requested channels in normal mode. */
>> +       adc_engine_control_reg_val = (data->enabled_channel_mask << 16) |
>> +               ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>> +       writel(adc_engine_control_reg_val, data->base + ASPEED_ADC_REG_ADC00);
>> +
>> +       /* Register sysfs hooks */
>> +       hwmon_dev = devm_hwmon_device_register_with_info(
>> +                       &pdev->dev, "aspeed_adc", data,
>> +                       &aspeed_adc_hwmon_chip_info, NULL);
>> +       if (IS_ERR(hwmon_dev)) {
>> +               clk_disable_unprepare(data->pclk);
>> +               mutex_destroy(&data->lock);
>> +               return PTR_ERR(hwmon_dev);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_adc_remove(struct platform_device *pdev) {
>> +       struct aspeed_adc_data *data = dev_get_drvdata(&pdev->dev);
>> +       clk_disable_unprepare(data->pclk);
>> +       mutex_destroy(&data->lock);
>> +       return 0;
>> +}
>> +
>> +const struct of_device_id aspeed_adc_matches[] = {
>> +       { .compatible = "aspeed,ast2400-adc" },
>> +       { .compatible = "aspeed,ast2500-adc" },
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>> +
>> +static struct platform_driver aspeed_adc_driver = {
>> +       .probe = aspeed_adc_probe,
>> +       .remove = aspeed_adc_remove,
>> +       .driver = {
>> +               .name = KBUILD_MODNAME,
>> +               .of_match_table = aspeed_adc_matches,
>> +       }
>> +};
>> +
>> +module_platform_driver(aspeed_adc_driver);
>> +
>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.11.0.483.g087da7b7c-goog
>>
> --
> 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
>

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Rick Altherr]

Resending in plain text.

On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/28/2017 04:49 PM, Joel Stanley wrote:
>>
>> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
>>>
>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
>>> driver implements reading the ADC values, enabling channels via device
>>> tree, and optionally providing channel labels via device tree.  Low and
>>> high threshold interrupts are supported by the hardware but not
>>> implemented.
>>>
>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>
>>
>> Looks good. Some minor comments below.
>>
>> Is there a reason you wrote a hwmon driver instead of an iio driver? I
>> wasn't sure what the recommended subsystem is.
>
>
> Excellent point. Question is really if there is a plan to add support for
> thresholds. If not, an iio driver might be more appropriate.
>
> Guenter
>

The hardware supports firing interrupts on high and low thresholds.
I'm not planning to use that feature yet so I didn't implement it.
Would you prefer that I leave it as is (maybe with a TODO), implement
the thresholding, or change to iio?

Rick

>>
>>> ---
>>>  drivers/hwmon/Kconfig      |  10 ++
>>>  drivers/hwmon/Makefile     |   1 +
>>>  drivers/hwmon/aspeed_adc.c | 383
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 394 insertions(+)
>>>  create mode 100644 drivers/hwmon/aspeed_adc.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 0649d53f3d16..c99a67b4afe4 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -261,6 +261,16 @@ config SENSORS_ASC7621
>>>           This driver can also be built as a module.  If so, the module
>>>           will be called asc7621.
>>>
>>> +config SENSORS_ASPEED_ADC
>>> +       tristate "Aspeed AST2400/AST2500 ADC"
>>> +       depends on ARCH_ASPEED
>>
>>
>> depends on ARCH_ASPEED || COMPILE_TEST.
>>
>>> +       help
>>> +         If you say yes here you get support for the Aspeed
>>> AST2400/AST2500
>>> +         ADC.
>>> +
>>> +         This driver can also be built as a module.  If so, the module
>>> +         will be called aspeed_adc.
>>> +
>>>  config SENSORS_K8TEMP
>>>         tristate "AMD Athlon64/FX or Opteron temperature sensor"
>>>         depends on X86 && PCI
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 5509edf6186a..eede049c9d0d 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
>>>  obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
>>>  obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
>>>  obj-$(CONFIG_SENSORS_ASC7621)  += asc7621.o
>>> +obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o
>>>  obj-$(CONFIG_SENSORS_ATXP1)    += atxp1.o
>>>  obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
>>>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>>> diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c
>>> new file mode 100644
>>> index 000000000000..098e32315264
>>> --- /dev/null
>>> +++ b/drivers/hwmon/aspeed_adc.c
>>> @@ -0,0 +1,383 @@
>>> +/*
>>> + * Aspeed AST2400/2500 ADC
>>> + *
>>> + * Copyright (C) 2017 Google, Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + */
>>> +
>>> +#include <asm/io.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/types.h>
>>> +
>>> +#define ASPEED_ADC_NUM_CHANNELS        16
>>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>>> +
>>> +#define ASPEED_ADC_REG_ADC00 0x00
>>> +#define ASPEED_ADC_REG_ADC04 0x04
>>> +#define ASPEED_ADC_REG_ADC08 0x08
>>> +#define ASPEED_ADC_REG_ADC0C 0x0C
>>> +#define ASPEED_ADC_REG_ADC10 0x10
>>> +#define ASPEED_ADC_REG_ADC14 0x14
>>> +#define ASPEED_ADC_REG_ADC18 0x18
>>> +#define ASPEED_ADC_REG_ADC1C 0x1C
>>> +#define ASPEED_ADC_REG_ADC20 0x20
>>> +#define ASPEED_ADC_REG_ADC24 0x24
>>> +#define ASPEED_ADC_REG_ADC28 0x28
>>> +#define ASPEED_ADC_REG_ADC2C 0x2C
>>
>>
>> I'm not sure that these defines add any value. I'd either give them
>> names such as "ASPEED_ADC_ENGINE_CTRL". or just use the register
>> offset directly.
>>
>>> +
>>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN   (0x0 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY      (0x1 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL       (0x7 << 1)
>>> +
>>> +#define ASPEED_ADC_ENGINE_ENABLE       BIT(0)
>>> +
>>> +struct aspeed_adc_data {
>>> +       struct device   *dev;
>>> +       void __iomem    *base;
>>> +       struct clk      *pclk;
>>> +       struct mutex    lock;
>>> +       unsigned long   update_interval_ms;
>>> +       u32             enabled_channel_mask;
>>> +       const char*     channel_labels[ASPEED_ADC_NUM_CHANNELS];
>>> +};
>>> +
>>> +static int aspeed_adc_set_adc_clock_frequency(
>>> +               struct aspeed_adc_data *data,
>>> +               unsigned long desired_update_interval_ms)
>>> +{
>>> +       long pclk_hz = clk_get_rate(data->pclk);
>>> +       long adc_combined_divisor;
>>> +       long adc_pre_divisor;
>>> +       long adc_divisor;
>>> +       long adc_clock_control_reg_val;
>>> +        long num_enabled_channels =
>>> hweight32(data->enabled_channel_mask);
>>
>>
>> Some whitespace damage here.
>>
>>> +
>>> +       if (desired_update_interval_ms < 1)
>>> +               return -EINVAL;
>>> +
>>> +       /* From the AST2400 datasheet:
>>
>>
>> Nit: kernel style is to leave a blank line on the first line of
>> multi-line comments:
>>
>>  /*
>>   * Foo
>>   * etc
>>
>>> +        *   adc_period_s = pclk_period_s * 2 * (pre_divisor+1) *
>>> (divisor+1)
>>> +        *
>>> +        *   and
>>> +        *
>>> +        *   sample_period_s = 12 * adc_period_s
>>> +        *
>>> +        * Substitute pclk_period_s = (1 / pclk_hz)
>>> +        *
>>> +        * Solve for (pre-divisor+1) * (divisor+1) as those are settings
>>> we need
>>> +        * to program:
>>> +        *   (pre-divisor+1) * (divisor+1) = (sample_period_s * pclk_hz)
>>> / 24
>>> +        */
>>> +
>>> +       /* Assume PCLK runs at a fairly high frequency so dividing it
>>> first
>>> +        * loses little accuracy.  Also note that the above formulas have
>>> +        * sample_period in seconds while our desired_update_interval is
>>> in
>>> +        * milliseconds, hence the divide by 1000.
>>> +        */
>>> +       adc_combined_divisor = desired_update_interval_ms *
>>> +                       (pclk_hz / 24 / 1000 / num_enabled_channels);
>>> +
>>> +       /* Prefer to use the ADC divisor over the ADC pre-divisor.  ADC
>>> divisor
>>> +        * is 10-bits wide so anything over 1024 must use the
>>> pre-divisor.
>>> +        */
>>> +       adc_pre_divisor = 1;
>>> +       while (adc_combined_divisor/adc_pre_divisor > (1<<10)) {
>>> +               adc_pre_divisor += 1;
>>> +       };
>>> +       adc_divisor = adc_combined_divisor / adc_pre_divisor;
>>> +
>>> +       /* Since ADC divisor and pre-divisor are used in division, the
>>> register
>>> +        * value is implicitly incremented by one before use.  This means
>>> we
>>> +        * need to subtract one from our calculated values above.
>>> +        */
>>> +       adc_pre_divisor -= 1;
>>> +       adc_divisor -= 1;
>>> +
>>> +       adc_clock_control_reg_val = (adc_pre_divisor << 17) |
>>> adc_divisor;
>>> +
>>> +       mutex_lock(&data->lock);
>>> +       data->update_interval_ms =
>>> +                       (adc_pre_divisor + 1) * (adc_divisor + 1)
>>> +                       / (pclk_hz / 24 / 1000);
>>> +       writel(adc_clock_control_reg_val, data->base +
>>> ASPEED_ADC_REG_ADC0C);
>>> +       mutex_unlock(&data->lock);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int aspeed_adc_get_channel_reading(struct aspeed_adc_data *data,
>>> +                                               int channel, long *val)
>>> +{
>>> +       u32 data_reg;
>>> +       u32 data_reg_val;
>>> +       long adc_val;
>>> +
>>> +       /* Each 32-bit data register contains 2 10-bit ADC values. */
>>> +       data_reg = ASPEED_ADC_REG_ADC10 + (channel / 2) * 4;
>>> +       data_reg_val = readl(data->base + data_reg);
>>> +       if (channel % 2 == 0) {
>>> +               adc_val = data_reg_val & 0x3FF;
>>> +       } else {
>>> +               adc_val = (data_reg_val >> 16) & 0x3FF;
>>> +       }
>>
>>
>> I wonder if you could replace the above block with:
>>
>>             adc_val = readw(data->base + ASPEED_ADC_REG_ADC10 + channel);
>>
>>> +
>>> +       /* Scale 10-bit input reading to millivolts. */
>>> +       *val = adc_val * ASPEED_ADC_REF_VOLTAGE / 1024;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static umode_t aspeed_adc_hwmon_is_visible(const void *_data,
>>> +                                               enum hwmon_sensor_types
>>> type,
>>> +                                               u32 attr, int channel)
>>> +{
>>> +       const struct aspeed_adc_data* data = _data;
>>> +
>>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>>> +               return S_IRUGO | S_IWUSR;
>>> +       } else if (type == hwmon_in) {
>>> +               /* Only channels that are enabled should be visible. */
>>> +               if (channel >= 0 && channel <= ASPEED_ADC_NUM_CHANNELS &&
>>> +                   (data->enabled_channel_mask & BIT(channel))) {
>>> +
>>> +                       /* All enabled channels have an input but labels
>>> are
>>> +                        * optional in the device tree.
>>> +                        */
>>> +                       if (attr == hwmon_in_input) {
>>> +                               return S_IRUGO;
>>> +                       } else if (attr == hwmon_in_label &&
>>> +                                       data->channel_labels[channel] !=
>>> NULL) {
>>> +                               return S_IRUGO;
>>> +                       }
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int aspeed_adc_hwmon_read(struct device *dev, enum
>>> hwmon_sensor_types type,
>>> +                        u32 attr, int channel, long *val)
>>> +{
>>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>>> +
>>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>>> +               *val = data->update_interval_ms;
>>> +               return 0;
>>> +       } else if (type == hwmon_in && attr == hwmon_in_input) {
>>> +               return aspeed_adc_get_channel_reading(data, channel,
>>> val);
>>> +       }
>>> +
>>> +       return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int aspeed_adc_hwmon_read_string(struct device *dev,
>>> +                                       enum hwmon_sensor_types type,
>>> +                                       u32 attr, int channel, char
>>> **str)
>>> +{
>>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>>> +
>>> +       if (type == hwmon_in && attr == hwmon_in_label) {
>>> +               *str = (char*)data->channel_labels[channel];
>>> +               return 0;
>>> +       }
>>> +
>>> +       return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int aspeed_adc_hwmon_write(struct device *dev, enum
>>> hwmon_sensor_types type,
>>> +                         u32 attr, int channel, long val)
>>> +{
>>> +       struct aspeed_adc_data *data = dev_get_drvdata(dev);
>>> +
>>> +       if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>>> +               return aspeed_adc_set_adc_clock_frequency(data, val);
>>> +       }
>>> +
>>> +       return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static const struct hwmon_ops aspeed_adc_hwmon_ops = {
>>> +       .is_visible = aspeed_adc_hwmon_is_visible,
>>> +       .read = aspeed_adc_hwmon_read,
>>> +       .read_string = aspeed_adc_hwmon_read_string,
>>> +       .write = aspeed_adc_hwmon_write,
>>> +};
>>> +
>>> +static const u32 aspeed_adc_hwmon_chip_config[] = {
>>> +       HWMON_C_UPDATE_INTERVAL,
>>> +       0
>>> +};
>>> +
>>> +static const struct hwmon_channel_info aspeed_adc_hwmon_chip_channel = {
>>> +       .type = hwmon_chip,
>>> +       .config = aspeed_adc_hwmon_chip_config,
>>> +};
>>> +
>>> +static const u32 aspeed_adc_hwmon_in_config[] = {
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       HWMON_I_INPUT | HWMON_I_LABEL,
>>> +       0
>>> +};
>>> +
>>> +static const struct hwmon_channel_info aspeed_adc_hwmon_in_channel = {
>>> +       .type = hwmon_in,
>>> +       .config = aspeed_adc_hwmon_in_config,
>>> +};
>>> +
>>> +static const struct hwmon_channel_info *aspeed_adc_hwmon_channel_info[]
>>> = {
>>> +       &aspeed_adc_hwmon_chip_channel,
>>> +       &aspeed_adc_hwmon_in_channel,
>>> +       NULL
>>> +};
>>> +
>>> +static const struct hwmon_chip_info aspeed_adc_hwmon_chip_info = {
>>> +       .ops = &aspeed_adc_hwmon_ops,
>>> +       .info = aspeed_adc_hwmon_channel_info,
>>> +};
>>> +
>>> +static int aspeed_adc_probe(struct platform_device *pdev)
>>> +{
>>> +       struct aspeed_adc_data *data;
>>> +       struct resource *res;
>>> +       int ret;
>>> +       struct device *hwmon_dev;
>>> +       u32 desired_update_interval_ms;
>>> +       u32 adc_engine_control_reg_val;
>>> +       struct device_node* node;
>>> +
>>> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +       if (!data)
>>> +               return -ENOMEM;
>>> +
>>> +       data->pclk = devm_clk_get(&pdev->dev, NULL);
>>> +       if (IS_ERR(data->pclk)) {
>>> +               dev_err(&pdev->dev, "clk_get failed\n");
>>> +               return PTR_ERR(data->pclk);
>>> +       }
>>> +
>>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +       data->base = devm_ioremap_resource(&pdev->dev, res);
>>> +       if (IS_ERR(data->base))
>>> +               return PTR_ERR(data->base);
>>> +
>>> +       ret = of_property_read_u32(pdev->dev.of_node,
>>> +                       "update-interval-ms",
>>> &desired_update_interval_ms);
>>> +       if (ret < 0 || desired_update_interval_ms == 0) {
>>> +               dev_err(&pdev->dev,
>>> +                               "Missing or zero update-interval-ms
>>> property, "
>>> +                               "defaulting to 100ms\n");
>>
>>
>> Put the string on one line so it can be easily searched for.
>>
>>> +               desired_update_interval_ms = 100;
>>> +       }
>>> +
>>> +       mutex_init(&data->lock);
>>> +
>>> +       ret = clk_prepare_enable(data->pclk);
>>> +       if (ret) {
>>> +               dev_err(&pdev->dev, "failed to enable clock\n");
>>> +               mutex_destroy(&data->lock);
>>> +                return ret;
>>
>>
>> Strange whitespace here.
>>
>>> +       }
>>> +
>>> +       /* Figure out which channels are marked available in the device
>>> tree. */
>>> +       data->enabled_channel_mask = 0;
>>> +       for_each_available_child_of_node(pdev->dev.of_node, node) {
>>> +               u32 pval;
>>> +               unsigned int channel;
>>> +
>>> +               if (of_property_read_u32(node, "reg", &pval)) {
>>> +                       dev_err(&pdev->dev, "invalid reg on %s\n",
>>> +                               node->full_name);
>>> +                       continue;
>>> +               }
>>> +
>>> +               channel = pval;
>>> +               if (channel >= ASPEED_ADC_NUM_CHANNELS) {
>>> +                       dev_err(&pdev->dev,
>>> +                               "invalid channel index %d on %s\n",
>>> +                               channel, node->full_name);
>>> +                       continue;
>>> +               }
>>> +
>>> +               data->enabled_channel_mask |= BIT(channel);
>>> +
>>> +               /* Cache a pointer to the label if one is specified.
>>> Ignore any
>>> +                * errors as the label property is optional.
>>> +                */
>>> +               of_property_read_string(node, "label",
>>> &data->channel_labels[channel]);
>>
>>
>> I was wondering how long we could keep that pointer around. I think we
>> are ok for the lifetime of the driver, as we hold a reference to the
>> node in dev.of_node.
>>
>>> +       }
>>> +
>>> +       platform_set_drvdata(pdev, data);
>>> +       aspeed_adc_set_adc_clock_frequency(data,
>>> desired_update_interval_ms);
>>
>>
>> This reads funny. aspeed_adc_set_clock_frequency instead?
>>
>>> +
>>> +       /* Start all the requested channels in normal mode. */
>>> +       adc_engine_control_reg_val = (data->enabled_channel_mask << 16) |
>>> +               ASPEED_ADC_OPERATION_MODE_NORMAL |
>>> ASPEED_ADC_ENGINE_ENABLE;
>>> +       writel(adc_engine_control_reg_val, data->base +
>>> ASPEED_ADC_REG_ADC00);
>>> +
>>> +       /* Register sysfs hooks */
>>> +       hwmon_dev = devm_hwmon_device_register_with_info(
>>> +                       &pdev->dev, "aspeed_adc", data,
>>> +                       &aspeed_adc_hwmon_chip_info, NULL);
>>> +       if (IS_ERR(hwmon_dev)) {
>>> +               clk_disable_unprepare(data->pclk);
>>> +               mutex_destroy(&data->lock);
>>> +               return PTR_ERR(hwmon_dev);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int aspeed_adc_remove(struct platform_device *pdev) {
>>> +       struct aspeed_adc_data *data = dev_get_drvdata(&pdev->dev);
>>> +       clk_disable_unprepare(data->pclk);
>>> +       mutex_destroy(&data->lock);
>>> +       return 0;
>>> +}
>>> +
>>> +const struct of_device_id aspeed_adc_matches[] = {
>>> +       { .compatible = "aspeed,ast2400-adc" },
>>> +       { .compatible = "aspeed,ast2500-adc" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>>> +
>>> +static struct platform_driver aspeed_adc_driver = {
>>> +       .probe = aspeed_adc_probe,
>>> +       .remove = aspeed_adc_remove,
>>> +       .driver = {
>>> +               .name = KBUILD_MODNAME,
>>> +               .of_match_table = aspeed_adc_matches,
>>> +       }
>>> +};
>>> +
>>> +module_platform_driver(aspeed_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.11.0.483.g087da7b7c-goog
>>>
>> --
>> 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
>>
>

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Guenter Roeck]

On 03/01/2017 07:29 PM, Rick Altherr wrote:
> Resending in plain text.
>
> On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 02/28/2017 04:49 PM, Joel Stanley wrote:
>>>
>>> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
>>>>
>>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
>>>> driver implements reading the ADC values, enabling channels via device
>>>> tree, and optionally providing channel labels via device tree.  Low and
>>>> high threshold interrupts are supported by the hardware but not
>>>> implemented.
>>>>
>>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>>
>>>
>>> Looks good. Some minor comments below.
>>>
>>> Is there a reason you wrote a hwmon driver instead of an iio driver? I
>>> wasn't sure what the recommended subsystem is.
>>
>>
>> Excellent point. Question is really if there is a plan to add support for
>> thresholds. If not, an iio driver might be more appropriate.
>>
>> Guenter
>>
>
> The hardware supports firing interrupts on high and low thresholds.
> I'm not planning to use that feature yet so I didn't implement it.
> Would you prefer that I leave it as is (maybe with a TODO), implement
> the thresholding, or change to iio?
>

Let's try to determine the intended use of the ADC. I don't find the datasheet
online; all I can find is brief summaries which don't me tell much, but suggest
that it is a general purpose ADC and not specifically intended for hardware
monitoring. What is the minimum sampling rate ? That should give us an idea.
If it is in the uS range, iio would be more appropriate (and the iio-hwmon
bridge could be used if a channel is in fact used for hardware monitoring).

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Rick Altherr]

On Sun, Mar 5, 2017 at 8:28 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/01/2017 07:29 PM, Rick Altherr wrote:
>>
>> Resending in plain text.
>>
>> On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 02/28/2017 04:49 PM, Joel Stanley wrote:
>>>>
>>>>
>>>> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
>>>>> driver implements reading the ADC values, enabling channels via device
>>>>> tree, and optionally providing channel labels via device tree.  Low and
>>>>> high threshold interrupts are supported by the hardware but not
>>>>> implemented.
>>>>>
>>>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>>>
>>>>
>>>>
>>>> Looks good. Some minor comments below.
>>>>
>>>> Is there a reason you wrote a hwmon driver instead of an iio driver? I
>>>> wasn't sure what the recommended subsystem is.
>>>
>>>
>>>
>>> Excellent point. Question is really if there is a plan to add support for
>>> thresholds. If not, an iio driver might be more appropriate.
>>>
>>> Guenter
>>>
>>
>> The hardware supports firing interrupts on high and low thresholds.
>> I'm not planning to use that feature yet so I didn't implement it.
>> Would you prefer that I leave it as is (maybe with a TODO), implement
>> the thresholding, or change to iio?
>>
>
> Let's try to determine the intended use of the ADC. I don't find the
> datasheet
> online; all I can find is brief summaries which don't me tell much, but
> suggest
> that it is a general purpose ADC and not specifically intended for hardware
> monitoring. What is the minimum sampling rate ? That should give us an idea.
> If it is in the uS range, iio would be more appropriate (and the iio-hwmon
> bridge could be used if a channel is in fact used for hardware monitoring).
>
> Thanks,
> Guenter
>

AST2500 is a BMC SoC from Aspeed
(https://www.aspeedtech.com/products.php?fPath=20&rId=440).  The BMC
is a separate, always-on computer that manages the health and remote
management for a server.  The driver I sent is for the ADCs included
in the SoC that are intended to monitor power rails on the server
motherboard but are really just general-purpose ADCs.  According to
the (not public) datasheet, the sampling rate is 10-500kHz, resolution
is 10-bit, and precision +/- 2%.  This is my first time writing a
linux driver for ADCs.  My cursory look at iio indicates that that
will work fine for this driver and the hwmon-iio bridge will suffice
for the userspace stack which is currently expecting hwmon APIs.

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Rob Herring]

On Tue, Feb 28, 2017 at 07:45:23PM -0800, Guenter Roeck wrote:
> On 02/28/2017 04:49 PM, Joel Stanley wrote:
> > On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
> > > Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
> > > driver implements reading the ADC values, enabling channels via device
> > > tree, and optionally providing channel labels via device tree.  Low and
> > > high threshold interrupts are supported by the hardware but not
> > > implemented.
> > > 
> > > Signed-off-by: Rick Altherr <raltherr@google.com>
> > 
> > Looks good. Some minor comments below.
> > 
> > Is there a reason you wrote a hwmon driver instead of an iio driver? I
> > wasn't sure what the recommended subsystem is.
> 
> Excellent point. Question is really if there is a plan to add support for
> thresholds. If not, an iio driver might be more appropriate.

Sigh. We have ADCs in 2 places? Fine for the kernel I guess, but not 
bindings.

Rob

Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

[Guenter Roeck]

On 03/02/2017 10:21 PM, Rob Herring wrote:
> On Tue, Feb 28, 2017 at 07:45:23PM -0800, Guenter Roeck wrote:
>> On 02/28/2017 04:49 PM, Joel Stanley wrote:
>>> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
>>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
>>>> driver implements reading the ADC values, enabling channels via device
>>>> tree, and optionally providing channel labels via device tree.  Low and
>>>> high threshold interrupts are supported by the hardware but not
>>>> implemented.
>>>>
>>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>>
>>> Looks good. Some minor comments below.
>>>
>>> Is there a reason you wrote a hwmon driver instead of an iio driver? I
>>> wasn't sure what the recommended subsystem is.
>>
>> Excellent point. Question is really if there is a plan to add support for
>> thresholds. If not, an iio driver might be more appropriate.
>
> Sigh. We have ADCs in 2 places? Fine for the kernel I guess, but not
> bindings.
>

Almost every {voltage, current, power, temperature} sensor chip is implemented
as ADC. Given that, we have (at least) three places. hwmon, iio, thermal.

Guenter

Re: [PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC

[Joel Stanley]

On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@google.com> wrote:
> Signed-off-by: Rick Altherr <raltherr@google.com>

Acked-by: Joel Stanley <joel@jms.id.au>

> ---
>  .../devicetree/bindings/hwmon/aspeed_adc.txt       | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
> new file mode 100644
> index 000000000000..9e481668c4d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
> @@ -0,0 +1,48 @@
> +Aspeed AST2400/2500 ADC
> +
> +This device is a 10-bit converter for 16 voltage channels.  All inputs are
> +single ended.  Each channel can be individually enabled to allow for use of
> +alternate pin functions.
> +
> +1) adc node
> +
> +  Required properties:
> +  - compatible : Should be one of
> +       "aspeed,ast2400-adc"
> +       "aspeed,ast2500-adc"
> +  - reg : memory window mapping address and length
> +  - #address-cells : must be <1> corresponding to the channel child binding
> +  - #size-cells : must be <0> corresponding to the channel child binding
> +  - clocks : Input clock used to derive the sample clock. Expected to be the
> +    SoC's APB clock.
> +  - update-interval-ms : initial time between updates on a channel
> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example adc node:
> +    adc@1e6e9000 {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       compatible = "aspeed,ast2400-adc";
> +       reg = <0x1e6e9000 0xB0>;
> +       clocks = <&clk_apb>;
> +       update-interval-ms = <100>;
> +
> +       [ child node definitions... ]
> +    };
> +
> +2) channel nodes
> +
> +  Optional properties:
> +  - status: indicates the operational status of the device.
> +    Value must be either "disabled" or "okay".
> +  - label : string describing the monitored value
> +
> +  Example channel node:
> +  channel@1 {
> +       status = "okay";
> +       label = "3V3 rail";
> +
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_adc0_default>;
> +  };
> --
> 2.11.0.483.g087da7b7c-goog
>

Re: [PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC

[Rob Herring]

 On Tue, Feb 28, 2017 at 12:14:03PM -0800, Rick Altherr wrote:
> Signed-off-by: Rick Altherr <raltherr@google.com>
> ---
>  .../devicetree/bindings/hwmon/aspeed_adc.txt       | 48 ++++++++++++++++++++++

ADCs should really be documented in one place regardless of whether 
hwmon or IIO is used. Don't need to move it now, but certainly the 
bindings need to be compatible.

>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
> new file mode 100644
> index 000000000000..9e481668c4d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
> @@ -0,0 +1,48 @@
> +Aspeed AST2400/2500 ADC
> +
> +This device is a 10-bit converter for 16 voltage channels.  All inputs are
> +single ended.  Each channel can be individually enabled to allow for use of
> +alternate pin functions.
> +
> +1) adc node
> +
> +  Required properties:
> +  - compatible : Should be one of
> +	"aspeed,ast2400-adc"
> +	"aspeed,ast2500-adc"
> +  - reg : memory window mapping address and length
> +  - #address-cells : must be <1> corresponding to the channel child binding
> +  - #size-cells : must be <0> corresponding to the channel child binding
> +  - clocks : Input clock used to derive the sample clock. Expected to be the
> +    SoC's APB clock.
> +  - update-interval-ms : initial time between updates on a channel

This is like sampling rate? I think we have a standard ADC property for 
that.

> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example adc node:
> +    adc@1e6e9000 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "aspeed,ast2400-adc";
> +	reg = <0x1e6e9000 0xB0>;
> +	clocks = <&clk_apb>;
> +	update-interval-ms = <100>;
> +
> +	[ child node definitions... ]
> +    };
> +
> +2) channel nodes
> +
> +  Optional properties:
> +  - status: indicates the operational status of the device.
> +    Value must be either "disabled" or "okay".

Don't need to document this.

> +  - label : string describing the monitored value

You need a reg property for the channel number.

> +
> +  Example channel node:
> +  channel@1 {
> +	status = "okay";
> +	label = "3V3 rail";
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_adc0_default>;

Need to document these.

> +  };
> -- 
> 2.11.0.483.g087da7b7c-goog
> 

Re: [PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC

[Rick Altherr]

On Thu, Mar 2, 2017 at 10:21 PM, Rob Herring <robh@kernel.org> wrote:
>  On Tue, Feb 28, 2017 at 12:14:03PM -0800, Rick Altherr wrote:
>> Signed-off-by: Rick Altherr <raltherr@google.com>
>> ---
>>  .../devicetree/bindings/hwmon/aspeed_adc.txt       | 48 ++++++++++++++++++++++
>
> ADCs should really be documented in one place regardless of whether
> hwmon or IIO is used. Don't need to move it now, but certainly the
> bindings need to be compatible.
>

hwmon maintainers suggested IIO as well.  Looks like there is an IIO
to hwmon bridge so no concerns there.  I think IIO looks like a
plausible framework for this part based on my cursory look at the IIO
docs.  I'll be working on v2 as an IIO driver.

>>  1 file changed, 48 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>> new file mode 100644
>> index 000000000000..9e481668c4d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>> @@ -0,0 +1,48 @@
>> +Aspeed AST2400/2500 ADC
>> +
>> +This device is a 10-bit converter for 16 voltage channels.  All inputs are
>> +single ended.  Each channel can be individually enabled to allow for use of
>> +alternate pin functions.
>> +
>> +1) adc node
>> +
>> +  Required properties:
>> +  - compatible : Should be one of
>> +     "aspeed,ast2400-adc"
>> +     "aspeed,ast2500-adc"
>> +  - reg : memory window mapping address and length
>> +  - #address-cells : must be <1> corresponding to the channel child binding
>> +  - #size-cells : must be <0> corresponding to the channel child binding
>> +  - clocks : Input clock used to derive the sample clock. Expected to be the
>> +    SoC's APB clock.
>> +  - update-interval-ms : initial time between updates on a channel
>
> This is like sampling rate? I think we have a standard ADC property for
> that.
>

Will look.

>> +
>> +  The node contains child nodes for each channel that the platform uses.
>> +
>> +  Example adc node:
>> +    adc@1e6e9000 {
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     compatible = "aspeed,ast2400-adc";
>> +     reg = <0x1e6e9000 0xB0>;
>> +     clocks = <&clk_apb>;
>> +     update-interval-ms = <100>;
>> +
>> +     [ child node definitions... ]
>> +    };
>> +
>> +2) channel nodes
>> +
>> +  Optional properties:
>> +  - status: indicates the operational status of the device.
>> +    Value must be either "disabled" or "okay".
>
> Don't need to document this.
>

OK

>> +  - label : string describing the monitored value
>
> You need a reg property for the channel number.
>

Missed that when I fixed the code.  Will include in next revision.

>> +
>> +  Example channel node:
>> +  channel@1 {
>> +     status = "okay";
>> +     label = "3V3 rail";
>> +
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_adc0_default>;
>
> Need to document these.

These are described in the pinctrl bindings.  I only put them in the
example as they will be commonly used together but the driver does not
use them.

>
>> +  };
>> --
>> 2.11.0.483.g087da7b7c-goog
>>