Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1605987 > unrolled thread
| Started by | Rick Altherr <raltherr@google.com> |
|---|---|
| First post | 2017-03-21 21:50 +0100 |
| Last post | 2017-03-25 18:30 +0100 |
| Articles | 12 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC Rick Altherr <raltherr@google.com> - 2017-03-21 21:50 +0100
[PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Rick Altherr <raltherr@google.com> - 2017-03-21 21:50 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-03-21 22:30 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Rick Altherr <raltherr@google.com> - 2017-03-23 18:00 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-03-22 08:30 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Joel Stanley <joel@jms.id.au> - 2017-03-22 11:10 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Jonathan Cameron <jic23@kernel.org> - 2017-03-25 18:30 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-03-23 09:00 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Rick Altherr <raltherr@google.com> - 2017-03-23 18:00 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Joel Stanley <joel@jms.id.au> - 2017-03-22 10:50 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Rick Altherr <raltherr@google.com> - 2017-03-23 18:00 +0100
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC Jonathan Cameron <jic23@kernel.org> - 2017-03-25 18:30 +0100
| From | Rick Altherr <raltherr@google.com> |
|---|---|
| Date | 2017-03-21 21:50 +0100 |
| Subject | [PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnyT8-8vM-19@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Rick Altherr <raltherr@google.com> |
|---|---|
| Date | 2017-03-21 21:50 +0100 |
| Subject | [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnyT8-8vM-21@gated-at.bofh.it> |
| In reply to | #1605987 |
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
[toc] | [prev] | [next] | [standalone]
| From | Peter Meerwald-Stadler <pmeerw@pmeerw.net> |
|---|---|
| Date | 2017-03-21 22:30 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnzvP-BH-1@gated-at.bofh.it> |
| In reply to | #1605991 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Rick Altherr <raltherr@google.com> |
|---|---|
| Date | 2017-03-23 18:00 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <toefE-4ZJ-19@gated-at.bofh.it> |
| In reply to | #1606008 |
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
[toc] | [prev] | [next] | [standalone]
| From | Quentin Schulz <quentin.schulz@free-electrons.com> |
|---|---|
| Date | 2017-03-22 08:30 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnISt-7c5-5@gated-at.bofh.it> |
| In reply to | #1605991 |
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
[toc] | [prev] | [next] | [standalone]
| From | Joel Stanley <joel@jms.id.au> |
|---|---|
| Date | 2017-03-22 11:10 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnLnk-Jn-15@gated-at.bofh.it> |
| In reply to | #1606248 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-03-25 18:30 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <toXFL-3ZY-5@gated-at.bofh.it> |
| In reply to | #1606323 |
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
>
[toc] | [prev] | [next] | [standalone]
| From | Quentin Schulz <quentin.schulz@free-electrons.com> |
|---|---|
| Date | 2017-03-23 09:00 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <to5P3-7B0-1@gated-at.bofh.it> |
| In reply to | #1606248 |
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
[toc] | [prev] | [next] | [standalone]
| From | Rick Altherr <raltherr@google.com> |
|---|---|
| Date | 2017-03-23 18:00 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <toefE-4ZJ-7@gated-at.bofh.it> |
| In reply to | #1607216 |
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
[toc] | [prev] | [next] | [standalone]
| From | Joel Stanley <joel@jms.id.au> |
|---|---|
| Date | 2017-03-22 10:50 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <tnL3X-if-13@gated-at.bofh.it> |
| In reply to | #1605991 |
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
>
[toc] | [prev] | [next] | [standalone]
| From | Rick Altherr <raltherr@google.com> |
|---|---|
| Date | 2017-03-23 18:00 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <toefF-4ZJ-43@gated-at.bofh.it> |
| In reply to | #1606317 |
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
>>>
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-03-25 18:30 +0100 |
| Subject | Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC |
| Message-ID | <toXFL-3ZY-1@gated-at.bofh.it> |
| In reply to | #1606317 |
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
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web