From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 11:11:54 -0800 Message-ID: Subject: Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC Content-Type: multipart/alternative; boundary=94eb2c05a380f744660549b01511 To: Guenter Roeck Cc: Mark Rutland , Rob Herring , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Joel Stanley , linux-hwmon@vger.kernel.org, jdelvare@suse.com List-ID: --94eb2c05a380f744660549b01511 Content-Type: text/plain; charset=UTF-8 On Feb 28, 2017 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_channe >> l_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 > > --94eb2c05a380f744660549b01511 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Feb 28, 2017 7:45 PM, "Guenter Roeck" <linux@roeck-us.net> wrote:
On 02/28/201= 7 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.=C2=A0 Low an= d
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 thresh= olds. I'm not planning to use that feature yet so I didn't implemen= t it. Would you prefer that I leave it as is (maybe with a TODO), implement= the thresholding, or change to iio?

Rick



---
=C2=A0drivers/hwmon/Kconfig=C2=A0 =C2=A0 =C2=A0 |=C2=A0 10 ++
=C2=A0drivers/hwmon/Makefile=C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A01 +
=C2=A0drivers/hwmon/aspeed_adc.c | 383 +++++++++++++++++++++++++++++++= ++++++++++++++
=C2=A03 files changed, 394 insertions(+)
=C2=A0create 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
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 This driver can also be built as a modul= e.=C2=A0 If so, the module
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 will be called asc7621.

+config SENSORS_ASPEED_ADC
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tristate "Aspeed AST2400/AST2500 ADC"=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0depends on ARCH_ASPEED

depends on ARCH_ASPEED || COMPILE_TEST.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0help
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0If you say yes here you get support for = the Aspeed AST2400/AST2500
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ADC.
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This driver can also be built as a modul= e.=C2=A0 If so, the module
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0will be called aspeed_adc.
+
=C2=A0config SENSORS_K8TEMP
=C2=A0 =C2=A0 =C2=A0 =C2=A0 tristate "AMD Athlon64/FX or Opteron tempe= rature sensor"
=C2=A0 =C2=A0 =C2=A0 =C2=A0 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) +=3D adt7475.o
=C2=A0obj-$(CONFIG_SENSORS_APPLESMC) +=3D applesmc.o
=C2=A0obj-$(CONFIG_SENSORS_ARM_SCPI) +=3D scpi-hwmon.o
=C2=A0obj-$(CONFIG_SENSORS_ASC7621)=C2=A0 +=3D asc7621.o
+obj-$(CONFIG_SENSORS_ASPEED_ADC) +=3D aspeed_adc.o
=C2=A0obj-$(CONFIG_SENSORS_ATXP1)=C2=A0 =C2=A0 +=3D atxp1.o
=C2=A0obj-$(CONFIG_SENSORS_CORETEMP) +=3D coretemp.o
=C2=A0obj-$(CONFIG_SENSORS_DA9052_ADC)+=3D 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=C2=A0 =C2=A0 =C2=A0 =C2=A0 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<= br> offset directly.

