* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-03-23 18:41 ` [PATCH v4 2/2] iio: " Rick Altherr
@ 2017-03-24 4:03 ` Joel Stanley
2017-03-25 17:32 ` Jonathan Cameron
2017-04-05 20:27 ` Stephen Boyd
2 siblings, 0 replies; 18+ messages in thread
From: Joel Stanley @ 2017-03-24 4:03 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Stephen Boyd, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Matt Ranostay
On Fri, Mar 24, 2017 at 5:11 AM, Rick Altherr <raltherr@google.com> wrote:
> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
> interrupts are supported by the hardware but are not currently implemented.
>
> Signed-off-by: Rick Altherr <raltherr@google.com>
I gave it another test with the hwmon bridge on my ast2500-evb:
root@evb-ast2500:~# sensors
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.30 V
root@evb-ast2500:~# sensors
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.80 V
Does what it says on the box! The changes you made look good too. If
you do respin, please mention that it was tested on an ast2500evb and
whatever hardware you tested it on.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
> ---
>
> Changes in v4:
> - Avoid copying per-model data to per-instance data
>
> Changes in v3:
> - Drop model numbers from description as same IP is used in every generation
> - Remove unused macros
> - Shorten macro prefix to just ASPEED_
> - Remove extraneous whitespace
> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose
> - Use existing macros in place of magic constants
> - Remove unnecessary logging in failure conditions during probe
> - Disable ADC hardware during remove()
> - Add a NULL terminator to of_match_table
> - Add per-model data to accomodate slight variations (vref, min/max sample rate)
> - Added missing Kconfig depends on COMMON_CLK
>
> Changes in v2:
> - Rewritten as an IIO device
> - Renamed register macros to describe the register's purpose
> - Replaced awkward reading of 16-bit data registers with readw()
> - Added Kconfig dependency on COMPILE_TEST
>
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 284 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 296 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..236d05f99e80 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,17 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed ADC"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on COMMON_CLK
> + help
> + If you say yes here you get support for the ADC included in Aspeed
> + BMC SoCs.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called aspeed_adc.
> +
> config AT91_ADC
> tristate "Atmel AT91 ADC"
> depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe399f894..306f10ffca74 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
> obj-$(CONFIG_AD7793) += ad7793.o
> obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> new file mode 100644
> index 000000000000..e051fdf8ecbd
> --- /dev/null
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,284 @@
> +/*
> + * 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/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +
> +#define ASPEED_RESOLUTION_BITS 10
> +#define ASPEED_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_REG_MAX 0xC0
> +
> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ENGINE_ENABLE BIT(0)
> +
> +struct aspeed_adc_model_data {
> + const char *model_name;
> + unsigned int min_sampling_rate; // Hz
> + unsigned int max_sampling_rate; // Hz
> + unsigned int vref_voltage; // mV
> +};
> +
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_CHAN(_idx, _data_reg_addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_data_reg_addr), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +}
> +
> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
> + ASPEED_CHAN(0, 0x10),
> + ASPEED_CHAN(1, 0x12),
> + ASPEED_CHAN(2, 0x14),
> + ASPEED_CHAN(3, 0x16),
> + ASPEED_CHAN(4, 0x18),
> + ASPEED_CHAN(5, 0x1A),
> + ASPEED_CHAN(6, 0x1C),
> + ASPEED_CHAN(7, 0x1E),
> + ASPEED_CHAN(8, 0x20),
> + ASPEED_CHAN(9, 0x22),
> + ASPEED_CHAN(10, 0x24),
> + ASPEED_CHAN(11, 0x26),
> + ASPEED_CHAN(12, 0x28),
> + ASPEED_CHAN(13, 0x2A),
> + ASPEED_CHAN(14, 0x2C),
> + ASPEED_CHAN(15, 0x2E),
> +};
> +
> +static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = model_data->vref_voltage;
> + *val2 = ASPEED_RESOLUTION_BITS;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EPERM;
> + }
> +}
> +
> +static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < model_data->min_sampling_rate ||
> + val > model_data->max_sampling_rate)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EPERM;
> + }
> +}
> +
> +static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + if (!readval || reg % 4 || reg > ASPEED_REG_MAX)
> + return -EINVAL;
> +
> + *readval = readl(data->base + reg);
> + return 0;
> +}
> +
> +static const struct iio_info aspeed_adc_iio_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = aspeed_adc_read_raw,
> + .write_raw = aspeed_adc_write_raw,
> + .debugfs_reg_access = aspeed_adc_reg_access,
> +};
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct aspeed_adc_data *data;
> + const struct aspeed_adc_model_data *model_data;
> + struct resource *res;
> + const char *clk_parent_name;
> + int ret;
> + u32 adc_engine_control_reg_val;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &pdev->dev;
> +
> + 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);
> +
> + /* Register ADC clock prescaler with source specified by device tree. */
> + spin_lock_init(&data->clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + &pdev->dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler))
> + return PTR_ERR(data->clk_prescaler);
> +
> + /*
> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> + * setting to adjust the prescaler as well.
> + */
> + data->clk_scaler = clk_hw_register_divider(
> + &pdev->dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + ret = PTR_ERR(data->clk_scaler);
> + goto scaler_error;
> + }
> +
> + /* Start all channels in normal mode. */
> + clk_prepare_enable(data->clk_scaler->clk);
> + adc_engine_control_reg_val = GENMASK(31, 16) |
> + ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> +
> + model_data = of_device_get_match_data(&pdev->dev);
> + indio_dev->name = model_data->model_name;
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->info = &aspeed_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = aspeed_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto iio_register_error;
> +
> + return 0;
> +
> +iio_register_error:
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> +
> +scaler_error:
> + clk_hw_unregister_divider(data->clk_prescaler);
> + return ret;
> +}
> +
> +static int aspeed_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +static const struct aspeed_adc_model_data ast2400_model_data = {
> + .model_name = "ast2400-adc",
> + .vref_voltage = 2500, // mV
> + .min_sampling_rate = 10000,
> + .max_sampling_rate = 500000,
> +};
> +
> +static const struct aspeed_adc_model_data ast2500_model_data = {
> + .model_name = "ast2500-adc",
> + .vref_voltage = 1800, // mV
> + .min_sampling_rate = 1,
> + .max_sampling_rate = 1000000,
> +};
> +
> +static const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
> + { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> + .probe = aspeed_adc_probe,
> + .remove = aspeed_adc_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_adc_matches,
> + }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.12.1.500.gab5fba24ee-goog
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
@ 2017-03-24 4:03 ` Joel Stanley
0 siblings, 0 replies; 18+ messages in thread
From: Joel Stanley @ 2017-03-24 4:03 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Stephen Boyd, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Matt Ranostay
On Fri, Mar 24, 2017 at 5:11 AM, Rick Altherr <raltherr@google.com> wrote:
> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
> interrupts are supported by the hardware but are not currently implemented.
>
> Signed-off-by: Rick Altherr <raltherr@google.com>
I gave it another test with the hwmon bridge on my ast2500-evb:
root@evb-ast2500:~# sensors
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.30 V
root@evb-ast2500:~# sensors
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.80 V
Does what it says on the box! The changes you made look good too. If
you do respin, please mention that it was tested on an ast2500evb and
whatever hardware you tested it on.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
> ---
>
> Changes in v4:
> - Avoid copying per-model data to per-instance data
>
> Changes in v3:
> - Drop model numbers from description as same IP is used in every generation
> - Remove unused macros
> - Shorten macro prefix to just ASPEED_
> - Remove extraneous whitespace
> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose
> - Use existing macros in place of magic constants
> - Remove unnecessary logging in failure conditions during probe
> - Disable ADC hardware during remove()
> - Add a NULL terminator to of_match_table
> - Add per-model data to accomodate slight variations (vref, min/max sample rate)
> - Added missing Kconfig depends on COMMON_CLK
>
> Changes in v2:
> - Rewritten as an IIO device
> - Renamed register macros to describe the register's purpose
> - Replaced awkward reading of 16-bit data registers with readw()
> - Added Kconfig dependency on COMPILE_TEST
>
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 284 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 296 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..236d05f99e80 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,17 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed ADC"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on COMMON_CLK
> + help
> + If you say yes here you get support for the ADC included in Aspeed
> + BMC SoCs.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called aspeed_adc.
> +
> config AT91_ADC
> tristate "Atmel AT91 ADC"
> depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe399f894..306f10ffca74 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
> obj-$(CONFIG_AD7793) += ad7793.o
> obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> new file mode 100644
> index 000000000000..e051fdf8ecbd
> --- /dev/null
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,284 @@
> +/*
> + * 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/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +
> +#define ASPEED_RESOLUTION_BITS 10
> +#define ASPEED_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_REG_MAX 0xC0
> +
> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ENGINE_ENABLE BIT(0)
> +
> +struct aspeed_adc_model_data {
> + const char *model_name;
> + unsigned int min_sampling_rate; // Hz
> + unsigned int max_sampling_rate; // Hz
> + unsigned int vref_voltage; // mV
> +};
> +
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_CHAN(_idx, _data_reg_addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_data_reg_addr), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +}
> +
> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
> + ASPEED_CHAN(0, 0x10),
> + ASPEED_CHAN(1, 0x12),
> + ASPEED_CHAN(2, 0x14),
> + ASPEED_CHAN(3, 0x16),
> + ASPEED_CHAN(4, 0x18),
> + ASPEED_CHAN(5, 0x1A),
> + ASPEED_CHAN(6, 0x1C),
> + ASPEED_CHAN(7, 0x1E),
> + ASPEED_CHAN(8, 0x20),
> + ASPEED_CHAN(9, 0x22),
> + ASPEED_CHAN(10, 0x24),
> + ASPEED_CHAN(11, 0x26),
> + ASPEED_CHAN(12, 0x28),
> + ASPEED_CHAN(13, 0x2A),
> + ASPEED_CHAN(14, 0x2C),
> + ASPEED_CHAN(15, 0x2E),
> +};
> +
> +static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = model_data->vref_voltage;
> + *val2 = ASPEED_RESOLUTION_BITS;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EPERM;
> + }
> +}
> +
> +static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < model_data->min_sampling_rate ||
> + val > model_data->max_sampling_rate)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EPERM;
> + }
> +}
> +
> +static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + if (!readval || reg % 4 || reg > ASPEED_REG_MAX)
> + return -EINVAL;
> +
> + *readval = readl(data->base + reg);
> + return 0;
> +}
> +
> +static const struct iio_info aspeed_adc_iio_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = aspeed_adc_read_raw,
> + .write_raw = aspeed_adc_write_raw,
> + .debugfs_reg_access = aspeed_adc_reg_access,
> +};
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct aspeed_adc_data *data;
> + const struct aspeed_adc_model_data *model_data;
> + struct resource *res;
> + const char *clk_parent_name;
> + int ret;
> + u32 adc_engine_control_reg_val;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &pdev->dev;
> +
> + 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);
> +
> + /* Register ADC clock prescaler with source specified by device tree. */
> + spin_lock_init(&data->clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + &pdev->dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler))
> + return PTR_ERR(data->clk_prescaler);
> +
> + /*
> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> + * setting to adjust the prescaler as well.
> + */
> + data->clk_scaler = clk_hw_register_divider(
> + &pdev->dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + ret = PTR_ERR(data->clk_scaler);
> + goto scaler_error;
> + }
> +
> + /* Start all channels in normal mode. */
> + clk_prepare_enable(data->clk_scaler->clk);
> + adc_engine_control_reg_val = GENMASK(31, 16) |
> + ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> +
> + model_data = of_device_get_match_data(&pdev->dev);
> + indio_dev->name = model_data->model_name;
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->info = &aspeed_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = aspeed_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto iio_register_error;
> +
> + return 0;
> +
> +iio_register_error:
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> +
> +scaler_error:
> + clk_hw_unregister_divider(data->clk_prescaler);
> + return ret;
> +}
> +
> +static int aspeed_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +static const struct aspeed_adc_model_data ast2400_model_data = {
> + .model_name = "ast2400-adc",
> + .vref_voltage = 2500, // mV
> + .min_sampling_rate = 10000,
> + .max_sampling_rate = 500000,
> +};
> +
> +static const struct aspeed_adc_model_data ast2500_model_data = {
> + .model_name = "ast2500-adc",
> + .vref_voltage = 1800, // mV
> + .min_sampling_rate = 1,
> + .max_sampling_rate = 1000000,
> +};
> +
> +static const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
> + { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> + .probe = aspeed_adc_probe,
> + .remove = aspeed_adc_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_adc_matches,
> + }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.12.1.500.gab5fba24ee-goog
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-03-23 18:41 ` [PATCH v4 2/2] iio: " Rick Altherr
2017-03-24 4:03 ` Joel Stanley
@ 2017-03-25 17:32 ` Jonathan Cameron
2017-03-27 23:05 ` Rick Altherr
2017-04-05 20:27 ` Stephen Boyd
2 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2017-03-25 17:32 UTC (permalink / raw)
To: Rick Altherr, openbmc, linux-kernel
Cc: linux-iio, Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Peter Meerwald-Stadler, Martin Blumenstingl, Michael Turquette,
Rob Herring, Stephen Boyd, Alison Schofield, Fabrice Gasnier,
Hartmut Knaack, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Matt Ranostay
On 23/03/17 18:41, Rick Altherr wrote:
> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
> interrupts are supported by the hardware but are not currently implemented.
>
> Signed-off-by: Rick Altherr <raltherr@google.com>
Nice little driver.
I'd like input from clk maintainers on the clk stuff though before applying.
Michael, Stephen MAINTAINERS suggests this is you two. Any comments?
Thanks,
Jonathan
> ---
>
> Changes in v4:
> - Avoid copying per-model data to per-instance data
>
> Changes in v3:
> - Drop model numbers from description as same IP is used in every generation
> - Remove unused macros
> - Shorten macro prefix to just ASPEED_
> - Remove extraneous whitespace
> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose
> - Use existing macros in place of magic constants
> - Remove unnecessary logging in failure conditions during probe
> - Disable ADC hardware during remove()
> - Add a NULL terminator to of_match_table
> - Add per-model data to accomodate slight variations (vref, min/max sample rate)
> - Added missing Kconfig depends on COMMON_CLK
>
> Changes in v2:
> - Rewritten as an IIO device
> - Renamed register macros to describe the register's purpose
> - Replaced awkward reading of 16-bit data registers with readw()
> - Added Kconfig dependency on COMPILE_TEST
>
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 284 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 296 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..236d05f99e80 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,17 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed ADC"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on COMMON_CLK
> + help
> + If you say yes here you get support for the ADC included in Aspeed
> + BMC SoCs.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called aspeed_adc.
> +
> config AT91_ADC
> tristate "Atmel AT91 ADC"
> depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe399f894..306f10ffca74 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
> obj-$(CONFIG_AD7793) += ad7793.o
> obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> new file mode 100644
> index 000000000000..e051fdf8ecbd
> --- /dev/null
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,284 @@
> +/*
> + * 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/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +
> +#define ASPEED_RESOLUTION_BITS 10
> +#define ASPEED_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_REG_MAX 0xC0
> +
> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ENGINE_ENABLE BIT(0)
> +
> +struct aspeed_adc_model_data {
> + const char *model_name;
> + unsigned int min_sampling_rate; // Hz
> + unsigned int max_sampling_rate; // Hz
> + unsigned int vref_voltage; // mV
> +};
> +
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_CHAN(_idx, _data_reg_addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_data_reg_addr), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +}
> +
> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
> + ASPEED_CHAN(0, 0x10),
> + ASPEED_CHAN(1, 0x12),
> + ASPEED_CHAN(2, 0x14),
> + ASPEED_CHAN(3, 0x16),
> + ASPEED_CHAN(4, 0x18),
> + ASPEED_CHAN(5, 0x1A),
> + ASPEED_CHAN(6, 0x1C),
> + ASPEED_CHAN(7, 0x1E),
> + ASPEED_CHAN(8, 0x20),
> + ASPEED_CHAN(9, 0x22),
> + ASPEED_CHAN(10, 0x24),
> + ASPEED_CHAN(11, 0x26),
> + ASPEED_CHAN(12, 0x28),
> + ASPEED_CHAN(13, 0x2A),
> + ASPEED_CHAN(14, 0x2C),
> + ASPEED_CHAN(15, 0x2E),
> +};
> +
> +static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = model_data->vref_voltage;
> + *val2 = ASPEED_RESOLUTION_BITS;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EPERM;
Unusual, but I supose a valid choice.
> + }
> +}
> +
> +static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> + const struct aspeed_adc_model_data *model_data =
> + of_device_get_match_data(data->dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < model_data->min_sampling_rate ||
> + val > model_data->max_sampling_rate)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EPERM;
> + }
> +}
> +
> +static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + if (!readval || reg % 4 || reg > ASPEED_REG_MAX)
> + return -EINVAL;
> +
> + *readval = readl(data->base + reg);
> + return 0;
> +}
> +
> +static const struct iio_info aspeed_adc_iio_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = aspeed_adc_read_raw,
> + .write_raw = aspeed_adc_write_raw,
> + .debugfs_reg_access = aspeed_adc_reg_access,
> +};
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct aspeed_adc_data *data;
> + const struct aspeed_adc_model_data *model_data;
> + struct resource *res;
> + const char *clk_parent_name;
> + int ret;
> + u32 adc_engine_control_reg_val;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &pdev->dev;
> +
> + 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);
> +
> + /* Register ADC clock prescaler with source specified by device tree. */
> + spin_lock_init(&data->clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + &pdev->dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler))
> + return PTR_ERR(data->clk_prescaler);
> +
> + /*
> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> + * setting to adjust the prescaler as well.
> + */
> + data->clk_scaler = clk_hw_register_divider(
> + &pdev->dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + ret = PTR_ERR(data->clk_scaler);
> + goto scaler_error;
> + }
> +
> + /* Start all channels in normal mode. */
> + clk_prepare_enable(data->clk_scaler->clk);
> + adc_engine_control_reg_val = GENMASK(31, 16) |
> + ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> +
> + model_data = of_device_get_match_data(&pdev->dev);
> + indio_dev->name = model_data->model_name;
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->info = &aspeed_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = aspeed_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto iio_register_error;
> +
> + return 0;
> +
> +iio_register_error:
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> +
> +scaler_error:
> + clk_hw_unregister_divider(data->clk_prescaler);
> + return ret;
> +}
> +
> +static int aspeed_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> + data->base + ASPEED_REG_ENGINE_CONTROL);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +static const struct aspeed_adc_model_data ast2400_model_data = {
> + .model_name = "ast2400-adc",
> + .vref_voltage = 2500, // mV
> + .min_sampling_rate = 10000,
> + .max_sampling_rate = 500000,
> +};
> +
> +static const struct aspeed_adc_model_data ast2500_model_data = {
> + .model_name = "ast2500-adc",
> + .vref_voltage = 1800, // mV
> + .min_sampling_rate = 1,
> + .max_sampling_rate = 1000000,
> +};
> +
> +static const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
> + { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> + .probe = aspeed_adc_probe,
> + .remove = aspeed_adc_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_adc_matches,
> + }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-03-25 17:32 ` Jonathan Cameron
@ 2017-03-27 23:05 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-03-27 23:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Peter Meerwald-Stadler, Martin Blumenstingl, Michael Turquette,
Rob Herring, Stephen Boyd, Alison Schofield, Fabrice Gasnier,
Hartmut Knaack, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Matt Ranostay
On Sat, Mar 25, 2017 at 10:32 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/03/17 18:41, Rick Altherr wrote:
>> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
>> interrupts are supported by the hardware but are not currently implemented.
>>
>> Signed-off-by: Rick Altherr <raltherr@google.com>
> Nice little driver.
>
> I'd like input from clk maintainers on the clk stuff though before applying.
> Michael, Stephen MAINTAINERS suggests this is you two. Any comments?
>
> Thanks,
>
> Jonathan
>
>> ---
>>
>> Changes in v4:
>> - Avoid copying per-model data to per-instance data
>>
>> Changes in v3:
>> - Drop model numbers from description as same IP is used in every generation
>> - Remove unused macros
>> - Shorten macro prefix to just ASPEED_
>> - Remove extraneous whitespace
>> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose
>> - Use existing macros in place of magic constants
>> - Remove unnecessary logging in failure conditions during probe
>> - Disable ADC hardware during remove()
>> - Add a NULL terminator to of_match_table
>> - Add per-model data to accomodate slight variations (vref, min/max sample rate)
>> - Added missing Kconfig depends on COMMON_CLK
>>
>> Changes in v2:
>> - Rewritten as an IIO device
>> - Renamed register macros to describe the register's purpose
>> - Replaced awkward reading of 16-bit data registers with readw()
>> - Added Kconfig dependency on COMPILE_TEST
>>
>> drivers/iio/adc/Kconfig | 11 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/aspeed_adc.c | 284 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 296 insertions(+)
>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 2268a6fb9865..236d05f99e80 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -130,6 +130,17 @@ config AD799X
>> To compile this driver as a module, choose M here: the module will be
>> called ad799x.
>>
>> +config ASPEED_ADC
>> + tristate "Aspeed ADC"
>> + depends on ARCH_ASPEED || COMPILE_TEST
>> + depends on COMMON_CLK
>> + help
>> + If you say yes here you get support for the ADC included in Aspeed
>> + BMC SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called aspeed_adc.
>> +
>> config AT91_ADC
>> tristate "Atmel AT91 ADC"
>> depends on ARCH_AT91
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 73dbe399f894..306f10ffca74 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
>> obj-$(CONFIG_AD7793) += ad7793.o
>> obj-$(CONFIG_AD7887) += ad7887.o
>> obj-$(CONFIG_AD799X) += ad799x.o
>> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
>> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>> new file mode 100644
>> index 000000000000..e051fdf8ecbd
>> --- /dev/null
>> +++ b/drivers/iio/adc/aspeed_adc.c
>> @@ -0,0 +1,284 @@
>> +/*
>> + * 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/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +
>> +#define ASPEED_RESOLUTION_BITS 10
>> +#define ASPEED_CLOCKS_PER_SAMPLE 12
>> +
>> +#define ASPEED_REG_ENGINE_CONTROL 0x00
>> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04
>> +#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
>> +#define ASPEED_REG_CLOCK_CONTROL 0x0C
>> +#define ASPEED_REG_MAX 0xC0
>> +
>> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>> +#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
>> +#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
>> +
>> +#define ASPEED_ENGINE_ENABLE BIT(0)
>> +
>> +struct aspeed_adc_model_data {
>> + const char *model_name;
>> + unsigned int min_sampling_rate; // Hz
>> + unsigned int max_sampling_rate; // Hz
>> + unsigned int vref_voltage; // mV
>> +};
>> +
>> +struct aspeed_adc_data {
>> + struct device *dev;
>> + void __iomem *base;
>> + spinlock_t clk_lock;
>> + struct clk_hw *clk_prescaler;
>> + struct clk_hw *clk_scaler;
>> +};
>> +
>> +#define ASPEED_CHAN(_idx, _data_reg_addr) { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (_idx), \
>> + .address = (_data_reg_addr), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +}
>> +
>> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>> + ASPEED_CHAN(0, 0x10),
>> + ASPEED_CHAN(1, 0x12),
>> + ASPEED_CHAN(2, 0x14),
>> + ASPEED_CHAN(3, 0x16),
>> + ASPEED_CHAN(4, 0x18),
>> + ASPEED_CHAN(5, 0x1A),
>> + ASPEED_CHAN(6, 0x1C),
>> + ASPEED_CHAN(7, 0x1E),
>> + ASPEED_CHAN(8, 0x20),
>> + ASPEED_CHAN(9, 0x22),
>> + ASPEED_CHAN(10, 0x24),
>> + ASPEED_CHAN(11, 0x26),
>> + ASPEED_CHAN(12, 0x28),
>> + ASPEED_CHAN(13, 0x2A),
>> + ASPEED_CHAN(14, 0x2C),
>> + ASPEED_CHAN(15, 0x2E),
>> +};
>> +
>> +static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> + const struct aspeed_adc_model_data *model_data =
>> + of_device_get_match_data(data->dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + *val = readw(data->base + chan->address);
>> + return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = model_data->vref_voltage;
>> + *val2 = ASPEED_RESOLUTION_BITS;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *val = clk_get_rate(data->clk_scaler->clk) /
>> + ASPEED_CLOCKS_PER_SAMPLE;
>> + return IIO_VAL_INT;
>> +
>> + default:
>> + return -EPERM;
> Unusual, but I supose a valid choice.
I must have changed this one by accident. It was EINVAL previously
and that is at least literally true.
>> + }
>> +}
>> +
>> +static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> + const struct aspeed_adc_model_data *model_data =
>> + of_device_get_match_data(data->dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + if (val < model_data->min_sampling_rate ||
>> + val > model_data->max_sampling_rate)
>> + return -EINVAL;
>> +
>> + clk_set_rate(data->clk_scaler->clk,
>> + val * ASPEED_CLOCKS_PER_SAMPLE);
>> + return 0;
>> +
>> + default:
>> + return -EPERM;
Similar to above, EINVAL makes more sense as the default case. EPERM
came from thinking about attempts to write INFO_SCALE or INFO_RAW.
Technically EINVAL is accurate but it's more a policy decision that
I'm not allowing writes to those.
>> + }
>> +}
>> +
>> +static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
>> + unsigned int reg, unsigned int writeval,
>> + unsigned int *readval)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> +
>> + if (!readval || reg % 4 || reg > ASPEED_REG_MAX)
>> + return -EINVAL;
>> +
>> + *readval = readl(data->base + reg);
>> + return 0;
>> +}
>> +
>> +static const struct iio_info aspeed_adc_iio_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = aspeed_adc_read_raw,
>> + .write_raw = aspeed_adc_write_raw,
>> + .debugfs_reg_access = aspeed_adc_reg_access,
>> +};
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct aspeed_adc_data *data;
>> + const struct aspeed_adc_model_data *model_data;
>> + struct resource *res;
>> + const char *clk_parent_name;
>> + int ret;
>> + u32 adc_engine_control_reg_val;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &pdev->dev;
>> +
>> + 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);
>> +
>> + /* Register ADC clock prescaler with source specified by device tree. */
>> + spin_lock_init(&data->clk_lock);
>> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>> +
>> + data->clk_prescaler = clk_hw_register_divider(
>> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_prescaler))
>> + return PTR_ERR(data->clk_prescaler);
>> +
>> + /*
>> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> + * setting to adjust the prescaler as well.
>> + */
>> + data->clk_scaler = clk_hw_register_divider(
>> + &pdev->dev, "scaler", "prescaler",
>> + CLK_SET_RATE_PARENT,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + ret = PTR_ERR(data->clk_scaler);
>> + goto scaler_error;
>> + }
>> +
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>> + adc_engine_control_reg_val = GENMASK(31, 16) |
>> + ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
>> + writel(adc_engine_control_reg_val,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> +
>> + model_data = of_device_get_match_data(&pdev->dev);
>> + indio_dev->name = model_data->model_name;
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->info = &aspeed_adc_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->channels = aspeed_adc_iio_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret)
>> + goto iio_register_error;
>> +
>> + return 0;
>> +
>> +iio_register_error:
>> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> +
>> +scaler_error:
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> + return ret;
>> +}
>> +
>> +static int aspeed_adc_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> +
>> + iio_device_unregister(indio_dev);
>> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct aspeed_adc_model_data ast2400_model_data = {
>> + .model_name = "ast2400-adc",
>> + .vref_voltage = 2500, // mV
>> + .min_sampling_rate = 10000,
>> + .max_sampling_rate = 500000,
>> +};
>> +
>> +static const struct aspeed_adc_model_data ast2500_model_data = {
>> + .model_name = "ast2500-adc",
>> + .vref_voltage = 1800, // mV
>> + .min_sampling_rate = 1,
>> + .max_sampling_rate = 1000000,
>> +};
>> +
>> +static const struct of_device_id aspeed_adc_matches[] = {
>> + { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
>> + { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>> +
>> +static struct platform_driver aspeed_adc_driver = {
>> + .probe = aspeed_adc_probe,
>> + .remove = aspeed_adc_remove,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .of_match_table = aspeed_adc_matches,
>> + }
>> +};
>> +
>> +module_platform_driver(aspeed_adc_driver);
>> +
>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>> +MODULE_LICENSE("GPL");
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
@ 2017-03-27 23:05 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-03-27 23:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Peter Meerwald-Stadler, Martin Blumenstingl, Michael Turquette,
Rob Herring, Stephen Boyd, Alison Schofield, Fabrice Gasnier,
Hartmut Knaack, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Matt Ranostay
On Sat, Mar 25, 2017 at 10:32 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/03/17 18:41, Rick Altherr wrote:
>> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
>> interrupts are supported by the hardware but are not currently implemented.
>>
>> Signed-off-by: Rick Altherr <raltherr@google.com>
> Nice little driver.
>
> I'd like input from clk maintainers on the clk stuff though before applying.
> Michael, Stephen MAINTAINERS suggests this is you two. Any comments?
>
> Thanks,
>
> Jonathan
>
>> ---
>>
>> Changes in v4:
>> - Avoid copying per-model data to per-instance data
>>
>> Changes in v3:
>> - Drop model numbers from description as same IP is used in every generation
>> - Remove unused macros
>> - Shorten macro prefix to just ASPEED_
>> - Remove extraneous whitespace
>> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose
>> - Use existing macros in place of magic constants
>> - Remove unnecessary logging in failure conditions during probe
>> - Disable ADC hardware during remove()
>> - Add a NULL terminator to of_match_table
>> - Add per-model data to accomodate slight variations (vref, min/max sample rate)
>> - Added missing Kconfig depends on COMMON_CLK
>>
>> Changes in v2:
>> - Rewritten as an IIO device
>> - Renamed register macros to describe the register's purpose
>> - Replaced awkward reading of 16-bit data registers with readw()
>> - Added Kconfig dependency on COMPILE_TEST
>>
>> drivers/iio/adc/Kconfig | 11 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/aspeed_adc.c | 284 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 296 insertions(+)
>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 2268a6fb9865..236d05f99e80 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -130,6 +130,17 @@ config AD799X
>> To compile this driver as a module, choose M here: the module will be
>> called ad799x.
>>
>> +config ASPEED_ADC
>> + tristate "Aspeed ADC"
>> + depends on ARCH_ASPEED || COMPILE_TEST
>> + depends on COMMON_CLK
>> + help
>> + If you say yes here you get support for the ADC included in Aspeed
>> + BMC SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called aspeed_adc.
>> +
>> config AT91_ADC
>> tristate "Atmel AT91 ADC"
>> depends on ARCH_AT91
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 73dbe399f894..306f10ffca74 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
>> obj-$(CONFIG_AD7793) += ad7793.o
>> obj-$(CONFIG_AD7887) += ad7887.o
>> obj-$(CONFIG_AD799X) += ad799x.o
>> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
>> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>> new file mode 100644
>> index 000000000000..e051fdf8ecbd
>> --- /dev/null
>> +++ b/drivers/iio/adc/aspeed_adc.c
>> @@ -0,0 +1,284 @@
>> +/*
>> + * 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/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +
>> +#define ASPEED_RESOLUTION_BITS 10
>> +#define ASPEED_CLOCKS_PER_SAMPLE 12
>> +
>> +#define ASPEED_REG_ENGINE_CONTROL 0x00
>> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04
>> +#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
>> +#define ASPEED_REG_CLOCK_CONTROL 0x0C
>> +#define ASPEED_REG_MAX 0xC0
>> +
>> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>> +#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
>> +#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
>> +
>> +#define ASPEED_ENGINE_ENABLE BIT(0)
>> +
>> +struct aspeed_adc_model_data {
>> + const char *model_name;
>> + unsigned int min_sampling_rate; // Hz
>> + unsigned int max_sampling_rate; // Hz
>> + unsigned int vref_voltage; // mV
>> +};
>> +
>> +struct aspeed_adc_data {
>> + struct device *dev;
>> + void __iomem *base;
>> + spinlock_t clk_lock;
>> + struct clk_hw *clk_prescaler;
>> + struct clk_hw *clk_scaler;
>> +};
>> +
>> +#define ASPEED_CHAN(_idx, _data_reg_addr) { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (_idx), \
>> + .address = (_data_reg_addr), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +}
>> +
>> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>> + ASPEED_CHAN(0, 0x10),
>> + ASPEED_CHAN(1, 0x12),
>> + ASPEED_CHAN(2, 0x14),
>> + ASPEED_CHAN(3, 0x16),
>> + ASPEED_CHAN(4, 0x18),
>> + ASPEED_CHAN(5, 0x1A),
>> + ASPEED_CHAN(6, 0x1C),
>> + ASPEED_CHAN(7, 0x1E),
>> + ASPEED_CHAN(8, 0x20),
>> + ASPEED_CHAN(9, 0x22),
>> + ASPEED_CHAN(10, 0x24),
>> + ASPEED_CHAN(11, 0x26),
>> + ASPEED_CHAN(12, 0x28),
>> + ASPEED_CHAN(13, 0x2A),
>> + ASPEED_CHAN(14, 0x2C),
>> + ASPEED_CHAN(15, 0x2E),
>> +};
>> +
>> +static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> + const struct aspeed_adc_model_data *model_data =
>> + of_device_get_match_data(data->dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + *val = readw(data->base + chan->address);
>> + return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = model_data->vref_voltage;
>> + *val2 = ASPEED_RESOLUTION_BITS;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *val = clk_get_rate(data->clk_scaler->clk) /
>> + ASPEED_CLOCKS_PER_SAMPLE;
>> + return IIO_VAL_INT;
>> +
>> + default:
>> + return -EPERM;
> Unusual, but I supose a valid choice.
I must have changed this one by accident. It was EINVAL previously
and that is at least literally true.
>> + }
>> +}
>> +
>> +static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> + const struct aspeed_adc_model_data *model_data =
>> + of_device_get_match_data(data->dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + if (val < model_data->min_sampling_rate ||
>> + val > model_data->max_sampling_rate)
>> + return -EINVAL;
>> +
>> + clk_set_rate(data->clk_scaler->clk,
>> + val * ASPEED_CLOCKS_PER_SAMPLE);
>> + return 0;
>> +
>> + default:
>> + return -EPERM;
Similar to above, EINVAL makes more sense as the default case. EPERM
came from thinking about attempts to write INFO_SCALE or INFO_RAW.
Technically EINVAL is accurate but it's more a policy decision that
I'm not allowing writes to those.
>> + }
>> +}
>> +
>> +static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
>> + unsigned int reg, unsigned int writeval,
>> + unsigned int *readval)
>> +{
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> +
>> + if (!readval || reg % 4 || reg > ASPEED_REG_MAX)
>> + return -EINVAL;
>> +
>> + *readval = readl(data->base + reg);
>> + return 0;
>> +}
>> +
>> +static const struct iio_info aspeed_adc_iio_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = aspeed_adc_read_raw,
>> + .write_raw = aspeed_adc_write_raw,
>> + .debugfs_reg_access = aspeed_adc_reg_access,
>> +};
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct aspeed_adc_data *data;
>> + const struct aspeed_adc_model_data *model_data;
>> + struct resource *res;
>> + const char *clk_parent_name;
>> + int ret;
>> + u32 adc_engine_control_reg_val;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &pdev->dev;
>> +
>> + 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);
>> +
>> + /* Register ADC clock prescaler with source specified by device tree. */
>> + spin_lock_init(&data->clk_lock);
>> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>> +
>> + data->clk_prescaler = clk_hw_register_divider(
>> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_prescaler))
>> + return PTR_ERR(data->clk_prescaler);
>> +
>> + /*
>> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> + * setting to adjust the prescaler as well.
>> + */
>> + data->clk_scaler = clk_hw_register_divider(
>> + &pdev->dev, "scaler", "prescaler",
>> + CLK_SET_RATE_PARENT,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + ret = PTR_ERR(data->clk_scaler);
>> + goto scaler_error;
>> + }
>> +
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>> + adc_engine_control_reg_val = GENMASK(31, 16) |
>> + ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
>> + writel(adc_engine_control_reg_val,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> +
>> + model_data = of_device_get_match_data(&pdev->dev);
>> + indio_dev->name = model_data->model_name;
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->info = &aspeed_adc_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->channels = aspeed_adc_iio_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret)
>> + goto iio_register_error;
>> +
>> + return 0;
>> +
>> +iio_register_error:
>> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> +
>> +scaler_error:
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> + return ret;
>> +}
>> +
>> +static int aspeed_adc_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> + struct aspeed_adc_data *data = iio_priv(indio_dev);
>> +
>> + iio_device_unregister(indio_dev);
>> + writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>> + data->base + ASPEED_REG_ENGINE_CONTROL);
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct aspeed_adc_model_data ast2400_model_data = {
>> + .model_name = "ast2400-adc",
>> + .vref_voltage = 2500, // mV
>> + .min_sampling_rate = 10000,
>> + .max_sampling_rate = 500000,
>> +};
>> +
>> +static const struct aspeed_adc_model_data ast2500_model_data = {
>> + .model_name = "ast2500-adc",
>> + .vref_voltage = 1800, // mV
>> + .min_sampling_rate = 1,
>> + .max_sampling_rate = 1000000,
>> +};
>> +
>> +static const struct of_device_id aspeed_adc_matches[] = {
>> + { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
>> + { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>> +
>> +static struct platform_driver aspeed_adc_driver = {
>> + .probe = aspeed_adc_probe,
>> + .remove = aspeed_adc_remove,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .of_match_table = aspeed_adc_matches,
>> + }
>> +};
>> +
>> +module_platform_driver(aspeed_adc_driver);
>> +
>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>> +MODULE_LICENSE("GPL");
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-03-23 18:41 ` [PATCH v4 2/2] iio: " Rick Altherr
2017-03-24 4:03 ` Joel Stanley
2017-03-25 17:32 ` Jonathan Cameron
@ 2017-04-05 20:27 ` Stephen Boyd
2017-04-05 23:49 ` Rick Altherr
2 siblings, 1 reply; 18+ messages in thread
From: Stephen Boyd @ 2017-04-05 20:27 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, linux-kernel, linux-iio, Quentin Schulz, David Lechner,
Geert Uytterhoeven, William Breathitt Gray, linux-clk,
Andreas Klinger, Marek Vasut, Jonathan Cameron,
Peter Meerwald-Stadler, Martin Blumenstingl, Michael Turquette,
Rob Herring, Alison Schofield, Fabrice Gasnier, Hartmut Knaack,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Matt Ranostay
On 03/23, Rick Altherr wrote:
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct aspeed_adc_data *data;
> + const struct aspeed_adc_model_data *model_data;
> + struct resource *res;
> + const char *clk_parent_name;
> + int ret;
> + u32 adc_engine_control_reg_val;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &pdev->dev;
> +
> + 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);
> +
> + /* Register ADC clock prescaler with source specified by device tree. */
> + spin_lock_init(&data->clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
What if the parent clk is not registered yet? Or if we're not
always using DT in this driver? Put another way, this code is
fragile. But I guess it probably works well enough for now so no
big deal, just pointing out my fear.
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + &pdev->dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler))
> + return PTR_ERR(data->clk_prescaler);
> +
> + /*
> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> + * setting to adjust the prescaler as well.
> + */
> + data->clk_scaler = clk_hw_register_divider(
> + &pdev->dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + ret = PTR_ERR(data->clk_scaler);
> + goto scaler_error;
> + }
> +
> + /* Start all channels in normal mode. */
> + clk_prepare_enable(data->clk_scaler->clk);
Eventually we'd like to get rid of hw->clk pointer so that users
have to call some sort of clk_get() API and then we get warm
fuzzies from knowing who is consuming a clk structure. Can you
change this to register a clk provider and call clk_get()? I
think a device that references itself should be OK in DT still,
and would properly reflect what's going on.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-04-05 20:27 ` Stephen Boyd
@ 2017-04-05 23:49 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-04-05 23:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Matt Ranostay
On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 03/23, Rick Altherr wrote:
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct aspeed_adc_data *data;
>> + const struct aspeed_adc_model_data *model_data;
>> + struct resource *res;
>> + const char *clk_parent_name;
>> + int ret;
>> + u32 adc_engine_control_reg_val;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &pdev->dev;
>> +
>> + 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);
>> +
>> + /* Register ADC clock prescaler with source specified by device tree. */
>> + spin_lock_init(&data->clk_lock);
>> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>
> What if the parent clk is not registered yet? Or if we're not
> always using DT in this driver? Put another way, this code is
> fragile. But I guess it probably works well enough for now so no
> big deal, just pointing out my fear.
I'm not terribly worried about not using DT for this driver as it is
for an ARM SoC's built-in ADC which is only supported via DT. I take
your point though. What's the right way to do this? Use clk_get() to
request by name and clock-names in the DT?
>
>> +
>> + data->clk_prescaler = clk_hw_register_divider(
>> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_prescaler))
>> + return PTR_ERR(data->clk_prescaler);
>> +
>> + /*
>> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> + * setting to adjust the prescaler as well.
>> + */
>> + data->clk_scaler = clk_hw_register_divider(
>> + &pdev->dev, "scaler", "prescaler",
>> + CLK_SET_RATE_PARENT,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + ret = PTR_ERR(data->clk_scaler);
>> + goto scaler_error;
>> + }
>> +
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>
> Eventually we'd like to get rid of hw->clk pointer so that users
> have to call some sort of clk_get() API and then we get warm
> fuzzies from knowing who is consuming a clk structure. Can you
> change this to register a clk provider and call clk_get()? I
> think a device that references itself should be OK in DT still,
> and would properly reflect what's going on.
Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
something else? I'm still wrapping my head around the distinction
between clk, clk_hw, and a clk provider.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
@ 2017-04-05 23:49 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-04-05 23:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Matt Ranostay
On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 03/23, Rick Altherr wrote:
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct aspeed_adc_data *data;
>> + const struct aspeed_adc_model_data *model_data;
>> + struct resource *res;
>> + const char *clk_parent_name;
>> + int ret;
>> + u32 adc_engine_control_reg_val;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &pdev->dev;
>> +
>> + 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);
>> +
>> + /* Register ADC clock prescaler with source specified by device tree. */
>> + spin_lock_init(&data->clk_lock);
>> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>
> What if the parent clk is not registered yet? Or if we're not
> always using DT in this driver? Put another way, this code is
> fragile. But I guess it probably works well enough for now so no
> big deal, just pointing out my fear.
I'm not terribly worried about not using DT for this driver as it is
for an ARM SoC's built-in ADC which is only supported via DT. I take
your point though. What's the right way to do this? Use clk_get() to
request by name and clock-names in the DT?
>
>> +
>> + data->clk_prescaler = clk_hw_register_divider(
>> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_prescaler))
>> + return PTR_ERR(data->clk_prescaler);
>> +
>> + /*
>> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> + * setting to adjust the prescaler as well.
>> + */
>> + data->clk_scaler = clk_hw_register_divider(
>> + &pdev->dev, "scaler", "prescaler",
>> + CLK_SET_RATE_PARENT,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + ret = PTR_ERR(data->clk_scaler);
>> + goto scaler_error;
>> + }
>> +
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>
> Eventually we'd like to get rid of hw->clk pointer so that users
> have to call some sort of clk_get() API and then we get warm
> fuzzies from knowing who is consuming a clk structure. Can you
> change this to register a clk provider and call clk_get()? I
> think a device that references itself should be OK in DT still,
> and would properly reflect what's going on.
Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
something else? I'm still wrapping my head around the distinction
between clk, clk_hw, and a clk provider.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-04-05 23:49 ` Rick Altherr
(?)
@ 2017-04-06 1:10 ` Stephen Boyd
2017-04-06 18:57 ` Rick Altherr
-1 siblings, 1 reply; 18+ messages in thread
From: Stephen Boyd @ 2017-04-06 1:10 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Matt Ranostay
On 04/05, Rick Altherr wrote:
> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 03/23, Rick Altherr wrote:
> >> +
> >> +static int aspeed_adc_probe(struct platform_device *pdev)
> >> +{
> >> + struct iio_dev *indio_dev;
> >> + struct aspeed_adc_data *data;
> >> + const struct aspeed_adc_model_data *model_data;
> >> + struct resource *res;
> >> + const char *clk_parent_name;
> >> + int ret;
> >> + u32 adc_engine_control_reg_val;
> >> +
> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> >> + if (!indio_dev)
> >> + return -ENOMEM;
> >> +
> >> + data = iio_priv(indio_dev);
> >> + data->dev = &pdev->dev;
> >> +
> >> + 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);
> >> +
> >> + /* Register ADC clock prescaler with source specified by device tree. */
> >> + spin_lock_init(&data->clk_lock);
> >> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> >
> > What if the parent clk is not registered yet? Or if we're not
> > always using DT in this driver? Put another way, this code is
> > fragile. But I guess it probably works well enough for now so no
> > big deal, just pointing out my fear.
>
> I'm not terribly worried about not using DT for this driver as it is
> for an ARM SoC's built-in ADC which is only supported via DT. I take
> your point though. What's the right way to do this? Use clk_get() to
> request by name and clock-names in the DT?
Yes that will work. When we add probe defer to clk_get() things
will work better. Presumably the clocks property already exists
for this device so that of_clk_get_parent_name() works so it's
not a binding change?
>
> >
> >> +
> >> + data->clk_prescaler = clk_hw_register_divider(
> >> + &pdev->dev, "prescaler", clk_parent_name, 0,
> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
> >> + 17, 15, 0, &data->clk_lock);
> >> + if (IS_ERR(data->clk_prescaler))
> >> + return PTR_ERR(data->clk_prescaler);
> >> +
> >> + /*
> >> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> >> + * setting to adjust the prescaler as well.
> >> + */
> >> + data->clk_scaler = clk_hw_register_divider(
> >> + &pdev->dev, "scaler", "prescaler",
> >> + CLK_SET_RATE_PARENT,
> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
> >> + 0, 10, 0, &data->clk_lock);
> >> + if (IS_ERR(data->clk_scaler)) {
> >> + ret = PTR_ERR(data->clk_scaler);
> >> + goto scaler_error;
> >> + }
> >> +
> >> + /* Start all channels in normal mode. */
> >> + clk_prepare_enable(data->clk_scaler->clk);
> >
> > Eventually we'd like to get rid of hw->clk pointer so that users
> > have to call some sort of clk_get() API and then we get warm
> > fuzzies from knowing who is consuming a clk structure. Can you
> > change this to register a clk provider and call clk_get()? I
> > think a device that references itself should be OK in DT still,
> > and would properly reflect what's going on.
>
> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
> something else? I'm still wrapping my head around the distinction
> between clk, clk_hw, and a clk provider.
>
Yes. Unless you don't want to expose these details in DT because
it's all internal to the device?
In that case we need to go merge that patch on the clk list to
have clk_hw_create_clk() or something like that, so we can turn a
clk_hw structure into a clk pointer without directly referencing
the clk member of clk_hw.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-04-06 1:10 ` Stephen Boyd
@ 2017-04-06 18:57 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-04-06 18:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Akinobu Mita, Matt Ranostay
On Wed, Apr 5, 2017 at 6:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/05, Rick Altherr wrote:
>> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 03/23, Rick Altherr wrote:
>> >> +
>> >> +static int aspeed_adc_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct iio_dev *indio_dev;
>> >> + struct aspeed_adc_data *data;
>> >> + const struct aspeed_adc_model_data *model_data;
>> >> + struct resource *res;
>> >> + const char *clk_parent_name;
>> >> + int ret;
>> >> + u32 adc_engine_control_reg_val;
>> >> +
>> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> >> + if (!indio_dev)
>> >> + return -ENOMEM;
>> >> +
>> >> + data = iio_priv(indio_dev);
>> >> + data->dev = &pdev->dev;
>> >> +
>> >> + 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);
>> >> +
>> >> + /* Register ADC clock prescaler with source specified by device tree. */
>> >> + spin_lock_init(&data->clk_lock);
>> >> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>> >
>> > What if the parent clk is not registered yet? Or if we're not
>> > always using DT in this driver? Put another way, this code is
>> > fragile. But I guess it probably works well enough for now so no
>> > big deal, just pointing out my fear.
>>
>> I'm not terribly worried about not using DT for this driver as it is
>> for an ARM SoC's built-in ADC which is only supported via DT. I take
>> your point though. What's the right way to do this? Use clk_get() to
>> request by name and clock-names in the DT?
>
> Yes that will work. When we add probe defer to clk_get() things
> will work better. Presumably the clocks property already exists
> for this device so that of_clk_get_parent_name() works so it's
> not a binding change?
The bindings include clocks but not clock-names. In this case,
clk_register_divider() only has variants that take a parent clock
name. I can't see what I'd do with the result of clk_get(). If the
bindings provide clock-names, I can provide a fixed string for the
parent name instead of relying on of_clk_get_parent_name(). The
removes an explicit driver dependency on DT. I'm unclear if it
resolves your concerns about registration ordering.
>
>>
>> >
>> >> +
>> >> + data->clk_prescaler = clk_hw_register_divider(
>> >> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 17, 15, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_prescaler))
>> >> + return PTR_ERR(data->clk_prescaler);
>> >> +
>> >> + /*
>> >> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> >> + * setting to adjust the prescaler as well.
>> >> + */
>> >> + data->clk_scaler = clk_hw_register_divider(
>> >> + &pdev->dev, "scaler", "prescaler",
>> >> + CLK_SET_RATE_PARENT,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 0, 10, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_scaler)) {
>> >> + ret = PTR_ERR(data->clk_scaler);
>> >> + goto scaler_error;
>> >> + }
>> >> +
>> >> + /* Start all channels in normal mode. */
>> >> + clk_prepare_enable(data->clk_scaler->clk);
>> >
>> > Eventually we'd like to get rid of hw->clk pointer so that users
>> > have to call some sort of clk_get() API and then we get warm
>> > fuzzies from knowing who is consuming a clk structure. Can you
>> > change this to register a clk provider and call clk_get()? I
>> > think a device that references itself should be OK in DT still,
>> > and would properly reflect what's going on.
>>
>> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
>> something else? I'm still wrapping my head around the distinction
>> between clk, clk_hw, and a clk provider.
>>
>
> Yes. Unless you don't want to expose these details in DT because
> it's all internal to the device?
>
There's no reason to expose the scaler or prescaler in DT. These are
clocks that internal to the ADC and have no way for other devices to
use them. I only used the clock framework to avoid reinventing the
wheel on calculating divider values for a desired clock rate.
> In that case we need to go merge that patch on the clk list to
> have clk_hw_create_clk() or something like that, so we can turn a
> clk_hw structure into a clk pointer without directly referencing
> the clk member of clk_hw.
This makes the most sense to me.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
@ 2017-04-06 18:57 ` Rick Altherr
0 siblings, 0 replies; 18+ messages in thread
From: Rick Altherr @ 2017-04-06 18:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Akinobu Mita, Matt Ranostay
On Wed, Apr 5, 2017 at 6:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/05, Rick Altherr wrote:
>> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 03/23, Rick Altherr wrote:
>> >> +
>> >> +static int aspeed_adc_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct iio_dev *indio_dev;
>> >> + struct aspeed_adc_data *data;
>> >> + const struct aspeed_adc_model_data *model_data;
>> >> + struct resource *res;
>> >> + const char *clk_parent_name;
>> >> + int ret;
>> >> + u32 adc_engine_control_reg_val;
>> >> +
>> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> >> + if (!indio_dev)
>> >> + return -ENOMEM;
>> >> +
>> >> + data = iio_priv(indio_dev);
>> >> + data->dev = &pdev->dev;
>> >> +
>> >> + 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);
>> >> +
>> >> + /* Register ADC clock prescaler with source specified by device tree. */
>> >> + spin_lock_init(&data->clk_lock);
>> >> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>> >
>> > What if the parent clk is not registered yet? Or if we're not
>> > always using DT in this driver? Put another way, this code is
>> > fragile. But I guess it probably works well enough for now so no
>> > big deal, just pointing out my fear.
>>
>> I'm not terribly worried about not using DT for this driver as it is
>> for an ARM SoC's built-in ADC which is only supported via DT. I take
>> your point though. What's the right way to do this? Use clk_get() to
>> request by name and clock-names in the DT?
>
> Yes that will work. When we add probe defer to clk_get() things
> will work better. Presumably the clocks property already exists
> for this device so that of_clk_get_parent_name() works so it's
> not a binding change?
The bindings include clocks but not clock-names. In this case,
clk_register_divider() only has variants that take a parent clock
name. I can't see what I'd do with the result of clk_get(). If the
bindings provide clock-names, I can provide a fixed string for the
parent name instead of relying on of_clk_get_parent_name(). The
removes an explicit driver dependency on DT. I'm unclear if it
resolves your concerns about registration ordering.
>
>>
>> >
>> >> +
>> >> + data->clk_prescaler = clk_hw_register_divider(
>> >> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 17, 15, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_prescaler))
>> >> + return PTR_ERR(data->clk_prescaler);
>> >> +
>> >> + /*
>> >> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> >> + * setting to adjust the prescaler as well.
>> >> + */
>> >> + data->clk_scaler = clk_hw_register_divider(
>> >> + &pdev->dev, "scaler", "prescaler",
>> >> + CLK_SET_RATE_PARENT,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 0, 10, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_scaler)) {
>> >> + ret = PTR_ERR(data->clk_scaler);
>> >> + goto scaler_error;
>> >> + }
>> >> +
>> >> + /* Start all channels in normal mode. */
>> >> + clk_prepare_enable(data->clk_scaler->clk);
>> >
>> > Eventually we'd like to get rid of hw->clk pointer so that users
>> > have to call some sort of clk_get() API and then we get warm
>> > fuzzies from knowing who is consuming a clk structure. Can you
>> > change this to register a clk provider and call clk_get()? I
>> > think a device that references itself should be OK in DT still,
>> > and would properly reflect what's going on.
>>
>> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
>> something else? I'm still wrapping my head around the distinction
>> between clk, clk_hw, and a clk provider.
>>
>
> Yes. Unless you don't want to expose these details in DT because
> it's all internal to the device?
>
There's no reason to expose the scaler or prescaler in DT. These are
clocks that internal to the ADC and have no way for other devices to
use them. I only used the clock framework to avoid reinventing the
wheel on calculating divider values for a desired clock rate.
> In that case we need to go merge that patch on the clk list to
> have clk_hw_create_clk() or something like that, so we can turn a
> clk_hw structure into a clk pointer without directly referencing
> the clk member of clk_hw.
This makes the most sense to me.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/2] iio: Aspeed ADC
2017-04-06 18:57 ` Rick Altherr
(?)
@ 2017-04-06 23:03 ` Stephen Boyd
-1 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2017-04-06 23:03 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, linux-iio,
Quentin Schulz, David Lechner, Geert Uytterhoeven,
William Breathitt Gray, linux-clk, Andreas Klinger, Marek Vasut,
Jonathan Cameron, Peter Meerwald-Stadler, Martin Blumenstingl,
Michael Turquette, Rob Herring, Alison Schofield,
Fabrice Gasnier, Hartmut Knaack, Lars-Peter Clausen,
Akinobu Mita, Matt Ranostay
On 04/06, Rick Altherr wrote:
> On Wed, Apr 5, 2017 at 6:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 04/05, Rick Altherr wrote:
> >> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> > On 03/23, Rick Altherr wrote:
> >> >> +
> >> >> +static int aspeed_adc_probe(struct platform_device *pdev)
> >> >> +{
> >> >> + struct iio_dev *indio_dev;
> >> >> + struct aspeed_adc_data *data;
> >> >> + const struct aspeed_adc_model_data *model_data;
> >> >> + struct resource *res;
> >> >> + const char *clk_parent_name;
> >> >> + int ret;
> >> >> + u32 adc_engine_control_reg_val;
> >> >> +
> >> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> >> >> + if (!indio_dev)
> >> >> + return -ENOMEM;
> >> >> +
> >> >> + data = iio_priv(indio_dev);
> >> >> + data->dev = &pdev->dev;
> >> >> +
> >> >> + 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);
> >> >> +
> >> >> + /* Register ADC clock prescaler with source specified by device tree. */
> >> >> + spin_lock_init(&data->clk_lock);
> >> >> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> >> >
> >> > What if the parent clk is not registered yet? Or if we're not
> >> > always using DT in this driver? Put another way, this code is
> >> > fragile. But I guess it probably works well enough for now so no
> >> > big deal, just pointing out my fear.
> >>
> >> I'm not terribly worried about not using DT for this driver as it is
> >> for an ARM SoC's built-in ADC which is only supported via DT. I take
> >> your point though. What's the right way to do this? Use clk_get() to
> >> request by name and clock-names in the DT?
> >
> > Yes that will work. When we add probe defer to clk_get() things
> > will work better. Presumably the clocks property already exists
> > for this device so that of_clk_get_parent_name() works so it's
> > not a binding change?
>
> The bindings include clocks but not clock-names. In this case,
> clk_register_divider() only has variants that take a parent clock
> name. I can't see what I'd do with the result of clk_get(). If the
> bindings provide clock-names, I can provide a fixed string for the
> parent name instead of relying on of_clk_get_parent_name(). The
> removes an explicit driver dependency on DT. I'm unclear if it
> resolves your concerns about registration ordering.
We have __clk_get_name() as a helper in clk-provider.h to assist
drivers in finding parent names of clks they get through
clk_get(). Eventually we want to get rid of that function, but
using it at least helps us find it later.
Aside: This is a problem we've had for a while. You're an
innocent bystander here. The usage of strings for clk tree
description is not working well and we really need to move away
from it. So far we have no solution so all I can say is sorry you
tripped up on this.
>
> >
> >>
> >> >
> >> >> +
> >> >> + data->clk_prescaler = clk_hw_register_divider(
> >> >> + &pdev->dev, "prescaler", clk_parent_name, 0,
> >> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
> >> >> + 17, 15, 0, &data->clk_lock);
> >> >> + if (IS_ERR(data->clk_prescaler))
> >> >> + return PTR_ERR(data->clk_prescaler);
> >> >> +
> >> >> + /*
> >> >> + * Register ADC clock scaler downstream from the prescaler. Allow rate
> >> >> + * setting to adjust the prescaler as well.
> >> >> + */
> >> >> + data->clk_scaler = clk_hw_register_divider(
> >> >> + &pdev->dev, "scaler", "prescaler",
> >> >> + CLK_SET_RATE_PARENT,
> >> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
> >> >> + 0, 10, 0, &data->clk_lock);
> >> >> + if (IS_ERR(data->clk_scaler)) {
> >> >> + ret = PTR_ERR(data->clk_scaler);
> >> >> + goto scaler_error;
> >> >> + }
> >> >> +
> >> >> + /* Start all channels in normal mode. */
> >> >> + clk_prepare_enable(data->clk_scaler->clk);
> >> >
> >> > Eventually we'd like to get rid of hw->clk pointer so that users
> >> > have to call some sort of clk_get() API and then we get warm
> >> > fuzzies from knowing who is consuming a clk structure. Can you
> >> > change this to register a clk provider and call clk_get()? I
> >> > think a device that references itself should be OK in DT still,
> >> > and would properly reflect what's going on.
> >>
> >> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
> >> something else? I'm still wrapping my head around the distinction
> >> between clk, clk_hw, and a clk provider.
> >>
> >
> > Yes. Unless you don't want to expose these details in DT because
> > it's all internal to the device?
> >
>
> There's no reason to expose the scaler or prescaler in DT. These are
> clocks that internal to the ADC and have no way for other devices to
> use them. I only used the clock framework to avoid reinventing the
> wheel on calculating divider values for a desired clock rate.
>
> > In that case we need to go merge that patch on the clk list to
> > have clk_hw_create_clk() or something like that, so we can turn a
> > clk_hw structure into a clk pointer without directly referencing
> > the clk member of clk_hw.
>
> This makes the most sense to me.
>
Ok. Let me see what I can do to merge that patch now. I should be
able to put it on its own branch that could be pulled into the
tree for this driver. Or a merge window can pass and things fixed
then.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread