* [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
@ 2017-03-21 20:48 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-21 20:48 UTC (permalink / raw)
To: openbmc, linux-kernel
Cc: devicetree, linux-iio, Hartmut Knaack, Rob Herring,
Lars-Peter Clausen, Mark Rutland, Jonathan Cameron,
Peter Meerwald-Stadler
Signed-off-by: Rick Altherr <raltherr@google.com>
---
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index 000000000000..7748c2c2ad0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed AST2400/2500 ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xB0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.1.500.gab5fba24ee-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
@ 2017-03-21 20:48 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-21 20:48 UTC (permalink / raw)
To: openbmc-uLR06cmDAlY/bJ5BZ2RsiQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Hartmut Knaack, Rob Herring,
Lars-Peter Clausen, Mark Rutland, Jonathan Cameron,
Peter Meerwald-Stadler
Signed-off-by: Rick Altherr <raltherr-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index 000000000000..7748c2c2ad0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed AST2400/2500 ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xB0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.1.500.gab5fba24ee-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` Rick Altherr
(?)
@ 2017-03-21 20:48 ` Rick Altherr
2017-03-21 21:14 ` Peter Meerwald-Stadler
` (4 more replies)
-1 siblings, 5 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-21 20:48 UTC (permalink / raw)
To: openbmc, linux-kernel
Cc: Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, Hartmut Knaack, Quentin Schulz,
Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
Aspeed AST2400/AST2500 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>
---
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 | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 282 insertions(+)
create mode 100644 drivers/iio/adc/aspeed_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6fb9865..9672d799a3fb 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -130,6 +130,16 @@ config AD799X
To compile this driver as a module, choose M here: the module will be
called ad799x.
+config ASPEED_ADC
+ tristate "Aspeed AST2400/AST2500 ADC"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ help
+ If you say yes here you get support for the Aspeed AST2400/AST2500
+ ADC.
+
+ 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..9220909aefd4
--- /dev/null
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -0,0 +1,271 @@
+/*
+ * 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_ADC_NUM_CHANNELS 16
+#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
+#define ASPEED_ADC_RESOLUTION_BITS 10
+#define ASPEED_ADC_MIN_SAMP_RATE 10000
+#define ASPEED_ADC_MAX_SAMP_RATE 500000
+#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
+
+#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
+#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
+#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
+#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
+#define ASPEED_ADC_REG_MAX 0xC0
+
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem *base;
+
+ spinlock_t clk_lock;
+ struct clk_hw *clk_prescaler;
+ struct clk_hw *clk_scaler;
+};
+
+#define ASPEED_ADC_CHAN(_idx, _addr) { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = (_idx), \
+ .address = (_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_ADC_CHAN(0, 0x10),
+ ASPEED_ADC_CHAN(1, 0x12),
+ ASPEED_ADC_CHAN(2, 0x14),
+ ASPEED_ADC_CHAN(3, 0x16),
+ ASPEED_ADC_CHAN(4, 0x18),
+ ASPEED_ADC_CHAN(5, 0x1A),
+ ASPEED_ADC_CHAN(6, 0x1C),
+ ASPEED_ADC_CHAN(7, 0x1E),
+ ASPEED_ADC_CHAN(8, 0x20),
+ ASPEED_ADC_CHAN(9, 0x22),
+ ASPEED_ADC_CHAN(10, 0x24),
+ ASPEED_ADC_CHAN(11, 0x26),
+ ASPEED_ADC_CHAN(12, 0x28),
+ ASPEED_ADC_CHAN(13, 0x2A),
+ ASPEED_ADC_CHAN(14, 0x2C),
+ ASPEED_ADC_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);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *val = readw(data->base + chan->address);
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ *val = 2500; // mV
+ *val2 = 10;
+ return IIO_VAL_FRACTIONAL_LOG2;
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = clk_get_rate(data->clk_scaler->clk) /
+ ASPEED_ADC_CLOCKS_PER_SAMPLE;
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+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);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ if (val < ASPEED_ADC_MIN_SAMP_RATE ||
+ val > ASPEED_ADC_MAX_SAMP_RATE)
+ return -EINVAL;
+
+ clk_set_rate(data->clk_scaler->clk,
+ val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+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_ADC_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;
+ 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) {
+ dev_err(&pdev->dev, "Failed allocating iio device\n");
+ return -ENOMEM;
+ }
+
+ data = iio_priv(indio_dev);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ data->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(data->base)) {
+ dev_err(&pdev->dev, "Failed allocating device resources\n");
+ ret = PTR_ERR(data->base);
+ goto resource_error;
+ }
+
+ /* 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_ADC_REG_CLOCK_CONTROL,
+ 17, 15, 0, &data->clk_lock);
+ if (IS_ERR(data->clk_prescaler)) {
+ dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
+ ret = PTR_ERR(data->clk_prescaler);
+ goto prescaler_error;
+ }
+
+ /*
+ * 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_ADC_REG_CLOCK_CONTROL,
+ 0, 10, 0, &data->clk_lock);
+ if (IS_ERR(data->clk_scaler)) {
+ dev_err(&pdev->dev, "Failed allocating scaler clock\n");
+ 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
+ writel(adc_engine_control_reg_val,
+ data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
+
+ indio_dev->name = dev_name(&pdev->dev);
+ 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) {
+ dev_err(&pdev->dev, "Could't register the device.\n");
+ goto iio_register_error;
+ }
+
+ return 0;
+
+iio_register_error:
+ writel(0x0, data->base + ASPEED_ADC_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);
+
+prescaler_error:
+resource_error:
+ 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);
+ clk_disable_unprepare(data->clk_scaler->clk);
+ clk_hw_unregister_divider(data->clk_scaler);
+ clk_hw_unregister_divider(data->clk_prescaler);
+
+ return 0;
+}
+
+const struct of_device_id aspeed_adc_matches[] = {
+ { .compatible = "aspeed,ast2400-adc" },
+ { .compatible = "aspeed,ast2500-adc" },
+};
+MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
+
+static struct platform_driver aspeed_adc_driver = {
+ .probe = aspeed_adc_probe,
+ .remove = aspeed_adc_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_adc_matches,
+ }
+};
+
+module_platform_driver(aspeed_adc_driver);
+
+MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
+MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
+MODULE_LICENSE("GPL");
--
2.12.1.500.gab5fba24ee-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
@ 2017-03-21 21:14 ` Peter Meerwald-Stadler
2017-03-22 7:21 ` Quentin Schulz
` (3 subsequent siblings)
4 siblings, 0 replies; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-21 21:14 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, linux-kernel, Martin Blumenstingl, linux-iio,
Lars-Peter Clausen, Jonathan Cameron
> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
> and high threshold interrupts are supported by the hardware but are not
> currently implemented.
comments below
link to a datasheet would be nice
> Signed-off-by: Rick Altherr <raltherr@google.com>
> ---
>
> 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 | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 282 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..9672d799a3fb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,16 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed AST2400/AST2500 ADC"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + help
> + If you say yes here you get support for the Aspeed AST2400/AST2500
> + ADC.
> +
> + 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..9220909aefd4
> --- /dev/null
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,271 @@
> +/*
> + * 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_ADC_NUM_CHANNELS 16
_NUM_CHANNELS is not used, remove
> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
should be used below
> +#define ASPEED_ADC_RESOLUTION_BITS 10
should be used below
> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_ADC_REG_MAX 0xC0
> +
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
> +
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> +
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_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_ADC_CHAN(0, 0x10),
> + ASPEED_ADC_CHAN(1, 0x12),
> + ASPEED_ADC_CHAN(2, 0x14),
> + ASPEED_ADC_CHAN(3, 0x16),
> + ASPEED_ADC_CHAN(4, 0x18),
> + ASPEED_ADC_CHAN(5, 0x1A),
> + ASPEED_ADC_CHAN(6, 0x1C),
> + ASPEED_ADC_CHAN(7, 0x1E),
> + ASPEED_ADC_CHAN(8, 0x20),
> + ASPEED_ADC_CHAN(9, 0x22),
> + ASPEED_ADC_CHAN(10, 0x24),
> + ASPEED_ADC_CHAN(11, 0x26),
> + ASPEED_ADC_CHAN(12, 0x28),
> + ASPEED_ADC_CHAN(13, 0x2A),
> + ASPEED_ADC_CHAN(14, 0x2C),
> + ASPEED_ADC_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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 2500; // mV
REF_VOLTAGE?
> + *val2 = 10;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
> + val > ASPEED_ADC_MAX_SAMP_RATE)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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_ADC_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,
without & (matter of taste)
> + .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;
> + 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) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "Failed allocating device resources\n");
> + ret = PTR_ERR(data->base);
> + goto resource_error;
just return here?
> + }
> +
> + /* 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_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> + ret = PTR_ERR(data->clk_prescaler);
> + goto prescaler_error;
just return here?
> + }
> +
> + /*
> + * 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_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + 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) {
> + dev_err(&pdev->dev, "Could't register the device.\n");
Couldn't
> + goto iio_register_error;
> + }
> +
> + return 0;
> +
> +iio_register_error:
> + writel(0x0, data->base + ASPEED_ADC_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);
> +
> +prescaler_error:
> +resource_error:
> + 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(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
I guess this power off the device?
the MODE #defines are now used (powerdown, normal, etc.)
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc" },
> + { .compatible = "aspeed,ast2500-adc" },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> + .probe = aspeed_adc_probe,
> + .remove = aspeed_adc_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_adc_matches,
> + }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-21 21:14 ` Peter Meerwald-Stadler
0 siblings, 0 replies; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-21 21:14 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, linux-kernel, Martin Blumenstingl, linux-iio,
Lars-Peter Clausen, Jonathan Cameron
> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
> and high threshold interrupts are supported by the hardware but are not
> currently implemented.
comments below
link to a datasheet would be nice
> Signed-off-by: Rick Altherr <raltherr@google.com>
> ---
>
> 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 | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 282 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..9672d799a3fb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,16 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed AST2400/AST2500 ADC"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + help
> + If you say yes here you get support for the Aspeed AST2400/AST2500
> + ADC.
> +
> + 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..9220909aefd4
> --- /dev/null
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,271 @@
> +/*
> + * 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_ADC_NUM_CHANNELS 16
_NUM_CHANNELS is not used, remove
> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
should be used below
> +#define ASPEED_ADC_RESOLUTION_BITS 10
should be used below
> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_ADC_REG_MAX 0xC0
> +
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
> +
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> +
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_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_ADC_CHAN(0, 0x10),
> + ASPEED_ADC_CHAN(1, 0x12),
> + ASPEED_ADC_CHAN(2, 0x14),
> + ASPEED_ADC_CHAN(3, 0x16),
> + ASPEED_ADC_CHAN(4, 0x18),
> + ASPEED_ADC_CHAN(5, 0x1A),
> + ASPEED_ADC_CHAN(6, 0x1C),
> + ASPEED_ADC_CHAN(7, 0x1E),
> + ASPEED_ADC_CHAN(8, 0x20),
> + ASPEED_ADC_CHAN(9, 0x22),
> + ASPEED_ADC_CHAN(10, 0x24),
> + ASPEED_ADC_CHAN(11, 0x26),
> + ASPEED_ADC_CHAN(12, 0x28),
> + ASPEED_ADC_CHAN(13, 0x2A),
> + ASPEED_ADC_CHAN(14, 0x2C),
> + ASPEED_ADC_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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 2500; // mV
REF_VOLTAGE?
> + *val2 = 10;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
> + val > ASPEED_ADC_MAX_SAMP_RATE)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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_ADC_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,
without & (matter of taste)
> + .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;
> + 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) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "Failed allocating device resources\n");
> + ret = PTR_ERR(data->base);
> + goto resource_error;
just return here?
> + }
> +
> + /* 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_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> + ret = PTR_ERR(data->clk_prescaler);
> + goto prescaler_error;
just return here?
> + }
> +
> + /*
> + * 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_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + 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) {
> + dev_err(&pdev->dev, "Could't register the device.\n");
Couldn't
> + goto iio_register_error;
> + }
> +
> + return 0;
> +
> +iio_register_error:
> + writel(0x0, data->base + ASPEED_ADC_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);
> +
> +prescaler_error:
> +resource_error:
> + 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(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
I guess this power off the device?
the MODE #defines are now used (powerdown, normal, etc.)
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc" },
> + { .compatible = "aspeed,ast2500-adc" },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> +
> +static struct platform_driver aspeed_adc_driver = {
> + .probe = aspeed_adc_probe,
> + .remove = aspeed_adc_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_adc_matches,
> + }
> +};
> +
> +module_platform_driver(aspeed_adc_driver);
> +
> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` Rick Altherr
(?)
@ 2017-03-21 21:16 ` Peter Meerwald-Stadler
-1 siblings, 0 replies; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-21 21:16 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Lars-Peter Clausen, Mark Rutland, Jonathan Cameron
> Signed-off-by: Rick Altherr <raltherr-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
extreme nitpicking below
---
>
> Changes in v2:
> - Rewritten as an IIO ADC device
>
> .../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> new file mode 100644
> index 000000000000..7748c2c2ad0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -0,0 +1,20 @@
> +Aspeed AST2400/2500 ADC
> +
> +This device is a 10-bit converter for 16 voltage channels. All inputs are
> +single ended.
> +
> +Required properties:
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- reg: memory window mapping address and length
> +- clocks: Input clock used to derive the sample clock. Expected to be the
> + SoC's APB clock.
> +- #io-channel-cells: Must be set to <1> to indicate channels are selected
> + by index.
> +
> +Example:
> + adc@1e6e9000 {
> + compatible = "aspeed,ast2400-adc";
> + reg = <0x1e6e9000 0xB0>;
maybe 0xb0 (consistent lowercase hex)?
> + clocks = <&clk_apb>;
> + #io-channel-cells = <1>;
> + };
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
@ 2017-03-21 21:16 ` Peter Meerwald-Stadler
0 siblings, 0 replies; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-21 21:16 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, devicetree, linux-iio, Rob Herring, Lars-Peter Clausen,
Mark Rutland, Jonathan Cameron
> Signed-off-by: Rick Altherr <raltherr@google.com>
extreme nitpicking below
---
>
> Changes in v2:
> - Rewritten as an IIO ADC device
>
> .../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> new file mode 100644
> index 000000000000..7748c2c2ad0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -0,0 +1,20 @@
> +Aspeed AST2400/2500 ADC
> +
> +This device is a 10-bit converter for 16 voltage channels. All inputs are
> +single ended.
> +
> +Required properties:
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- reg: memory window mapping address and length
> +- clocks: Input clock used to derive the sample clock. Expected to be the
> + SoC's APB clock.
> +- #io-channel-cells: Must be set to <1> to indicate channels are selected
> + by index.
> +
> +Example:
> + adc@1e6e9000 {
> + compatible = "aspeed,ast2400-adc";
> + reg = <0x1e6e9000 0xB0>;
maybe 0xb0 (consistent lowercase hex)?
> + clocks = <&clk_apb>;
> + #io-channel-cells = <1>;
> + };
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
@ 2017-03-21 21:16 ` Peter Meerwald-Stadler
0 siblings, 0 replies; 25+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-21 21:16 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, devicetree, linux-iio, Rob Herring, Lars-Peter Clausen,
Mark Rutland, Jonathan Cameron
> Signed-off-by: Rick Altherr <raltherr@google.com>
extreme nitpicking below
---
>
> Changes in v2:
> - Rewritten as an IIO ADC device
>
> .../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> new file mode 100644
> index 000000000000..7748c2c2ad0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -0,0 +1,20 @@
> +Aspeed AST2400/2500 ADC
> +
> +This device is a 10-bit converter for 16 voltage channels. All inputs are
> +single ended.
> +
> +Required properties:
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- reg: memory window mapping address and length
> +- clocks: Input clock used to derive the sample clock. Expected to be the
> + SoC's APB clock.
> +- #io-channel-cells: Must be set to <1> to indicate channels are selected
> + by index.
> +
> +Example:
> + adc@1e6e9000 {
> + compatible = "aspeed,ast2400-adc";
> + reg = <0x1e6e9000 0xB0>;
maybe 0xb0 (consistent lowercase hex)?
> + clocks = <&clk_apb>;
> + #io-channel-cells = <1>;
> + };
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
2017-03-21 21:14 ` Peter Meerwald-Stadler
@ 2017-03-22 7:21 ` Quentin Schulz
2017-03-22 10:08 ` Joel Stanley
[not found] ` <CAPLgG=kVsNake71fFLc2NsoMtrtG0_6Fb6XHep3dQKVuRgOmbA@mail.gmail.com>
2017-03-22 9:47 ` Joel Stanley
` (2 subsequent siblings)
4 siblings, 2 replies; 25+ messages in thread
From: Quentin Schulz @ 2017-03-22 7:21 UTC (permalink / raw)
To: Rick Altherr, openbmc, linux-kernel
Cc: Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, Hartmut Knaack, Geert Uytterhoeven,
Marek Vasut, Matt Ranostay, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Fabrice Gasnier,
Jonathan Cameron, Peter Meerwald-Stadler, Maxime Ripard,
Jacopo Mondi
Hi,
On 21/03/2017 21:48, Rick Altherr wrote:
> Aspeed AST2400/AST2500 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>
> ---
>
> 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
>
[...]
> +struct aspeed_adc_data {
> + struct device *dev;
> + void __iomem *base;
> +
Useless empty line?
> + spinlock_t clk_lock;
> + struct clk_hw *clk_prescaler;
> + struct clk_hw *clk_scaler;
> +};
> +
> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .address = (_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_ADC_CHAN(0, 0x10),
> + ASPEED_ADC_CHAN(1, 0x12),
> + ASPEED_ADC_CHAN(2, 0x14),
> + ASPEED_ADC_CHAN(3, 0x16),
> + ASPEED_ADC_CHAN(4, 0x18),
> + ASPEED_ADC_CHAN(5, 0x1A),
> + ASPEED_ADC_CHAN(6, 0x1C),
> + ASPEED_ADC_CHAN(7, 0x1E),
> + ASPEED_ADC_CHAN(8, 0x20),
> + ASPEED_ADC_CHAN(9, 0x22),
> + ASPEED_ADC_CHAN(10, 0x24),
> + ASPEED_ADC_CHAN(11, 0x26),
> + ASPEED_ADC_CHAN(12, 0x28),
> + ASPEED_ADC_CHAN(13, 0x2A),
> + ASPEED_ADC_CHAN(14, 0x2C),
> + ASPEED_ADC_CHAN(15, 0x2E),
It would make sense to name the registers (the _addr parameter of your
macro) so it's easier to understand what it refers to.
[...]
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct aspeed_adc_data *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) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "Failed allocating device resources\n");
> + ret = PTR_ERR(data->base);
> + goto resource_error;
> + }
> +
> + /* 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_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> + ret = PTR_ERR(data->clk_prescaler);
> + goto prescaler_error;
> + }
> +
> + /*
> + * 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_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +
> + indio_dev->name = dev_name(&pdev->dev);
This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
of the mail in the probe function). Better name it aspeed-adc or even
better to have a different name per compatible: ast2400-adc or ast2500-adc.
> + 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) {
> + dev_err(&pdev->dev, "Could't register the device.\n");
> + goto iio_register_error;
> + }
> +
> + return 0;
> +
> +iio_register_error:
> + writel(0x0, data->base + ASPEED_ADC_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);
> +
> +prescaler_error:
> +resource_error:
> + return ret;
> +}
[...]
Thanks,Quentin
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
@ 2017-03-22 9:47 ` Joel Stanley
2017-03-22 7:21 ` Quentin Schulz
` (3 subsequent siblings)
4 siblings, 0 replies; 25+ messages in thread
From: Joel Stanley @ 2017-03-22 9:47 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Quentin Schulz, Geert Uytterhoeven, Marek Vasut,
Matt Ranostay, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Fabrice Gasnier, Jonathan Cameron,
Peter Meerwald-Stadler, Maxime Ripard, Jacopo Mondi
On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@google.com> wrote:
> Aspeed AST2400/AST2500 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>
Looks good Rick. I gave it a review from the perspective of the Aspeed
soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
worked, but uncovered some things that need addressing.
My device tree additions looked like this:
adc: adc@1e6e9000 {
compatible = "aspeed,ast2500-adc";
reg = <0x1e6e9000 0xb0>;
clocks = <&clk_apb>;
#io-channel-cells = <1>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_adc0_default>;
};
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>;
};
I got this output from lm-sensors when booted:
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.28 V
I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.80 V
ADC_12V_TW is the 12V rail sampled through a voltage divider. The
voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
738
738 / 1023 * 1.8 = 1.2975
Looks like the first channel is working! However our reference is
incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
It does hardcode 2500 in the aspeed_adc_read_raw callback:
case IIO_CHAN_INFO_SCALE:
*val = 2500; // mV
*val2 = 10;
return IIO_VAL_FRACTIONAL_LOG2;
Should this value the the constant you define?
Regardless, I don't think the reference voltage should be a constant.
This is going to vary from system to system. Can we put it in the
device tree? I notice other devices have vref-supply in their
bindings.
I noticed that in_voltage_scale is writable. However, it did not
accept any of the values I give it. Is this because we do not handle
it in aspeed_adc_write_raw?
I suggest we add the reference in the device tree bindings, and also
allow the value to be updated from userspace.
> ---
>
> 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 | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 282 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..9672d799a3fb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,16 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed AST2400/AST2500 ADC"
You could just say Aspeed ADC to save us having to update it when the
ast2600 comes out.
> + depends on ARCH_ASPEED || COMPILE_TEST
> + help
> + If you say yes here you get support for the Aspeed AST2400/AST2500
> + ADC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called aspeed_adc.
Don't forget to test compiling as a module.
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> new file mode 100644
> index 000000000000..9220909aefd4
> --- /dev/null
> +#define ASPEED_ADC_NUM_CHANNELS 16
> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
> +#define ASPEED_ADC_RESOLUTION_BITS 10
> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_ADC_REG_MAX 0xC0
> +
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
Nit: You could chose to label these with a shorter prefix. Drop the
aspeed or adc, or both.
> +
> +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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 2500; // mV
> + *val2 = 10;
What does 10 mean?
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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);
> +
> + switch (mask) {
Handle IIO_CHAN_INFO_SCALE here too.
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
> + val > ASPEED_ADC_MAX_SAMP_RATE)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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_ADC_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;
> + 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) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "Failed allocating device resources\n");
The function you're calling will do that for you
http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134
Just return the error here. I'd consider dropping the dev_errs for the
other cases in the probe. We still get a reasonable error message
without printing something ourselves. For example, when bailing out
with ENOMEM:
[ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12
> + ret = PTR_ERR(data->base);
> + goto resource_error;
> + }
> +
> + /* 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_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
I couldn't see any other drivers that use these functions outside of
drivers/clk. I like what you've done here, but someone who understands
the clock framework should take a look.
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> + ret = PTR_ERR(data->clk_prescaler);
> + goto prescaler_error;
> + }
> +
> + /*
> + * 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_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + 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);
Should we be able to enable just the channels that we want? Perhaps
only the ones that are requested through the device tree?
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Could't register the device.\n");
> + goto iio_register_error;
> + }
> +
> + return 0;
> +
> +iio_register_error:
> + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
Should this be done in remove as well?
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> +
> +scaler_error:
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> +prescaler_error:
> +resource_error:
> + return ret;
You could just return from the error where it happens in the case
where no cleanup is required.
> +}
> +
> +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);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc" },
> + { .compatible = "aspeed,ast2500-adc" },
> +};
This is missing a null entry to terminate.
> +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] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-22 9:47 ` Joel Stanley
0 siblings, 0 replies; 25+ messages in thread
From: Joel Stanley @ 2017-03-22 9:47 UTC (permalink / raw)
To: Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Quentin Schulz, Geert Uytterhoeven, Marek Vasut,
Matt Ranostay, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Fabrice Gasnier, Jonathan Cameron,
Peter Meerwald-Stadler, Maxime Ripard, Jacopo Mondi
On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@google.com> wrote:
> Aspeed AST2400/AST2500 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>
Looks good Rick. I gave it a review from the perspective of the Aspeed
soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
worked, but uncovered some things that need addressing.
My device tree additions looked like this:
adc: adc@1e6e9000 {
compatible = "aspeed,ast2500-adc";
reg = <0x1e6e9000 0xb0>;
clocks = <&clk_apb>;
#io-channel-cells = <1>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_adc0_default>;
};
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>;
};
I got this output from lm-sensors when booted:
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.28 V
I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
iio_hwmon-isa-0000
Adapter: ISA adapter
in1: +1.80 V
ADC_12V_TW is the 12V rail sampled through a voltage divider. The
voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
738
738 / 1023 * 1.8 = 1.2975
Looks like the first channel is working! However our reference is
incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
It does hardcode 2500 in the aspeed_adc_read_raw callback:
case IIO_CHAN_INFO_SCALE:
*val = 2500; // mV
*val2 = 10;
return IIO_VAL_FRACTIONAL_LOG2;
Should this value the the constant you define?
Regardless, I don't think the reference voltage should be a constant.
This is going to vary from system to system. Can we put it in the
device tree? I notice other devices have vref-supply in their
bindings.
I noticed that in_voltage_scale is writable. However, it did not
accept any of the values I give it. Is this because we do not handle
it in aspeed_adc_write_raw?
I suggest we add the reference in the device tree bindings, and also
allow the value to be updated from userspace.
> ---
>
> 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 | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 282 insertions(+)
> create mode 100644 drivers/iio/adc/aspeed_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6fb9865..9672d799a3fb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -130,6 +130,16 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ASPEED_ADC
> + tristate "Aspeed AST2400/AST2500 ADC"
You could just say Aspeed ADC to save us having to update it when the
ast2600 comes out.
> + depends on ARCH_ASPEED || COMPILE_TEST
> + help
> + If you say yes here you get support for the Aspeed AST2400/AST2500
> + ADC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called aspeed_adc.
Don't forget to test compiling as a module.
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> new file mode 100644
> index 000000000000..9220909aefd4
> --- /dev/null
> +#define ASPEED_ADC_NUM_CHANNELS 16
> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
> +#define ASPEED_ADC_RESOLUTION_BITS 10
> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
> +
> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_ADC_REG_MAX 0xC0
> +
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
> +
> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
Nit: You could chose to label these with a shorter prefix. Drop the
aspeed or adc, or both.
> +
> +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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(data->base + chan->address);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 2500; // mV
> + *val2 = 10;
What does 10 mean?
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(data->clk_scaler->clk) /
> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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);
> +
> + switch (mask) {
Handle IIO_CHAN_INFO_SCALE here too.
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
> + val > ASPEED_ADC_MAX_SAMP_RATE)
> + return -EINVAL;
> +
> + clk_set_rate(data->clk_scaler->clk,
> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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_ADC_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;
> + 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) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "Failed allocating device resources\n");
The function you're calling will do that for you
http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134
Just return the error here. I'd consider dropping the dev_errs for the
other cases in the probe. We still get a reasonable error message
without printing something ourselves. For example, when bailing out
with ENOMEM:
[ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12
> + ret = PTR_ERR(data->base);
> + goto resource_error;
> + }
> +
> + /* 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_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, &data->clk_lock);
I couldn't see any other drivers that use these functions outside of
drivers/clk. I like what you've done here, but someone who understands
the clock framework should take a look.
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> + ret = PTR_ERR(data->clk_prescaler);
> + goto prescaler_error;
> + }
> +
> + /*
> + * 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_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, &data->clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> + writel(adc_engine_control_reg_val,
> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + 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);
Should we be able to enable just the channels that we want? Perhaps
only the ones that are requested through the device tree?
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Could't register the device.\n");
> + goto iio_register_error;
> + }
> +
> + return 0;
> +
> +iio_register_error:
> + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
Should this be done in remove as well?
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> +
> +scaler_error:
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> +prescaler_error:
> +resource_error:
> + return ret;
You could just return from the error where it happens in the case
where no cleanup is required.
> +}
> +
> +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);
> + clk_disable_unprepare(data->clk_scaler->clk);
> + clk_hw_unregister_divider(data->clk_scaler);
> + clk_hw_unregister_divider(data->clk_prescaler);
> +
> + return 0;
> +}
> +
> +const struct of_device_id aspeed_adc_matches[] = {
> + { .compatible = "aspeed,ast2400-adc" },
> + { .compatible = "aspeed,ast2500-adc" },
> +};
This is missing a null entry to terminate.
> +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] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-22 7:21 ` Quentin Schulz
@ 2017-03-22 10:08 ` Joel Stanley
[not found] ` <CAPLgG=kVsNake71fFLc2NsoMtrtG0_6Fb6XHep3dQKVuRgOmbA@mail.gmail.com>
1 sibling, 0 replies; 25+ messages in thread
From: Joel Stanley @ 2017-03-22 10:08 UTC (permalink / raw)
To: Quentin Schulz
Cc: Rick Altherr, OpenBMC Maillist, Linux Kernel Mailing List,
Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, Hartmut Knaack, Geert Uytterhoeven,
Marek Vasut, Matt Ranostay, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Fabrice Gasnier,
Jonathan Cameron, Peter Meerwald-Stadler, Maxime Ripard,
Jacopo Mondi
Hello Quentin,
On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
>> +
>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (_idx), \
>> + .address = (_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_ADC_CHAN(0, 0x10),
>> + ASPEED_ADC_CHAN(1, 0x12),
>> + ASPEED_ADC_CHAN(2, 0x14),
>> + ASPEED_ADC_CHAN(3, 0x16),
>> + ASPEED_ADC_CHAN(4, 0x18),
>> + ASPEED_ADC_CHAN(5, 0x1A),
>> + ASPEED_ADC_CHAN(6, 0x1C),
>> + ASPEED_ADC_CHAN(7, 0x1E),
>> + ASPEED_ADC_CHAN(8, 0x20),
>> + ASPEED_ADC_CHAN(9, 0x22),
>> + ASPEED_ADC_CHAN(10, 0x24),
>> + ASPEED_ADC_CHAN(11, 0x26),
>> + ASPEED_ADC_CHAN(12, 0x28),
>> + ASPEED_ADC_CHAN(13, 0x2A),
>> + ASPEED_ADC_CHAN(14, 0x2C),
>> + ASPEED_ADC_CHAN(15, 0x2E),
>
> It would make sense to name the registers (the _addr parameter of your
> macro) so it's easier to understand what it refers to.
I appreciate the desire to not have magic values. However, I think
these are okay. We don't use them anywhere else, and it is obvious
from reading that they are the registers containing the ADC values for
each channel.
The alternative would look like this:
+ ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA),
+ ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA),
Which doesn't really help me as someone reading the code.
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>> + adc_engine_control_reg_val = GENMASK(31, 16) |
>> + ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>> + writel(adc_engine_control_reg_val,
>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> +
>> + indio_dev->name = dev_name(&pdev->dev);
>
> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
> of the mail in the probe function). Better name it aspeed-adc or even
> better to have a different name per compatible: ast2400-adc or ast2500-adc.
The label comes out as "adc.1e6e9000". This is the reg property and
the node name from the device tree, which seems sensible, even if it
is a bit strange to be grabbing the name of the parent device for it.
Could the iio core fill in a sensible name for us here? Rick is the
31st person to make this mistake, so it would be nice to fix properly.
Cheers,
Joel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-22 10:08 ` Joel Stanley
0 siblings, 0 replies; 25+ messages in thread
From: Joel Stanley @ 2017-03-22 10:08 UTC (permalink / raw)
To: Quentin Schulz
Cc: Rick Altherr, OpenBMC Maillist, Linux Kernel Mailing List,
Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, Hartmut Knaack, Geert Uytterhoeven,
Marek Vasut, Matt Ranostay, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Fabrice Gasnier,
Jonathan Cameron, Peter Meerwald-Stadler, Maxime Ripard,
Jacopo Mondi
Hello Quentin,
On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
>> +
>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (_idx), \
>> + .address = (_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_ADC_CHAN(0, 0x10),
>> + ASPEED_ADC_CHAN(1, 0x12),
>> + ASPEED_ADC_CHAN(2, 0x14),
>> + ASPEED_ADC_CHAN(3, 0x16),
>> + ASPEED_ADC_CHAN(4, 0x18),
>> + ASPEED_ADC_CHAN(5, 0x1A),
>> + ASPEED_ADC_CHAN(6, 0x1C),
>> + ASPEED_ADC_CHAN(7, 0x1E),
>> + ASPEED_ADC_CHAN(8, 0x20),
>> + ASPEED_ADC_CHAN(9, 0x22),
>> + ASPEED_ADC_CHAN(10, 0x24),
>> + ASPEED_ADC_CHAN(11, 0x26),
>> + ASPEED_ADC_CHAN(12, 0x28),
>> + ASPEED_ADC_CHAN(13, 0x2A),
>> + ASPEED_ADC_CHAN(14, 0x2C),
>> + ASPEED_ADC_CHAN(15, 0x2E),
>
> It would make sense to name the registers (the _addr parameter of your
> macro) so it's easier to understand what it refers to.
I appreciate the desire to not have magic values. However, I think
these are okay. We don't use them anywhere else, and it is obvious
from reading that they are the registers containing the ADC values for
each channel.
The alternative would look like this:
+ ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA),
+ ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA),
Which doesn't really help me as someone reading the code.
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>> + adc_engine_control_reg_val = GENMASK(31, 16) |
>> + ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>> + writel(adc_engine_control_reg_val,
>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> +
>> + indio_dev->name = dev_name(&pdev->dev);
>
> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
> of the mail in the probe function). Better name it aspeed-adc or even
> better to have a different name per compatible: ast2400-adc or ast2500-adc.
The label comes out as "adc.1e6e9000". This is the reg property and
the node name from the device tree, which seems sensible, even if it
is a bit strange to be grabbing the name of the parent device for it.
Could the iio core fill in a sensible name for us here? Rick is the
31st person to make this mistake, so it would be nice to fix properly.
Cheers,
Joel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
` (2 preceding siblings ...)
2017-03-22 9:47 ` Joel Stanley
@ 2017-03-23 2:42 ` kbuild test robot
2017-03-23 5:52 ` kbuild test robot
4 siblings, 0 replies; 25+ messages in thread
From: kbuild test robot @ 2017-03-23 2:42 UTC (permalink / raw)
To: Rick Altherr
Cc: kbuild-all, openbmc, linux-kernel, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Quentin Schulz, Geert Uytterhoeven, Marek Vasut,
Matt Ranostay, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Fabrice Gasnier, Jonathan Cameron,
Peter Meerwald-Stadler, Maxime Ripard, Jacopo Mondi
[-- Attachment #1: Type: text/plain, Size: 7620 bytes --]
Hi Rick,
[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.11-rc3 next-20170322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rick-Altherr/Documentation-dt-bindings-Document-bindings-for-Aspeed-AST2400-AST2500-ADC/20170323-093517
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All error/warnings (new ones prefixed by >>):
drivers/iio/adc/aspeed_adc.c: In function 'aspeed_adc_read_raw':
>> drivers/iio/adc/aspeed_adc.c:100:39: error: dereferencing pointer to incomplete type 'struct clk_hw'
*val = clk_get_rate(data->clk_scaler->clk) /
^~
drivers/iio/adc/aspeed_adc.c: In function 'aspeed_adc_probe':
>> drivers/iio/adc/aspeed_adc.c:177:20: error: implicit declaration of function 'of_clk_get_parent_name' [-Werror=implicit-function-declaration]
clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
^~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/aspeed_adc.c:177:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
^
>> drivers/iio/adc/aspeed_adc.c:179:24: error: implicit declaration of function 'clk_hw_register_divider' [-Werror=implicit-function-declaration]
data->clk_prescaler = clk_hw_register_divider(
^~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/adc/aspeed_adc.c:179:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
data->clk_prescaler = clk_hw_register_divider(
^
>> drivers/iio/adc/aspeed_adc.c:195:5: error: 'CLK_SET_RATE_PARENT' undeclared (first use in this function)
CLK_SET_RATE_PARENT,
^~~~~~~~~~~~~~~~~~~
drivers/iio/adc/aspeed_adc.c:195:5: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iio/adc/aspeed_adc.c:229:2: error: implicit declaration of function 'clk_hw_unregister_divider' [-Werror=implicit-function-declaration]
clk_hw_unregister_divider(data->clk_scaler);
^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +100 drivers/iio/adc/aspeed_adc.c
94 case IIO_CHAN_INFO_SCALE:
95 *val = 2500; // mV
96 *val2 = 10;
97 return IIO_VAL_FRACTIONAL_LOG2;
98
99 case IIO_CHAN_INFO_SAMP_FREQ:
> 100 *val = clk_get_rate(data->clk_scaler->clk) /
101 ASPEED_ADC_CLOCKS_PER_SAMPLE;
102 return IIO_VAL_INT;
103
104 default:
105 return -EINVAL;
106 }
107 }
108
109 static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
110 struct iio_chan_spec const *chan,
111 int val, int val2, long mask)
112 {
113 struct aspeed_adc_data *data = iio_priv(indio_dev);
114
115 switch (mask) {
116 case IIO_CHAN_INFO_SAMP_FREQ:
117 if (val < ASPEED_ADC_MIN_SAMP_RATE ||
118 val > ASPEED_ADC_MAX_SAMP_RATE)
119 return -EINVAL;
120
121 clk_set_rate(data->clk_scaler->clk,
122 val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
123 return 0;
124
125 default:
126 return -EINVAL;
127 }
128 }
129
130 static int aspeed_adc_reg_access(struct iio_dev *indio_dev,
131 unsigned int reg, unsigned int writeval,
132 unsigned int *readval)
133 {
134 struct aspeed_adc_data *data = iio_priv(indio_dev);
135
136 if (!readval || reg % 4 || reg > ASPEED_ADC_REG_MAX)
137 return -EINVAL;
138
139 *readval = readl(data->base + reg);
140 return 0;
141 }
142
143 static const struct iio_info aspeed_adc_iio_info = {
144 .driver_module = THIS_MODULE,
145 .read_raw = &aspeed_adc_read_raw,
146 .write_raw = &aspeed_adc_write_raw,
147 .debugfs_reg_access = &aspeed_adc_reg_access,
148 };
149
150 static int aspeed_adc_probe(struct platform_device *pdev)
151 {
152 struct iio_dev *indio_dev;
153 struct aspeed_adc_data *data;
154 struct resource *res;
155 const char *clk_parent_name;
156 int ret;
157 u32 adc_engine_control_reg_val;
158
159 indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
160 if (!indio_dev) {
161 dev_err(&pdev->dev, "Failed allocating iio device\n");
162 return -ENOMEM;
163 }
164
165 data = iio_priv(indio_dev);
166
167 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
168 data->base = devm_ioremap_resource(&pdev->dev, res);
169 if (IS_ERR(data->base)) {
170 dev_err(&pdev->dev, "Failed allocating device resources\n");
171 ret = PTR_ERR(data->base);
172 goto resource_error;
173 }
174
175 /* Register ADC clock prescaler with source specified by device tree. */
176 spin_lock_init(&data->clk_lock);
> 177 clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
178
> 179 data->clk_prescaler = clk_hw_register_divider(
180 &pdev->dev, "prescaler", clk_parent_name, 0,
181 data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
182 17, 15, 0, &data->clk_lock);
183 if (IS_ERR(data->clk_prescaler)) {
184 dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
185 ret = PTR_ERR(data->clk_prescaler);
186 goto prescaler_error;
187 }
188
189 /*
190 * Register ADC clock scaler downstream from the prescaler. Allow rate
191 * setting to adjust the prescaler as well.
192 */
193 data->clk_scaler = clk_hw_register_divider(
194 &pdev->dev, "scaler", "prescaler",
> 195 CLK_SET_RATE_PARENT,
196 data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
197 0, 10, 0, &data->clk_lock);
198 if (IS_ERR(data->clk_scaler)) {
199 dev_err(&pdev->dev, "Failed allocating scaler clock\n");
200 ret = PTR_ERR(data->clk_scaler);
201 goto scaler_error;
202 }
203
204 /* Start all channels in normal mode. */
205 clk_prepare_enable(data->clk_scaler->clk);
206 adc_engine_control_reg_val = GENMASK(31, 16) |
207 ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
208 writel(adc_engine_control_reg_val,
209 data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
210
211 indio_dev->name = dev_name(&pdev->dev);
212 indio_dev->dev.parent = &pdev->dev;
213 indio_dev->info = &aspeed_adc_iio_info;
214 indio_dev->modes = INDIO_DIRECT_MODE;
215 indio_dev->channels = aspeed_adc_iio_channels;
216 indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
217
218 ret = iio_device_register(indio_dev);
219 if (ret) {
220 dev_err(&pdev->dev, "Could't register the device.\n");
221 goto iio_register_error;
222 }
223
224 return 0;
225
226 iio_register_error:
227 writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
228 clk_disable_unprepare(data->clk_scaler->clk);
> 229 clk_hw_unregister_divider(data->clk_scaler);
230
231 scaler_error:
232 clk_hw_unregister_divider(data->clk_prescaler);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45930 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
` (3 preceding siblings ...)
2017-03-23 2:42 ` kbuild test robot
@ 2017-03-23 5:52 ` kbuild test robot
4 siblings, 0 replies; 25+ messages in thread
From: kbuild test robot @ 2017-03-23 5:52 UTC (permalink / raw)
To: Rick Altherr
Cc: kbuild-all, openbmc, linux-kernel, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Quentin Schulz, Geert Uytterhoeven, Marek Vasut,
Matt Ranostay, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Fabrice Gasnier, Jonathan Cameron,
Peter Meerwald-Stadler, Maxime Ripard, Jacopo Mondi
[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]
Hi Rick,
[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.11-rc3 next-20170322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rick-Altherr/Documentation-dt-bindings-Document-bindings-for-Aspeed-AST2400-AST2500-ADC/20170323-093517
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/iio/adc/aspeed_adc: struct of_device_id is 196 bytes. The last of 2 is:
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x61 0x73 0x70 0x65 0x65 0x64 0x2c 0x61 0x73 0x74 0x32 0x35 0x30 0x30 0x2d 0x61 0x64 0x63 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>> FATAL: drivers/iio/adc/aspeed_adc: struct of_device_id is not terminated with a NULL entry!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57988 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
[not found] ` <CAPLgG=kVsNake71fFLc2NsoMtrtG0_6Fb6XHep3dQKVuRgOmbA@mail.gmail.com>
@ 2017-03-23 7:52 ` Quentin Schulz
2017-03-23 16:57 ` Rick Altherr
0 siblings, 1 reply; 25+ messages in thread
From: Quentin Schulz @ 2017-03-23 7:52 UTC (permalink / raw)
To: Rick Altherr, openbmc, linux-kernel
Cc: Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, knaack.h, geert, marek.vasut+renesas,
Matt Ranostay, Lars-Peter Clausen, leonard.crestez, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
Hi,
On 22/03/2017 21:46, Rick Altherr wrote:
> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> Hi,
>>
>> On 21/03/2017 21:48, Rick Altherr wrote:
>>> Aspeed AST2400/AST2500 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>
>>> ---
>>>
>>> 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
>>>
[...]
>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .channel = (_idx), \
>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>> + ASPEED_ADC_CHAN(1, 0x12),
>>> + ASPEED_ADC_CHAN(2, 0x14),
>>> + ASPEED_ADC_CHAN(3, 0x16),
>>> + ASPEED_ADC_CHAN(4, 0x18),
>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>> + ASPEED_ADC_CHAN(8, 0x20),
>>> + ASPEED_ADC_CHAN(9, 0x22),
>>> + ASPEED_ADC_CHAN(10, 0x24),
>>> + ASPEED_ADC_CHAN(11, 0x26),
>>> + ASPEED_ADC_CHAN(12, 0x28),
>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>> + ASPEED_ADC_CHAN(15, 0x2E),
>>
>> It would make sense to name the registers (the _addr parameter of your
>> macro) so it's easier to understand what it refers to.
>>
>
> I agree with Joel on this. There isn't really a better name than
> ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to
> make that clearer.
>
Is it the name in the datasheet?
[...]
>>> + indio_dev->name = dev_name(&pdev->dev);
>>
>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
>> of the mail in the probe function). Better name it aspeed-adc or even
>> better to have a different name per compatible: ast2400-adc or ast2500-adc.
>
> Ack. Will use aspeed-adc to avoid parsing the compatible match and
> stripping the 'aspeed,' prefix.
>
You don't need to parse the compatible match. You could use the data
void pointer in your struct of_device_id
(http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234)
like it's done here: https://lkml.org/lkml/2017/3/21/675
(sun4i_gpadc_of_id).
Note: please reply to all :)
Quentin
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
[not found] ` <CAPLgG=kVewvVzcrWMzK+XyNWLjiEs6nr_hykfC2bbdpeLsf4aw@mail.gmail.com>
@ 2017-03-23 16:54 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:54 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Maxime Ripard, Jacopo Mondi
Restoring the list after an accidental direct reply.
On Wed, Mar 22, 2017 at 1:30 PM, Rick Altherr <raltherr@google.com> wrote:
> On Tue, Mar 21, 2017 at 2:14 PM, Peter Meerwald-Stadler
> <pmeerw@pmeerw.net> wrote:
>>
>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
>>> and high threshold interrupts are supported by the hardware but are not
>>> currently implemented.
>>
>> comments below
>> link to a datasheet would be nice
>>
>
> No public datasheet is available. I've tried.
>
>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>> ---
>>>
>>> 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 | 10 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 282 insertions(+)
>>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 2268a6fb9865..9672d799a3fb 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -130,6 +130,16 @@ config AD799X
>>> To compile this driver as a module, choose M here: the module will be
>>> called ad799x.
>>>
>>> +config ASPEED_ADC
>>> + tristate "Aspeed AST2400/AST2500 ADC"
>>> + depends on ARCH_ASPEED || COMPILE_TEST
>>> + help
>>> + If you say yes here you get support for the Aspeed AST2400/AST2500
>>> + ADC.
>>> +
>>> + 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..9220909aefd4
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/aspeed_adc.c
>>> @@ -0,0 +1,271 @@
>>> +/*
>>> + * 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_ADC_NUM_CHANNELS 16
>>
>> _NUM_CHANNELS is not used, remove
>
> Ack
>
>>
>>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>>
>> should be used below
>
> Ack
>
>>
>>> +#define ASPEED_ADC_RESOLUTION_BITS 10
>>
>> should be used below
>
> Ack
>
>>
>>> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
>>> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
>>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
>>> +
>>> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
>>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
>>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
>>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
>>> +#define ASPEED_ADC_REG_MAX 0xC0
>>> +
>>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
>>> +
>>> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
>>> +
>>> +struct aspeed_adc_data {
>>> + struct device *dev;
>>> + void __iomem *base;
>>> +
>>> + spinlock_t clk_lock;
>>> + struct clk_hw *clk_prescaler;
>>> + struct clk_hw *clk_scaler;
>>> +};
>>> +
>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .channel = (_idx), \
>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>> + ASPEED_ADC_CHAN(1, 0x12),
>>> + ASPEED_ADC_CHAN(2, 0x14),
>>> + ASPEED_ADC_CHAN(3, 0x16),
>>> + ASPEED_ADC_CHAN(4, 0x18),
>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>> + ASPEED_ADC_CHAN(8, 0x20),
>>> + ASPEED_ADC_CHAN(9, 0x22),
>>> + ASPEED_ADC_CHAN(10, 0x24),
>>> + ASPEED_ADC_CHAN(11, 0x26),
>>> + ASPEED_ADC_CHAN(12, 0x28),
>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>> + ASPEED_ADC_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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + *val = readw(data->base + chan->address);
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = 2500; // mV
>>
>> REF_VOLTAGE?
>
> Ack
>
>>
>>> + *val2 = 10;
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + *val = clk_get_rate(data->clk_scaler->clk) /
>>> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
>>> + return IIO_VAL_INT;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
>>> + val > ASPEED_ADC_MAX_SAMP_RATE)
>>> + return -EINVAL;
>>> +
>>> + clk_set_rate(data->clk_scaler->clk,
>>> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
>>> + return 0;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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_ADC_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,
>>
>> without & (matter of taste)
>
> Ack
>
>>
>>> + .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;
>>> + 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) {
>>> + dev_err(&pdev->dev, "Failed allocating iio device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + data = iio_priv(indio_dev);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(data->base)) {
>>> + dev_err(&pdev->dev, "Failed allocating device resources\n");
>>> + ret = PTR_ERR(data->base);
>>> + goto resource_error;
>>
>> just return here?
>
> Ack
>
>>
>>> + }
>>> +
>>> + /* 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_ADC_REG_CLOCK_CONTROL,
>>> + 17, 15, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_prescaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
>>> + ret = PTR_ERR(data->clk_prescaler);
>>> + goto prescaler_error;
>>
>> just return here?
>
> Ack
>
>>
>>> + }
>>> +
>>> + /*
>>> + * 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_ADC_REG_CLOCK_CONTROL,
>>> + 0, 10, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_scaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
>>> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>>> + writel(adc_engine_control_reg_val,
>>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>> +
>>> + indio_dev->name = dev_name(&pdev->dev);
>>> + 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) {
>>> + dev_err(&pdev->dev, "Could't register the device.\n");
>>
>> Couldn't
>
> Ack
>
>>
>>> + goto iio_register_error;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +iio_register_error:
>>> + writel(0x0, data->base + ASPEED_ADC_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);
>>> +
>>> +prescaler_error:
>>> +resource_error:
>>> + 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(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> I guess this power off the device?
>
> Yes. You're right that it makes sense to do that during removal as well.
>
>>
>> the MODE #defines are now used (powerdown, normal, etc.)
>
> NORMAL is used in probe(). I'll use powerdown here.
>
>>
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const struct of_device_id aspeed_adc_matches[] = {
>>> + { .compatible = "aspeed,ast2400-adc" },
>>> + { .compatible = "aspeed,ast2500-adc" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>>> +
>>> +static struct platform_driver aspeed_adc_driver = {
>>> + .probe = aspeed_adc_probe,
>>> + .remove = aspeed_adc_remove,
>>> + .driver = {
>>> + .name = KBUILD_MODNAME,
>>> + .of_match_table = aspeed_adc_matches,
>>> + }
>>> +};
>>> +
>>> +module_platform_driver(aspeed_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>> --
>>
>> Peter Meerwald-Stadler
>> Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-23 16:54 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:54 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Maxime Ripard, Jacopo Mondi
Restoring the list after an accidental direct reply.
On Wed, Mar 22, 2017 at 1:30 PM, Rick Altherr <raltherr@google.com> wrote:
> On Tue, Mar 21, 2017 at 2:14 PM, Peter Meerwald-Stadler
> <pmeerw@pmeerw.net> wrote:
>>
>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
>>> and high threshold interrupts are supported by the hardware but are not
>>> currently implemented.
>>
>> comments below
>> link to a datasheet would be nice
>>
>
> No public datasheet is available. I've tried.
>
>>> Signed-off-by: Rick Altherr <raltherr@google.com>
>>> ---
>>>
>>> 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 | 10 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 282 insertions(+)
>>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 2268a6fb9865..9672d799a3fb 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -130,6 +130,16 @@ config AD799X
>>> To compile this driver as a module, choose M here: the module will be
>>> called ad799x.
>>>
>>> +config ASPEED_ADC
>>> + tristate "Aspeed AST2400/AST2500 ADC"
>>> + depends on ARCH_ASPEED || COMPILE_TEST
>>> + help
>>> + If you say yes here you get support for the Aspeed AST2400/AST2500
>>> + ADC.
>>> +
>>> + 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..9220909aefd4
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/aspeed_adc.c
>>> @@ -0,0 +1,271 @@
>>> +/*
>>> + * 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_ADC_NUM_CHANNELS 16
>>
>> _NUM_CHANNELS is not used, remove
>
> Ack
>
>>
>>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>>
>> should be used below
>
> Ack
>
>>
>>> +#define ASPEED_ADC_RESOLUTION_BITS 10
>>
>> should be used below
>
> Ack
>
>>
>>> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
>>> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
>>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
>>> +
>>> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
>>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
>>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
>>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
>>> +#define ASPEED_ADC_REG_MAX 0xC0
>>> +
>>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
>>> +
>>> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
>>> +
>>> +struct aspeed_adc_data {
>>> + struct device *dev;
>>> + void __iomem *base;
>>> +
>>> + spinlock_t clk_lock;
>>> + struct clk_hw *clk_prescaler;
>>> + struct clk_hw *clk_scaler;
>>> +};
>>> +
>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .channel = (_idx), \
>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>> + ASPEED_ADC_CHAN(1, 0x12),
>>> + ASPEED_ADC_CHAN(2, 0x14),
>>> + ASPEED_ADC_CHAN(3, 0x16),
>>> + ASPEED_ADC_CHAN(4, 0x18),
>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>> + ASPEED_ADC_CHAN(8, 0x20),
>>> + ASPEED_ADC_CHAN(9, 0x22),
>>> + ASPEED_ADC_CHAN(10, 0x24),
>>> + ASPEED_ADC_CHAN(11, 0x26),
>>> + ASPEED_ADC_CHAN(12, 0x28),
>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>> + ASPEED_ADC_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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + *val = readw(data->base + chan->address);
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = 2500; // mV
>>
>> REF_VOLTAGE?
>
> Ack
>
>>
>>> + *val2 = 10;
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + *val = clk_get_rate(data->clk_scaler->clk) /
>>> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
>>> + return IIO_VAL_INT;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
>>> + val > ASPEED_ADC_MAX_SAMP_RATE)
>>> + return -EINVAL;
>>> +
>>> + clk_set_rate(data->clk_scaler->clk,
>>> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
>>> + return 0;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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_ADC_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,
>>
>> without & (matter of taste)
>
> Ack
>
>>
>>> + .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;
>>> + 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) {
>>> + dev_err(&pdev->dev, "Failed allocating iio device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + data = iio_priv(indio_dev);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(data->base)) {
>>> + dev_err(&pdev->dev, "Failed allocating device resources\n");
>>> + ret = PTR_ERR(data->base);
>>> + goto resource_error;
>>
>> just return here?
>
> Ack
>
>>
>>> + }
>>> +
>>> + /* 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_ADC_REG_CLOCK_CONTROL,
>>> + 17, 15, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_prescaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
>>> + ret = PTR_ERR(data->clk_prescaler);
>>> + goto prescaler_error;
>>
>> just return here?
>
> Ack
>
>>
>>> + }
>>> +
>>> + /*
>>> + * 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_ADC_REG_CLOCK_CONTROL,
>>> + 0, 10, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_scaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
>>> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>>> + writel(adc_engine_control_reg_val,
>>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>> +
>>> + indio_dev->name = dev_name(&pdev->dev);
>>> + 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) {
>>> + dev_err(&pdev->dev, "Could't register the device.\n");
>>
>> Couldn't
>
> Ack
>
>>
>>> + goto iio_register_error;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +iio_register_error:
>>> + writel(0x0, data->base + ASPEED_ADC_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);
>>> +
>>> +prescaler_error:
>>> +resource_error:
>>> + 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(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> I guess this power off the device?
>
> Yes. You're right that it makes sense to do that during removal as well.
>
>>
>> the MODE #defines are now used (powerdown, normal, etc.)
>
> NORMAL is used in probe(). I'll use powerdown here.
>
>>
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const struct of_device_id aspeed_adc_matches[] = {
>>> + { .compatible = "aspeed,ast2400-adc" },
>>> + { .compatible = "aspeed,ast2500-adc" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
>>> +
>>> +static struct platform_driver aspeed_adc_driver = {
>>> + .probe = aspeed_adc_probe,
>>> + .remove = aspeed_adc_remove,
>>> + .driver = {
>>> + .name = KBUILD_MODNAME,
>>> + .of_match_table = aspeed_adc_matches,
>>> + }
>>> +};
>>> +
>>> +module_platform_driver(aspeed_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
>>> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>> --
>>
>> Peter Meerwald-Stadler
>> Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
[not found] ` <CAPLgG=m3ex1dX+Xu8DFLb+DfTn3e-r0=Ln82Z1br6bSinEAcDQ@mail.gmail.com>
@ 2017-03-23 16:54 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:54 UTC (permalink / raw)
To: Joel Stanley
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
Restoring the list after an accidental direct reply.
On Wed, Mar 22, 2017 at 2:32 PM, Rick Altherr <raltherr@google.com> wrote:
> On Wed, Mar 22, 2017 at 2:47 AM, Joel Stanley <joel@jms.id.au> wrote:
>> On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@google.com> wrote:
>>> Aspeed AST2400/AST2500 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>
>>
>> Looks good Rick. I gave it a review from the perspective of the Aspeed
>> soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
>> worked, but uncovered some things that need addressing.
>>
>> My device tree additions looked like this:
>>
>> adc: adc@1e6e9000 {
>> compatible = "aspeed,ast2500-adc";
>> reg = <0x1e6e9000 0xb0>;
>> clocks = <&clk_apb>;
>> #io-channel-cells = <1>;
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_adc0_default>;
>
> This shouldn't be strictly necessary as the ADCs are the default
> function for the pins they are available on.
>
>> };
>>
>> iio-hwmon {
>> compatible = "iio-hwmon";
>> io-channels = <&adc 0>;
>> };
>
> This is necessary to make it show up as hwmon. You can see the iio
> device directly at /sys/bus/iio/devices/.....
>
>>
>> I got this output from lm-sensors when booted:
>>
>> iio_hwmon-isa-0000
>> Adapter: ISA adapter
>> in1: +1.28 V
>>
>> I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
>>
>> iio_hwmon-isa-0000
>> Adapter: ISA adapter
>> in1: +1.80 V
>>
>> ADC_12V_TW is the 12V rail sampled through a voltage divider. The
>> voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
>>
>> cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
>> 738
>>
>> 738 / 1023 * 1.8 = 1.2975
>>
>> Looks like the first channel is working! However our reference is
>> incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
>> It does hardcode 2500 in the aspeed_adc_read_raw callback:
>>
>> case IIO_CHAN_INFO_SCALE:
>> *val = 2500; // mV
>> *val2 = 10;
>> return IIO_VAL_FRACTIONAL_LOG2;
>>
>> Should this value the the constant you define?
>>
>> Regardless, I don't think the reference voltage should be a constant.
>> This is going to vary from system to system. Can we put it in the
>> device tree? I notice other devices have vref-supply in their
>> bindings.
>>
>
> Good catch. AST2500 uses a 1.8Vref while AST2400 uses a 2.5Vref. I
> decided against putting a vref-supply in the device tree as neither
> part allows for an external reference. Both the 1.8V and 2.5V are
> fixed, internal references. It just happens that the reference
> voltage changes with the chip generation. Looking at the data sheet
> more, I also see the min/max sampling rate has changed for AST2500 as
> well. This is probably best handled by attaching data to the
> of_device_ids in the of_match_table. That would solve all of these
> per-chip variations.
>
>> I noticed that in_voltage_scale is writable. However, it did not
>> accept any of the values I give it. Is this because we do not handle
>> it in aspeed_adc_write_raw?
>>
>
> I think all attributes become writable because I implement write_raw
> even though only the sample frequency is actually writable. I'm of
> two minds on allowing the scale to be written. If the user knows
> there is a voltage divider before the ADC, why don't they apply that
> correction in userspace? On the other hand, the existence of the
> voltage divider _could_ be specified in the device tree and the kernel
> takes care of the scaling entirely. The latter seems like a
> general-purpose concept but I couldn't find an existing binding for
> specifying it. I decided to start with the minimal functionality of
> only reporting the ADC's natural scaling and letting userspace deal
> with any additional information it has.
>
>> I suggest we add the reference in the device tree bindings, and also
>> allow the value to be updated from userspace.
>>
>>> ---
>>>
>>> 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 | 10 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 282 insertions(+)
>>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 2268a6fb9865..9672d799a3fb 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -130,6 +130,16 @@ config AD799X
>>> To compile this driver as a module, choose M here: the module will be
>>> called ad799x.
>>>
>>> +config ASPEED_ADC
>>> + tristate "Aspeed AST2400/AST2500 ADC"
>>
>> You could just say Aspeed ADC to save us having to update it when the
>> ast2600 comes out.
>
> Ack
>
>>
>>> + depends on ARCH_ASPEED || COMPILE_TEST
>>> + help
>>> + If you say yes here you get support for the Aspeed AST2400/AST2500
>>> + ADC.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called aspeed_adc.
>>
>> Don't forget to test compiling as a module.
>
> Ack
>
>>
>>
>>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>>> new file mode 100644
>>> index 000000000000..9220909aefd4
>>> --- /dev/null
>>
>>> +#define ASPEED_ADC_NUM_CHANNELS 16
>>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>>> +#define ASPEED_ADC_RESOLUTION_BITS 10
>>> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
>>> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
>>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
>>> +
>>> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
>>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
>>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
>>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
>>> +#define ASPEED_ADC_REG_MAX 0xC0
>>> +
>>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
>>> +
>>> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
>>
>> Nit: You could chose to label these with a shorter prefix. Drop the
>> aspeed or adc, or both.
>
> Ack
>
>>
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + *val = readw(data->base + chan->address);
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = 2500; // mV
>>> + *val2 = 10;
>>
>> What does 10 mean?
>>
>
> ADC resolution in bits. I'll use the constant defined above.
>
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + *val = clk_get_rate(data->clk_scaler->clk) /
>>> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
>>> + return IIO_VAL_INT;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>
>> Handle IIO_CHAN_INFO_SCALE here too.
>
> See above reply.
>
>>
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
>>> + val > ASPEED_ADC_MAX_SAMP_RATE)
>>> + return -EINVAL;
>>> +
>>> + clk_set_rate(data->clk_scaler->clk,
>>> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
>>> + return 0;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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_ADC_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;
>>> + 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) {
>>> + dev_err(&pdev->dev, "Failed allocating iio device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + data = iio_priv(indio_dev);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(data->base)) {
>>> + dev_err(&pdev->dev, "Failed allocating device resources\n");
>>
>> The function you're calling will do that for you
>>
>> http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134
>>
>> Just return the error here. I'd consider dropping the dev_errs for the
>> other cases in the probe. We still get a reasonable error message
>> without printing something ourselves. For example, when bailing out
>> with ENOMEM:
>>
>> [ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12
>>
>
> Ack
>
>>
>>> + ret = PTR_ERR(data->base);
>>> + goto resource_error;
>>> + }
>>> +
>>> + /* 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_ADC_REG_CLOCK_CONTROL,
>>> + 17, 15, 0, &data->clk_lock);
>>
>> I couldn't see any other drivers that use these functions outside of
>> drivers/clk. I like what you've done here, but someone who understands
>> the clock framework should take a look.
>>
>
> I'll CC one of the clock maintainers in the next version.
>
>>
>>> + if (IS_ERR(data->clk_prescaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
>>> + ret = PTR_ERR(data->clk_prescaler);
>>> + goto prescaler_error;
>>> + }
>>> +
>>> + /*
>>> + * 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_ADC_REG_CLOCK_CONTROL,
>>> + 0, 10, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_scaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
>>> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>>> + writel(adc_engine_control_reg_val,
>>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>> +
>>> + indio_dev->name = dev_name(&pdev->dev);
>>> + 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);
>>
>> Should we be able to enable just the channels that we want? Perhaps
>> only the ones that are requested through the device tree?
>>
>
> It shouldn't matter. When the pins are muxed to other functions, the
> ADCs will read zero. The only potential advantage is increased
> sampling speed by omitting unused channels in the scan. I'm not
> concerned with that now and the code is much simpler by not dealing
> with per-channel enables.
>
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Could't register the device.\n");
>>> + goto iio_register_error;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +iio_register_error:
>>> + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>
>> Should this be done in remove as well?
>>
> Yes.
>
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> +
>>> +scaler_error:
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> +prescaler_error:
>>> +resource_error:
>>> + return ret;
>>
>> You could just return from the error where it happens in the case
>> where no cleanup is required.
>>
>
> Ack.
>
>>> +}
>>> +
>>> +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);
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const struct of_device_id aspeed_adc_matches[] = {
>>> + { .compatible = "aspeed,ast2400-adc" },
>>> + { .compatible = "aspeed,ast2500-adc" },
>>> +};
>>
>> This is missing a null entry to terminate.
>
> Ack
>
>>
>>> +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] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-23 16:54 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:54 UTC (permalink / raw)
To: Joel Stanley
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
Restoring the list after an accidental direct reply.
On Wed, Mar 22, 2017 at 2:32 PM, Rick Altherr <raltherr@google.com> wrote:
> On Wed, Mar 22, 2017 at 2:47 AM, Joel Stanley <joel@jms.id.au> wrote:
>> On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@google.com> wrote:
>>> Aspeed AST2400/AST2500 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>
>>
>> Looks good Rick. I gave it a review from the perspective of the Aspeed
>> soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
>> worked, but uncovered some things that need addressing.
>>
>> My device tree additions looked like this:
>>
>> adc: adc@1e6e9000 {
>> compatible = "aspeed,ast2500-adc";
>> reg = <0x1e6e9000 0xb0>;
>> clocks = <&clk_apb>;
>> #io-channel-cells = <1>;
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_adc0_default>;
>
> This shouldn't be strictly necessary as the ADCs are the default
> function for the pins they are available on.
>
>> };
>>
>> iio-hwmon {
>> compatible = "iio-hwmon";
>> io-channels = <&adc 0>;
>> };
>
> This is necessary to make it show up as hwmon. You can see the iio
> device directly at /sys/bus/iio/devices/.....
>
>>
>> I got this output from lm-sensors when booted:
>>
>> iio_hwmon-isa-0000
>> Adapter: ISA adapter
>> in1: +1.28 V
>>
>> I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
>>
>> iio_hwmon-isa-0000
>> Adapter: ISA adapter
>> in1: +1.80 V
>>
>> ADC_12V_TW is the 12V rail sampled through a voltage divider. The
>> voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
>>
>> cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
>> 738
>>
>> 738 / 1023 * 1.8 = 1.2975
>>
>> Looks like the first channel is working! However our reference is
>> incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
>> It does hardcode 2500 in the aspeed_adc_read_raw callback:
>>
>> case IIO_CHAN_INFO_SCALE:
>> *val = 2500; // mV
>> *val2 = 10;
>> return IIO_VAL_FRACTIONAL_LOG2;
>>
>> Should this value the the constant you define?
>>
>> Regardless, I don't think the reference voltage should be a constant.
>> This is going to vary from system to system. Can we put it in the
>> device tree? I notice other devices have vref-supply in their
>> bindings.
>>
>
> Good catch. AST2500 uses a 1.8Vref while AST2400 uses a 2.5Vref. I
> decided against putting a vref-supply in the device tree as neither
> part allows for an external reference. Both the 1.8V and 2.5V are
> fixed, internal references. It just happens that the reference
> voltage changes with the chip generation. Looking at the data sheet
> more, I also see the min/max sampling rate has changed for AST2500 as
> well. This is probably best handled by attaching data to the
> of_device_ids in the of_match_table. That would solve all of these
> per-chip variations.
>
>> I noticed that in_voltage_scale is writable. However, it did not
>> accept any of the values I give it. Is this because we do not handle
>> it in aspeed_adc_write_raw?
>>
>
> I think all attributes become writable because I implement write_raw
> even though only the sample frequency is actually writable. I'm of
> two minds on allowing the scale to be written. If the user knows
> there is a voltage divider before the ADC, why don't they apply that
> correction in userspace? On the other hand, the existence of the
> voltage divider _could_ be specified in the device tree and the kernel
> takes care of the scaling entirely. The latter seems like a
> general-purpose concept but I couldn't find an existing binding for
> specifying it. I decided to start with the minimal functionality of
> only reporting the ADC's natural scaling and letting userspace deal
> with any additional information it has.
>
>> I suggest we add the reference in the device tree bindings, and also
>> allow the value to be updated from userspace.
>>
>>> ---
>>>
>>> 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 | 10 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 282 insertions(+)
>>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 2268a6fb9865..9672d799a3fb 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -130,6 +130,16 @@ config AD799X
>>> To compile this driver as a module, choose M here: the module will be
>>> called ad799x.
>>>
>>> +config ASPEED_ADC
>>> + tristate "Aspeed AST2400/AST2500 ADC"
>>
>> You could just say Aspeed ADC to save us having to update it when the
>> ast2600 comes out.
>
> Ack
>
>>
>>> + depends on ARCH_ASPEED || COMPILE_TEST
>>> + help
>>> + If you say yes here you get support for the Aspeed AST2400/AST2500
>>> + ADC.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called aspeed_adc.
>>
>> Don't forget to test compiling as a module.
>
> Ack
>
>>
>>
>>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>>> new file mode 100644
>>> index 000000000000..9220909aefd4
>>> --- /dev/null
>>
>>> +#define ASPEED_ADC_NUM_CHANNELS 16
>>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>>> +#define ASPEED_ADC_RESOLUTION_BITS 10
>>> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
>>> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
>>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
>>> +
>>> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
>>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
>>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
>>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
>>> +#define ASPEED_ADC_REG_MAX 0xC0
>>> +
>>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
>>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
>>> +
>>> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
>>
>> Nit: You could chose to label these with a shorter prefix. Drop the
>> aspeed or adc, or both.
>
> Ack
>
>>
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + *val = readw(data->base + chan->address);
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = 2500; // mV
>>> + *val2 = 10;
>>
>> What does 10 mean?
>>
>
> ADC resolution in bits. I'll use the constant defined above.
>
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + *val = clk_get_rate(data->clk_scaler->clk) /
>>> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
>>> + return IIO_VAL_INT;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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);
>>> +
>>> + switch (mask) {
>>
>> Handle IIO_CHAN_INFO_SCALE here too.
>
> See above reply.
>
>>
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
>>> + val > ASPEED_ADC_MAX_SAMP_RATE)
>>> + return -EINVAL;
>>> +
>>> + clk_set_rate(data->clk_scaler->clk,
>>> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
>>> + return 0;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +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_ADC_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;
>>> + 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) {
>>> + dev_err(&pdev->dev, "Failed allocating iio device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + data = iio_priv(indio_dev);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(data->base)) {
>>> + dev_err(&pdev->dev, "Failed allocating device resources\n");
>>
>> The function you're calling will do that for you
>>
>> http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134
>>
>> Just return the error here. I'd consider dropping the dev_errs for the
>> other cases in the probe. We still get a reasonable error message
>> without printing something ourselves. For example, when bailing out
>> with ENOMEM:
>>
>> [ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12
>>
>
> Ack
>
>>
>>> + ret = PTR_ERR(data->base);
>>> + goto resource_error;
>>> + }
>>> +
>>> + /* 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_ADC_REG_CLOCK_CONTROL,
>>> + 17, 15, 0, &data->clk_lock);
>>
>> I couldn't see any other drivers that use these functions outside of
>> drivers/clk. I like what you've done here, but someone who understands
>> the clock framework should take a look.
>>
>
> I'll CC one of the clock maintainers in the next version.
>
>>
>>> + if (IS_ERR(data->clk_prescaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
>>> + ret = PTR_ERR(data->clk_prescaler);
>>> + goto prescaler_error;
>>> + }
>>> +
>>> + /*
>>> + * 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_ADC_REG_CLOCK_CONTROL,
>>> + 0, 10, 0, &data->clk_lock);
>>> + if (IS_ERR(data->clk_scaler)) {
>>> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
>>> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>>> + writel(adc_engine_control_reg_val,
>>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>> +
>>> + indio_dev->name = dev_name(&pdev->dev);
>>> + 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);
>>
>> Should we be able to enable just the channels that we want? Perhaps
>> only the ones that are requested through the device tree?
>>
>
> It shouldn't matter. When the pins are muxed to other functions, the
> ADCs will read zero. The only potential advantage is increased
> sampling speed by omitting unused channels in the scan. I'm not
> concerned with that now and the code is much simpler by not dealing
> with per-channel enables.
>
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Could't register the device.\n");
>>> + goto iio_register_error;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +iio_register_error:
>>> + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>
>> Should this be done in remove as well?
>>
> Yes.
>
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> +
>>> +scaler_error:
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> +prescaler_error:
>>> +resource_error:
>>> + return ret;
>>
>> You could just return from the error where it happens in the case
>> where no cleanup is required.
>>
>
> Ack.
>
>>> +}
>>> +
>>> +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);
>>> + clk_disable_unprepare(data->clk_scaler->clk);
>>> + clk_hw_unregister_divider(data->clk_scaler);
>>> + clk_hw_unregister_divider(data->clk_prescaler);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const struct of_device_id aspeed_adc_matches[] = {
>>> + { .compatible = "aspeed,ast2400-adc" },
>>> + { .compatible = "aspeed,ast2500-adc" },
>>> +};
>>
>> This is missing a null entry to terminate.
>
> Ack
>
>>
>>> +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] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-23 7:52 ` Quentin Schulz
@ 2017-03-23 16:57 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:57 UTC (permalink / raw)
To: Quentin Schulz
Cc: OpenBMC Maillist, linux-kernel, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
knaack.h, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
On Thu, Mar 23, 2017 at 12:52 AM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Hi,
>
> On 22/03/2017 21:46, Rick Altherr wrote:
>> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>>> Hi,
>>>
>>> On 21/03/2017 21:48, Rick Altherr wrote:
>>>> Aspeed AST2400/AST2500 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>
>>>> ---
>>>>
>>>> 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
>>>>
> [...]
>>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>>> + .type = IIO_VOLTAGE, \
>>>> + .indexed = 1, \
>>>> + .channel = (_idx), \
>>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>>> + ASPEED_ADC_CHAN(1, 0x12),
>>>> + ASPEED_ADC_CHAN(2, 0x14),
>>>> + ASPEED_ADC_CHAN(3, 0x16),
>>>> + ASPEED_ADC_CHAN(4, 0x18),
>>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>>> + ASPEED_ADC_CHAN(8, 0x20),
>>>> + ASPEED_ADC_CHAN(9, 0x22),
>>>> + ASPEED_ADC_CHAN(10, 0x24),
>>>> + ASPEED_ADC_CHAN(11, 0x26),
>>>> + ASPEED_ADC_CHAN(12, 0x28),
>>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>>> + ASPEED_ADC_CHAN(15, 0x2E),
>>>
>>> It would make sense to name the registers (the _addr parameter of your
>>> macro) so it's easier to understand what it refers to.
>>>
>>
>> I agree with Joel on this. There isn't really a better name than
>> ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to
>> make that clearer.
>>
>
> Is it the name in the datasheet?
>
No, the datasheet has such helpful register names as ADC10 where 0x10
is the offset in the register map.
> [...]
>>>> + indio_dev->name = dev_name(&pdev->dev);
>>>
>>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
>>> of the mail in the probe function). Better name it aspeed-adc or even
>>> better to have a different name per compatible: ast2400-adc or ast2500-adc.
>>
>> Ack. Will use aspeed-adc to avoid parsing the compatible match and
>> stripping the 'aspeed,' prefix.
>>
>
> You don't need to parse the compatible match. You could use the data
> void pointer in your struct of_device_id
> (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234)
> like it's done here: https://lkml.org/lkml/2017/3/21/675
> (sun4i_gpadc_of_id).
>
I figured that out a bit later and did so in v3 I sent out last night.
> Note: please reply to all :)
Whoops. Looks like I did that for all the replies I did yesterday.
>
> Quentin
>
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
@ 2017-03-23 16:57 ` Rick Altherr
0 siblings, 0 replies; 25+ messages in thread
From: Rick Altherr @ 2017-03-23 16:57 UTC (permalink / raw)
To: Quentin Schulz
Cc: OpenBMC Maillist, linux-kernel, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
knaack.h, Geert Uytterhoeven, Marek Vasut, Matt Ranostay,
Lars-Peter Clausen, Crestez Dan Leonard, Akinobu Mita,
Fabrice Gasnier, Jonathan Cameron, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
On Thu, Mar 23, 2017 at 12:52 AM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Hi,
>
> On 22/03/2017 21:46, Rick Altherr wrote:
>> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>>> Hi,
>>>
>>> On 21/03/2017 21:48, Rick Altherr wrote:
>>>> Aspeed AST2400/AST2500 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>
>>>> ---
>>>>
>>>> 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
>>>>
> [...]
>>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>>> + .type = IIO_VOLTAGE, \
>>>> + .indexed = 1, \
>>>> + .channel = (_idx), \
>>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>>> + ASPEED_ADC_CHAN(1, 0x12),
>>>> + ASPEED_ADC_CHAN(2, 0x14),
>>>> + ASPEED_ADC_CHAN(3, 0x16),
>>>> + ASPEED_ADC_CHAN(4, 0x18),
>>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>>> + ASPEED_ADC_CHAN(8, 0x20),
>>>> + ASPEED_ADC_CHAN(9, 0x22),
>>>> + ASPEED_ADC_CHAN(10, 0x24),
>>>> + ASPEED_ADC_CHAN(11, 0x26),
>>>> + ASPEED_ADC_CHAN(12, 0x28),
>>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>>> + ASPEED_ADC_CHAN(15, 0x2E),
>>>
>>> It would make sense to name the registers (the _addr parameter of your
>>> macro) so it's easier to understand what it refers to.
>>>
>>
>> I agree with Joel on this. There isn't really a better name than
>> ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to
>> make that clearer.
>>
>
> Is it the name in the datasheet?
>
No, the datasheet has such helpful register names as ADC10 where 0x10
is the offset in the register map.
> [...]
>>>> + indio_dev->name = dev_name(&pdev->dev);
>>>
>>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
>>> of the mail in the probe function). Better name it aspeed-adc or even
>>> better to have a different name per compatible: ast2400-adc or ast2500-adc.
>>
>> Ack. Will use aspeed-adc to avoid parsing the compatible match and
>> stripping the 'aspeed,' prefix.
>>
>
> You don't need to parse the compatible match. You could use the data
> void pointer in your struct of_device_id
> (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234)
> like it's done here: https://lkml.org/lkml/2017/3/21/675
> (sun4i_gpadc_of_id).
>
I figured that out a bit later and did so in v3 I sent out last night.
> Note: please reply to all :)
Whoops. Looks like I did that for all the replies I did yesterday.
>
> Quentin
>
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-22 10:08 ` Joel Stanley
(?)
@ 2017-03-25 17:11 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2017-03-25 17:11 UTC (permalink / raw)
To: Joel Stanley, Quentin Schulz
Cc: Rick Altherr, OpenBMC Maillist, Linux Kernel Mailing List,
Martin Blumenstingl, William Breathitt Gray, Andreas Klinger,
Rob Herring, linux-iio, Hartmut Knaack, Geert Uytterhoeven,
Marek Vasut, Matt Ranostay, Lars-Peter Clausen,
Crestez Dan Leonard, Akinobu Mita, Fabrice Gasnier,
Peter Meerwald-Stadler, Maxime Ripard, Jacopo Mondi
On 22/03/17 10:08, Joel Stanley wrote:
> Hello Quentin,
>
> On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>
>>> +
>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .channel = (_idx), \
>>> + .address = (_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_ADC_CHAN(0, 0x10),
>>> + ASPEED_ADC_CHAN(1, 0x12),
>>> + ASPEED_ADC_CHAN(2, 0x14),
>>> + ASPEED_ADC_CHAN(3, 0x16),
>>> + ASPEED_ADC_CHAN(4, 0x18),
>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>> + ASPEED_ADC_CHAN(8, 0x20),
>>> + ASPEED_ADC_CHAN(9, 0x22),
>>> + ASPEED_ADC_CHAN(10, 0x24),
>>> + ASPEED_ADC_CHAN(11, 0x26),
>>> + ASPEED_ADC_CHAN(12, 0x28),
>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>> + ASPEED_ADC_CHAN(15, 0x2E),
>>
>> It would make sense to name the registers (the _addr parameter of your
>> macro) so it's easier to understand what it refers to.
>
> I appreciate the desire to not have magic values. However, I think
> these are okay. We don't use them anywhere else, and it is obvious
> from reading that they are the registers containing the ADC values for
> each channel.
>
> The alternative would look like this:
>
> + ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA),
> + ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA),
>
> Which doesn't really help me as someone reading the code.
>
>>> + /* Start all channels in normal mode. */
>>> + clk_prepare_enable(data->clk_scaler->clk);
>>> + adc_engine_control_reg_val = GENMASK(31, 16) |
>>> + ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>>> + writel(adc_engine_control_reg_val,
>>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>>> +
>>> + indio_dev->name = dev_name(&pdev->dev);
>>
>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
>> of the mail in the probe function). Better name it aspeed-adc or even
>> better to have a different name per compatible: ast2400-adc or ast2500-adc.
>
> The label comes out as "adc.1e6e9000". This is the reg property and
> the node name from the device tree, which seems sensible, even if it
> is a bit strange to be grabbing the name of the parent device for it
The intent is this provides the part number rather than a device tree
handle. The devicetree handle is available via other routes if desired.
.
>
> Could the iio core fill in a sensible name for us here? Rick is the
> 31st person to make this mistake, so it would be nice to fix properly.
It's not easy to actually generate it. Quite a few drivers do it by
querying the hardware to get precise model numbers. Manufacturers
of certain devices have an irritating habit of switching 'compatiblish'
chips without bothering to update device tree blobs.
Jonathan
>
> Cheers,
>
> Joel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
2017-03-22 9:47 ` Joel Stanley
(?)
(?)
@ 2017-03-25 17:20 ` Jonathan Cameron
-1 siblings, 0 replies; 25+ messages in thread
From: Jonathan Cameron @ 2017-03-25 17:20 UTC (permalink / raw)
To: Joel Stanley, Rick Altherr
Cc: OpenBMC Maillist, Linux Kernel Mailing List, Martin Blumenstingl,
William Breathitt Gray, Andreas Klinger, Rob Herring, linux-iio,
Hartmut Knaack, Quentin Schulz, Geert Uytterhoeven, Marek Vasut,
Matt Ranostay, Lars-Peter Clausen, Crestez Dan Leonard,
Akinobu Mita, Fabrice Gasnier, Peter Meerwald-Stadler,
Maxime Ripard, Jacopo Mondi
On 22/03/17 09:47, Joel Stanley wrote:
> On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@google.com> wrote:
>> Aspeed AST2400/AST2500 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>
>
> Looks good Rick. I gave it a review from the perspective of the Aspeed
> soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
> worked, but uncovered some things that need addressing.
Few follow ups inline... Busy week so I'm playing catch up on this.
>
> My device tree additions looked like this:
>
> adc: adc@1e6e9000 {
> compatible = "aspeed,ast2500-adc";
> reg = <0x1e6e9000 0xb0>;
> clocks = <&clk_apb>;
> #io-channel-cells = <1>;
>
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_adc0_default>;
> };
>
> iio-hwmon {
> compatible = "iio-hwmon";
> io-channels = <&adc 0>;
> };
>
> I got this output from lm-sensors when booted:
>
> iio_hwmon-isa-0000
> Adapter: ISA adapter
> in1: +1.28 V
>
> I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
>
> iio_hwmon-isa-0000
> Adapter: ISA adapter
> in1: +1.80 V
>
> ADC_12V_TW is the 12V rail sampled through a voltage divider. The
> voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
>
> cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
> 738
>
> 738 / 1023 * 1.8 = 1.2975
>
> Looks like the first channel is working! However our reference is
> incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
> It does hardcode 2500 in the aspeed_adc_read_raw callback:
>
> case IIO_CHAN_INFO_SCALE:
> *val = 2500; // mV
> *val2 = 10;
> return IIO_VAL_FRACTIONAL_LOG2;
>
> Should this value the the constant you define?
>
> Regardless, I don't think the reference voltage should be a constant.
> This is going to vary from system to system. Can we put it in the
> device tree? I notice other devices have vref-supply in their
> bindings.
>
> I noticed that in_voltage_scale is writable. However, it did not
> accept any of the values I give it. Is this because we do not handle
> it in aspeed_adc_write_raw?
Yeah, that's an ugly quirk of IIO I wish we had done differently.
We don't have separate masks for read and write attributes, so there is
no way to have the attributes for the file set correctly.
>
> I suggest we add the reference in the device tree bindings, and also
> allow the value to be updated from userspace.
Not keen on updating from userspace, but indeed to providing it from
device tree. If there is a board out there where it is wrong the devicetree
should be fixed rather than applying a userspace hack. It's not as though
this device is likely to be accurate enough to warant a calibration procedure.
>
>> ---
>>
>> 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 | 10 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 282 insertions(+)
>> create mode 100644 drivers/iio/adc/aspeed_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 2268a6fb9865..9672d799a3fb 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -130,6 +130,16 @@ config AD799X
>> To compile this driver as a module, choose M here: the module will be
>> called ad799x.
>>
>> +config ASPEED_ADC
>> + tristate "Aspeed AST2400/AST2500 ADC"
>
> You could just say Aspeed ADC to save us having to update it when the
> ast2600 comes out.
That's fine (and definitely a good thing if we end up supporting 20 different
variants in a few years time) but make sure to add it to the help text below
so there is something to grep for.
>
>> + depends on ARCH_ASPEED || COMPILE_TEST
>> + help
>> + If you say yes here you get support for the Aspeed AST2400/AST2500
>> + ADC.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called aspeed_adc.
>
> Don't forget to test compiling as a module.
>
>
>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>> new file mode 100644
>> index 000000000000..9220909aefd4
>> --- /dev/null
>
>> +#define ASPEED_ADC_NUM_CHANNELS 16
>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
>> +#define ASPEED_ADC_RESOLUTION_BITS 10
>> +#define ASPEED_ADC_MIN_SAMP_RATE 10000
>> +#define ASPEED_ADC_MAX_SAMP_RATE 500000
>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
>> +
>> +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
>> +#define ASPEED_ADC_REG_MAX 0xC0
>> +
>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
>> +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
>> +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
>> +
>> +#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
>
> Nit: You could chose to label these with a shorter prefix. Drop the
> aspeed or adc, or both.
>
>> +
>> +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);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + *val = readw(data->base + chan->address);
>> + return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = 2500; // mV
>> + *val2 = 10;
>
> What does 10 mean?
>
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *val = clk_get_rate(data->clk_scaler->clk) /
>> + ASPEED_ADC_CLOCKS_PER_SAMPLE;
>> + return IIO_VAL_INT;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +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);
>> +
>> + switch (mask) {
>
> Handle IIO_CHAN_INFO_SCALE here too.
>
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + if (val < ASPEED_ADC_MIN_SAMP_RATE ||
>> + val > ASPEED_ADC_MAX_SAMP_RATE)
>> + return -EINVAL;
>> +
>> + clk_set_rate(data->clk_scaler->clk,
>> + val * ASPEED_ADC_CLOCKS_PER_SAMPLE);
>> + return 0;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +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_ADC_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;
>> + 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) {
>> + dev_err(&pdev->dev, "Failed allocating iio device\n");
>> + return -ENOMEM;
>> + }
>> +
>> + data = iio_priv(indio_dev);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(data->base)) {
>> + dev_err(&pdev->dev, "Failed allocating device resources\n");
>
> The function you're calling will do that for you
>
> http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134
>
> Just return the error here. I'd consider dropping the dev_errs for the
> other cases in the probe. We still get a reasonable error message
> without printing something ourselves. For example, when bailing out
> with ENOMEM:
>
> [ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12
>
>
>> + ret = PTR_ERR(data->base);
>> + goto resource_error;
>> + }
>> +
>> + /* 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_ADC_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>
> I couldn't see any other drivers that use these functions outside of
> drivers/clk. I like what you've done here, but someone who understands
> the clock framework should take a look.
Agreed on both counts.
>
>
>> + if (IS_ERR(data->clk_prescaler)) {
>> + dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
>> + ret = PTR_ERR(data->clk_prescaler);
>> + goto prescaler_error;
>> + }
>> +
>> + /*
>> + * 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_ADC_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + dev_err(&pdev->dev, "Failed allocating scaler clock\n");
>> + 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_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>> + writel(adc_engine_control_reg_val,
>> + data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> +
>> + indio_dev->name = dev_name(&pdev->dev);
>> + 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);
>
> Should we be able to enable just the channels that we want? Perhaps
> only the ones that are requested through the device tree?
That is sometimes done for SoC ADCs as often people don't wire all
the channels out on a given board. Lots of examples in tree...
Leads to a slightly more fiddly driver, but not too bad.
>
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Could't register the device.\n");
>> + goto iio_register_error;
>> + }
>> +
>> + return 0;
>> +
>> +iio_register_error:
>> + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>
> Should this be done in remove as well?
>
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> +
>> +scaler_error:
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> +
>> +prescaler_error:
>> +resource_error:
>> + return ret;
>
> You could just return from the error where it happens in the case
> where no cleanup is required.
>
>> +}
>> +
>> +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);
>> + clk_disable_unprepare(data->clk_scaler->clk);
>> + clk_hw_unregister_divider(data->clk_scaler);
>> + clk_hw_unregister_divider(data->clk_prescaler);
>> +
>> + return 0;
>> +}
>> +
>> +const struct of_device_id aspeed_adc_matches[] = {
>> + { .compatible = "aspeed,ast2400-adc" },
>> + { .compatible = "aspeed,ast2500-adc" },
>> +};
>
> This is missing a null entry to terminate.
>
>> +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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
2017-03-21 20:48 ` Rick Altherr
` (2 preceding siblings ...)
(?)
@ 2017-03-29 0:33 ` Rob Herring
-1 siblings, 0 replies; 25+ messages in thread
From: Rob Herring @ 2017-03-29 0:33 UTC (permalink / raw)
To: Rick Altherr
Cc: openbmc, linux-kernel, devicetree, linux-iio, Hartmut Knaack,
Lars-Peter Clausen, Mark Rutland, Jonathan Cameron,
Peter Meerwald-Stadler
On Tue, Mar 21, 2017 at 01:48:27PM -0700, Rick Altherr wrote:
> Signed-off-by: Rick Altherr <raltherr@google.com>
> ---
>
> Changes in v2:
> - Rewritten as an IIO ADC device
>
> .../devicetree/bindings/iio/adc/aspeed_adc.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-03-29 0:34 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-21 20:48 [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC Rick Altherr
2017-03-21 20:48 ` Rick Altherr
2017-03-21 20:48 ` [PATCH v2 2/2] iio: " Rick Altherr
2017-03-21 21:14 ` Peter Meerwald-Stadler
2017-03-21 21:14 ` Peter Meerwald-Stadler
[not found] ` <CAPLgG=kVewvVzcrWMzK+XyNWLjiEs6nr_hykfC2bbdpeLsf4aw@mail.gmail.com>
2017-03-23 16:54 ` Rick Altherr
2017-03-23 16:54 ` Rick Altherr
2017-03-22 7:21 ` Quentin Schulz
2017-03-22 10:08 ` Joel Stanley
2017-03-22 10:08 ` Joel Stanley
2017-03-25 17:11 ` Jonathan Cameron
[not found] ` <CAPLgG=kVsNake71fFLc2NsoMtrtG0_6Fb6XHep3dQKVuRgOmbA@mail.gmail.com>
2017-03-23 7:52 ` Quentin Schulz
2017-03-23 16:57 ` Rick Altherr
2017-03-23 16:57 ` Rick Altherr
2017-03-22 9:47 ` Joel Stanley
2017-03-22 9:47 ` Joel Stanley
[not found] ` <CAPLgG=m3ex1dX+Xu8DFLb+DfTn3e-r0=Ln82Z1br6bSinEAcDQ@mail.gmail.com>
2017-03-23 16:54 ` Rick Altherr
2017-03-23 16:54 ` Rick Altherr
2017-03-25 17:20 ` Jonathan Cameron
2017-03-23 2:42 ` kbuild test robot
2017-03-23 5:52 ` kbuild test robot
[not found] ` <20170321204828.31303-1-raltherr-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2017-03-21 21:16 ` [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for " Peter Meerwald-Stadler
2017-03-21 21:16 ` Peter Meerwald-Stadler
2017-03-21 21:16 ` Peter Meerwald-Stadler
2017-03-29 0:33 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.