| Message ID | 20170228201404.32125-2-raltherr@google.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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 > -- 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
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 > -- 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
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 >> > -- 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
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 -- 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
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 -- 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
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 -- 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
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. -- 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
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");
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