Date:   Tue, 28 Feb 2017 19:45:23 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Joel Stanley <joel@....id.au>, Rick Altherr <raltherr@...gle.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, jdelvare@...e.com,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC

On 02/28/2017 04:49 PM, Joel Stanley wrote:
> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr <raltherr@...gle.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@...gle.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@...gle.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Powered by blists - more mailing lists