From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:60231 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750725AbdCAGHK (ORCPT ); Wed, 1 Mar 2017 01:07:10 -0500 Subject: Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC To: Joel Stanley , Rick Altherr References: <20170228201404.32125-1-raltherr@google.com> <20170228201404.32125-2-raltherr@google.com> Cc: Rob Herring , Mark Rutland , devicetree@vger.kernel.org, jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org From: Guenter Roeck Message-ID: Date: Tue, 28 Feb 2017 19:45:23 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-hwmon-owner@vger.kernel.org List-Id: linux-hwmon@vger.kernel.org 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 > >> --- >> 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 >