Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC
From: Guenter Roeck
Date: Wed Mar 01 2017 - 05:04:32 EST
On 02/28/2017 04:49 PM, Joel Stanley wrote:
On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>");
+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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html