Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753802AbdCBD6K (ORCPT ); Wed, 1 Mar 2017 22:58:10 -0500 Received: from mail-io0-f173.google.com ([209.85.223.173]:35756 "EHLO mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753075AbdCBD6I (ORCPT ); Wed, 1 Mar 2017 22:58:08 -0500 MIME-Version: 1.0 In-Reply-To: References: <20170228201404.32125-1-raltherr@google.com> <20170228201404.32125-2-raltherr@google.com> From: Rick Altherr Date: Wed, 1 Mar 2017 19:29:52 -0800 Message-ID: Subject: Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC To: Guenter Roeck Cc: Joel Stanley , Rob Herring , Mark Rutland , devicetree@vger.kernel.org, jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 19299 Lines: 552 Resending in plain text. On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck wrote: > On 02/28/2017 04:49 PM, Joel Stanley wrote: >> >> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr 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 >> >> >> 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 >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#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 "); >>> +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 >> >