+
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN=C2=A0 =C2=A0(0x0 <<= ; 1)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY=C2=A0 =C2=A0 =C2=A0 (0x1 &l= t;< 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL=C2=A0 =C2=A0 =C2=A0 =C2=A0(0= x7 << 1)
+
+#define ASPEED_ADC_ENGINE_ENABLE=C2=A0 =C2=A0 =C2=A0 =C2=A0BIT(0)
+
+struct aspeed_adc_data {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct device=C2=A0 =C2=A0*dev;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0void __iomem=C2=A0 =C2=A0 *base;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct clk=C2=A0 =C2=A0 =C2=A0 *pclk;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct mutex=C2=A0 =C2=A0 lock;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long=C2=A0 =C2=A0update_interval_ms; +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0enabled_channel_mask;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0const char*=C2=A0 =C2=A0 =C2=A0channel_labels[A= SPEED_ADC_NUM_CHANNELS];
+};
+
+static int aspeed_adc_set_adc_clock_frequency(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_d= ata *data,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long desir= ed_update_interval_ms)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0long pclk_hz =3D clk_get_rate(data->pclk); +=C2=A0 =C2=A0 =C2=A0 =C2=A0long adc_combined_divisor;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0long adc_pre_divisor;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0long adc_divisor;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0long adc_clock_control_reg_val;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 long num_enabled_channels =3D hweight32(data-&= gt;enabled_channel_mask);

Some whitespace damage here.

+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (desired_update_interval_ms < 1)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* From the AST2400 datasheet:

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

=C2=A0/*
=C2=A0 * Foo
=C2=A0 * etc

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0adc_period_s =3D pclk_period_s *= 2 * (pre_divisor+1) * (divisor+1)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0and
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0sample_period_s =3D 12 * adc_per= iod_s
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Substitute pclk_period_s =3D (1 / pclk_hz) +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Solve for (pre-divisor+1) * (divisor+1) as t= hose are settings we need
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * to program:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0(pre-divisor+1) * (divisor+1) = =3D (sample_period_s * pclk_hz) / 24
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Assume PCLK runs at a fairly high frequency = so dividing it first
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * loses little accuracy.=C2=A0 Also note that = the above formulas have
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * sample_period in seconds while our desired_u= pdate_interval is in
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * milliseconds, hence the divide by 1000.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_combined_divisor =3D desired_update_interva= l_ms *
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0(pclk_hz / 24 / 1000 / num_enabled_channels);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Prefer to use the ADC divisor over the ADC p= re-divisor.=C2=A0 ADC divisor
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * is 10-bits wide so anything over 1024 must u= se the pre-divisor.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_pre_divisor =3D 1;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0while (adc_combined_divisor/adc_pre_diviso= r > (1<<10)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0adc_pre_divisor += =3D 1;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0};
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_divisor =3D adc_combined_divisor / adc_pre_= divisor;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Since ADC divisor and pre-divisor are used i= n division, the register
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * value is implicitly incremented by one befor= e use.=C2=A0 This means we
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * need to subtract one from our calculated val= ues above.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_pre_divisor -=3D 1;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_divisor -=3D 1;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_clock_control_reg_val =3D (adc_pre_divisor = << 17) | adc_divisor;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_lock(&data->lock);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data->update_interval_ms =3D
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0(adc_pre_divisor + 1) * (adc_divisor + 1)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/ (pclk_hz / 24 / 1000);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0writel(adc_clock_control_reg_val, data->= ;base + ASPEED_ADC_REG_ADC0C);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_unlock(&data->lock);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+static int aspeed_adc_get_channel_reading(struct aspeed_adc_data *dat= a,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0int channel, long *val)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 data_reg;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 data_reg_val;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0long adc_val;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Each 32-bit data register contains 2 10-bit = ADC values. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data_reg =3D ASPEED_ADC_REG_ADC10 + (channel / = 2) * 4;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data_reg_val =3D readl(data->base + data_reg= );
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (channel % 2 =3D=3D 0) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0adc_val =3D data_re= g_val & 0x3FF;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0adc_val =3D (data_r= eg_val >> 16) & 0x3FF;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}

I wonder if you could replace the above block with:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 adc_val =3D readw(data->base += ASPEED_ADC_REG_ADC10 + channel);

+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Scale 10-bit input reading to millivolts. */=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0*val =3D adc_val * ASPEED_ADC_REF_VOLTAGE / 102= 4;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+static umode_t aspeed_adc_hwmon_is_visible(const void *_data,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0enum hwmon_sensor_types type,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0u32 attr, int channel)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct aspeed_adc_data* data =3D _data; +
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (type =3D=3D hwmon_chip && attr =3D= =3D hwmon_chip_update_interval) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return S_IRUGO | S_= IWUSR;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (type =3D=3D hwmon_in) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Only channels th= at are enabled should be visible. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (channel >=3D= 0 && channel <=3D ASPEED_ADC_NUM_CHANNELS &&
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(data= ->enabled_channel_mask & BIT(channel))) {
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/* All enabled channels have an input but labels are
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 * optional in the device tree.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0if (attr =3D=3D hwmon_in_input) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return S_IRUGO;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0} else if (attr =3D=3D hwmon_in_label &&
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0data->= channel_labels[channel] !=3D NULL) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return S_IRUGO;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0}
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+static int aspeed_adc_hwmon_read(struct device *dev, enum hwmon_sensor_typ= es type,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 u32 attr, int channel, long *val)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_data *data =3D dev_get_drvdat= a(dev);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (type =3D=3D hwmon_chip && attr =3D= =3D hwmon_chip_update_interval) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*val =3D data->u= pdate_interval_ms;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (type =3D=3D hwmon_in && attr= =3D=3D hwmon_in_input) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return aspeed_adc_g= et_channel_reading(data, channel, val);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return -EOPNOTSUPP;
+}
+
+static int aspeed_adc_hwmon_read_string(struct device *dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0enum hwmo= n_sensor_types type,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 attr,= int channel, char **str)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_data *data =3D dev_get_drvdat= a(dev);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (type =3D=3D hwmon_in && attr =3D=3D= hwmon_in_label) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*str =3D (char*)dat= a->channel_labels[channel];
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return -EOPNOTSUPP;
+}
+
+static int aspeed_adc_hwmon_write(struct device *dev, enum hwmon_sensor_ty= pes type,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0u32 attr, int channel, long val)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_data *data =3D dev_get_drvdat= a(dev);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (type =3D=3D hwmon_chip && attr =3D= =3D hwmon_chip_update_interval) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return aspeed_adc_s= et_adc_clock_frequency(data, val);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops aspeed_adc_hwmon_ops =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.is_visible =3D aspeed_adc_hwmon_is_visible, +=C2=A0 =C2=A0 =C2=A0 =C2=A0.read =3D aspeed_adc_hwmon_read,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.read_string =3D aspeed_adc_hwmon_read_string,<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0.write =3D aspeed_adc_hwmon_write,
+};
+
+static const u32 aspeed_adc_hwmon_chip_config[] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_C_UPDATE_INTERVAL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A00
+};
+
+static const struct hwmon_channel_info aspeed_adc_hwmon_chip_channel =3D {=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.type =3D hwmon_chip,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.config =3D aspeed_adc_hwmon_chip_config,
+};
+
+static const u32 aspeed_adc_hwmon_in_config[] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0HWMON_I_INPUT | HWMON_I_LABEL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A00
+};
+
+static const struct hwmon_channel_info aspeed_adc_hwmon_in_channel =3D { +=C2=A0 =C2=A0 =C2=A0 =C2=A0.type =3D hwmon_in,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.config =3D aspeed_adc_hwmon_in_config,
+};
+
+static const struct hwmon_channel_info *aspeed_adc_hwmon_channel_info= [] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0&aspeed_adc_hwmon_chip_channel,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0&aspeed_adc_hwmon_in_channel,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0NULL
+};
+
+static const struct hwmon_chip_info aspeed_adc_hwmon_chip_info =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.ops =3D &aspeed_adc_hwmon_ops,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.info =3D aspeed_adc_hwmon_channel_info,
+};
+
+static int aspeed_adc_probe(struct platform_device *pdev)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_data *data;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct resource *res;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct device *hwmon_dev;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 desired_update_interval_ms;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 adc_engine_control_reg_val;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct device_node* node;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data =3D devm_kzalloc(&pdev->dev, sizeof= (*data), GFP_KERNEL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!data)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data->pclk =3D devm_clk_get(&pdev->de= v, NULL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (IS_ERR(data->pclk)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(&pdev-&= gt;dev, "clk_get failed\n");
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(data= ->pclk);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0res =3D platform_get_resource(pdev, IORESOURCE_= MEM, 0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data->base =3D devm_ioremap_resource(&pd= ev->dev, res);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (IS_ERR(data->base))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(data= ->base);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D of_property_read_u32(pdev->dev.= of_node,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0"update-interval-ms", &desired_update_interval_ms);=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0 || desired_update_interval_ms = =3D=3D 0) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(&pdev-&= gt;dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"Missing or zero update-interval= -ms property, "
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"defaulting to 100ms\n");

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

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0desired_update_inte= rval_ms =3D 100;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_init(&data->lock);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D clk_prepare_enable(data->pclk);=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(&pdev-&= gt;dev, "failed to enable clock\n");
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_destroy(&= data->lock);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;

Strange whitespace here.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Figure out which channels are marked availab= le in the device tree. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0data->enabled_channel_mask =3D 0;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0for_each_available_child_of_node(pdev->= dev.of_node, node) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 pval;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int channe= l;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (of_property_rea= d_u32(node, "reg", &pval)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0dev_err(&pdev->dev, "invalid reg on %s\n",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0node->full_name);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0continue;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0channel =3D pval; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (channel >=3D= ASPEED_ADC_NUM_CHANNELS) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0dev_err(&pdev->dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"invalid channel index %d on %s\= n",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0channel, node->full_name);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0continue;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0data->enabled_ch= annel_mask |=3D BIT(channel);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Cache a pointer = to the label if one is specified.=C2=A0 Ignore any
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * errors as the la= bel property is optional.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0of_property_read_st= ring(node, "label", &data->channel_labels[channel]);<= br>

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.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0platform_set_drvdata(pdev, data);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0aspeed_adc_set_adc_clock_frequency(data, d= esired_update_interval_ms);

This reads funny. aspeed_adc_set_clock_frequency instead?

+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Start all the requested channels in normal m= ode. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0adc_engine_control_reg_val =3D (data->enable= d_channel_mask << 16) |
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ASPEED_ADC_OPERATIO= N_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0writel(adc_engine_control_reg_val, data-&g= t;base + ASPEED_ADC_REG_ADC00);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Register sysfs hooks */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0hwmon_dev =3D devm_hwmon_device_register_with_info(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0&pdev->dev, "aspeed_adc", data,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0&aspeed_adc_hwmon_chip_info, NULL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (IS_ERR(hwmon_dev)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0clk_disable_unprepa= re(data->pclk);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_destroy(&= data->lock);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(hwmo= n_dev);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+static int aspeed_adc_remove(struct platform_device *pdev) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct aspeed_adc_data *data =3D dev_get_drvdat= a(&pdev->dev);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0clk_disable_unprepare(data->pclk);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_destroy(&data->lock);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+const struct of_device_id aspeed_adc_matches[] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .compatible =3D "aspeed,ast2400-adc"= ; },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .compatible =3D "aspeed,ast2500-adc"= ; },
+};
+MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
+
+static struct platform_driver aspeed_adc_driver =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.probe =3D aspeed_adc_probe,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.remove =3D aspeed_adc_remove,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0.driver =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.name =3D KBUILD_MO= DNAME,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.of_match_table =3D= aspeed_adc_matches,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+};
+
+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&= quot; in
the body of a message to majordomo@vger.kernel.org
More majordomo info at=C2=A0 http://vger.kernel.org/majord= omo-info.html



--94eb2c05a380f744660549b01511--