* [PATCH] iio: adc: add support for Intel ADC
@ 2019-09-16 10:34 Felipe Balbi
2019-09-17 13:38 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2019-09-16 10:34 UTC (permalink / raw
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, Felipe Balbi
Recent Intel SoCs have an integrated 14-bit, 4 MS/sec ADC. This patch
adds support for that controller.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
drivers/iio/adc/Kconfig | 9 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/intel-adc.c | 482 ++++++++++++++++++++++++++++++++++++
3 files changed, 492 insertions(+)
create mode 100644 drivers/iio/adc/intel-adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7e3286265a38..e4810a38b25f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -432,6 +432,15 @@ config INGENIC_ADC
This driver can also be built as a module. If so, the module will be
called ingenic_adc.
+config INTEL_ADC
+ tristate "Intel ADC IIO driver"
+ depends on PCI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Intel ADC available on
+ recent SoCs.
+
config IMX7D_ADC
tristate "Freescale IMX7D ADC driver"
depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ef9cc485fb67..f04e1bf89826 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_HX711) += hx711.o
obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
+obj-$(CONFIG_INTEL_ADC) += intel-adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
new file mode 100644
index 000000000000..381958668563
--- /dev/null
+++ b/drivers/iio/adc/intel-adc.c
@@ -0,0 +1,482 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * intel-adc.c - Intel ADC Driver
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#define PCI_DEVICE_ID_INTEL_EHLLP 0x4bb8
+
+#define ADC_DMA_CTRL 0x0000
+#define ADC_FIFO_STTS 0x0004
+#define ADC_DMA_DEBUG 0x0008
+#define ADC_PWR_STAT 0x000c
+
+#define ADC_CTRL 0x0400
+#define ADC_LOOP_CTRL 0x0404
+#define ADC_LOOP_SEQ 0x0408
+#define ADC_LOOP_DELAY_0 0x040c
+#define ADC_LOOP_DELAY_1 0x0410
+#define ADC_LOOP_DELAY_2 0x0414
+#define ADC_LOOP_DELAY_3 0x0418
+#define ADC_LOOP_DELAY_4 0x041c
+#define ADC_LOOP_DELAY_5 0x0420
+#define ADC_LOOP_DELAY_6 0x0424
+#define ADC_LOOP_DELAY_7 0x0428
+#define ADC_CAL_CTRL 0x042c
+#define ADC_CONV_CTRL 0x0430
+#define ADC_CONV_DELAY 0x0434
+#define ADC_CONFIG1 0x0438
+#define ADC_CONFIG2 0x043c
+#define ADC_FIFO_CTRL 0x0440
+#define ADC_STAT 0x0444
+#define ADC_FIFO_RD_POINTER 0x0448
+#define ADC_RAW_DATA 0x044c
+#define ADC_DATA_THRESHOLD_0 0x0450
+#define ADC_DATA_THRESHOLD_1 0x0454
+#define ADC_DATA_THRESHOLD_2 0x0458
+#define ADC_DATA_THRESHOLD_3 0x045c
+#define ADC_DATA_THRESHOLD_4 0x0460
+#define ADC_DATA_THRESHOLD_5 0x0464
+#define ADC_DATA_THRESHOLD_6 0x0468
+#define ADC_DATA_THRESHOLD_7 0x046c
+#define ADC_THRESHOLD_CONFIG 0x0470
+#define ADC_RIS 0x0474
+#define ADC_IMSC 0x0478
+#define ADC_MIS 0x047c
+#define ADC_LOOP_CFG_0 0x0480
+#define ADC_LOOP_CFG_1 0x0484
+#define ADC_LOOP_CFG_2 0x0488
+#define ADC_LOOP_CFG_3 0x048c
+#define ADC_LOOP_CFG_4 0x0490
+#define ADC_LOOP_CFG_5 0x0494
+#define ADC_LOOP_CFG_6 0x0498
+#define ADC_LOOP_CFG_7 0x049c
+#define ADC_FIFO_DATA 0x0800
+
+#define ADC_BITS 14
+
+/* ADC DMA Ctrl */
+#define ADC_DMA_CTRL_EN BIT(0)
+#define ADC_DMA_CTRL_BRST_THRSLD GENMASK(10, 1)
+
+/* ADC FIFO Status */
+#define ADC_FIFO_STTS_COUNT GENMASK(9, 0)
+
+/* ADC Ctrl */
+#define ADC_CTRL_EN BIT(0)
+#define ADC_CTRL_DATA_THRSHLD_MODE(r) (((r) >> 1) & 3)
+
+/* ADC Conversion Ctrl */
+#define ADC_CONV_CTRL_NUM_SMPL_MASK GENMASK(17, 8)
+#define ADC_CONV_CTRL_NUM_SMPL(n) (((n) - 1) << 8)
+#define ADC_CONV_CTRL_CONV_MODE BIT(4)
+#define ADC_CONV_CTRL_REQ BIT(0)
+
+/* ADC Config1 */
+#define ADC_CONFIG1_ATTEN_TRIM GENMASK(31, 30)
+#define ADC_CONFIG1_INBUF_CUR GENMASK(29, 28)
+#define ADC_CONFIG1_BG_BYPASS BIT(24)
+#define ADC_CONFIG1_BG_TRIM GENMASK(23, 19)
+#define ADC_CONFIG1_BG_CTRIM GENMASK(18, 16)
+#define ADC_CONFIG1_REF_TRIM GENMASK(15, 8)
+#define ADC_CONFIG1_ADC_RESET BIT(6)
+#define ADC_CONFIG1_REF_BYPASS_EN BIT(5)
+#define ADC_CONFIG1_REF_EN BIT(4)
+#define ADC_CONFIG1_CNL_SEL_MASK GENMASK(3, 1)
+#define ADC_CONFIG1_CNL_SEL(ch) ((ch) << 1)
+#define ADC_CONFIG1_DIFF_SE_SEL BIT(0)
+
+/* ADC Interrupt Mask Register */
+#define ADC_INTR_LOOP_DONE_INTR BIT(22)
+#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
+#define ADC_INTR_DMA_DONE_INTR BIT(20)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
+#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
+#define ADC_INTR_FIFO_FULL_INTR BIT(2)
+#define ADC_INTR_SMPL_DONE_INTR BIT(0)
+
+#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
+ ADC_INTR_FIFO_EMPTY_INTR | \
+ ADC_INTR_DMA_DONE_INTR | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
+ ADC_INTR_PWR_DWN_EXIT_INTR | \
+ ADC_INTR_FIFO_FULL_INTR | \
+ ADC_INTR_SMPL_DONE_INTR)
+
+#define ADC_VREF_UV 1600000 /* uV */
+#define ADC_DEFAULT_CONVERSION_TIMEOUT 5000 /* ms */
+
+struct intel_adc {
+ struct completion completion;
+ struct pci_dev *pci;
+ struct iio_dev *iio;
+
+ void __iomem *regs;
+
+ u32 value;
+};
+
+static inline void intel_adc_writel(void __iomem *base, u32 offset, u32 value)
+{
+ writel(value, base + offset);
+}
+
+static inline u32 intel_adc_readl(void __iomem *base, u32 offset)
+{
+ return readl(base + offset);
+}
+
+static void intel_adc_enable(struct intel_adc *adc)
+{
+ u32 ctrl;
+ u32 cfg1;
+
+ cfg1 = intel_adc_readl(adc->regs, ADC_CONFIG1);
+ cfg1 &= ~ADC_CONFIG1_ADC_RESET;
+ intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
+ ctrl |= ADC_CTRL_EN;
+ intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
+
+ cfg1 |= ADC_CONFIG1_REF_EN;
+ intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
+
+ /* must wait 1ms before allowing any further accesses */
+ usleep_range(1000, 1500);
+}
+
+static void intel_adc_disable(struct intel_adc *adc)
+{
+ u32 ctrl;
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
+ ctrl &= ~ADC_CTRL_EN;
+ intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
+}
+
+static int intel_adc_single_channel_conversion(struct intel_adc *adc,
+ struct iio_chan_spec const *channel, int *val)
+{
+ u32 ctrl;
+ u32 reg;
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CONV_CTRL);
+ ctrl |= ADC_CONV_CTRL_CONV_MODE;
+ ctrl &= ~ADC_CONV_CTRL_NUM_SMPL_MASK;
+ ctrl |= ADC_CONV_CTRL_NUM_SMPL(1);
+ intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
+
+ reg = intel_adc_readl(adc->regs, ADC_CONFIG1);
+ reg &= ~ADC_CONFIG1_CNL_SEL_MASK;
+ reg |= ADC_CONFIG1_CNL_SEL(channel->scan_index);
+
+ if (channel->differential)
+ reg &= ~ADC_CONFIG1_DIFF_SE_SEL;
+ else
+ reg |= ADC_CONFIG1_DIFF_SE_SEL;
+
+ intel_adc_writel(adc->regs, ADC_CONFIG1, reg);
+
+ ctrl |= ADC_CONV_CTRL_REQ;
+ intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
+
+ /* enable sample done IRQ event */
+ reg = intel_adc_readl(adc->regs, ADC_IMSC);
+ reg &= ~ADC_INTR_SMPL_DONE_INTR;
+ intel_adc_writel(adc->regs, ADC_IMSC, reg);
+
+ usleep_range(1000, 5000);
+ adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
+
+ return 0;
+}
+
+static int intel_adc_read_raw(struct iio_dev *iio,
+ struct iio_chan_spec const *channel, int *val, int *val2,
+ long mask)
+{
+ struct intel_adc *adc = iio_priv(iio);
+ int shift;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ shift = channel->scan_type.shift;
+
+ ret = iio_device_claim_direct_mode(iio);
+ if (ret)
+ break;
+
+ intel_adc_enable(adc);
+
+ ret = intel_adc_single_channel_conversion(adc, channel, val);
+ if (ret) {
+ intel_adc_disable(adc);
+ iio_device_release_direct_mode(iio);
+ break;
+ }
+ intel_adc_disable(adc);
+ ret = IIO_VAL_INT;
+ iio_device_release_direct_mode(iio);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static const struct iio_info intel_adc_info = {
+ .read_raw = intel_adc_read_raw,
+};
+
+static const struct iio_event_spec intel_adc_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_PERIOD),
+ },
+};
+
+#define INTEL_ADC_DIFF_CHAN(c1, c2) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .differential = true, \
+ .indexed = 1, \
+ .channel = (c1), \
+ .channel2 = (c2), \
+ .scan_index = (c1), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 14, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }, \
+ .event_spec = intel_adc_events, \
+ .num_event_specs = ARRAY_SIZE(intel_adc_events),\
+ .datasheet_name = "ain"#c1"-ain"#c2, \
+}
+
+#define INTEL_ADC_SINGLE_CHAN(c) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = (c), \
+ .scan_index = (c), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 14, \
+ .storagebits = 32, \
+ .shift = 0, \
+ .endianness = IIO_CPU, \
+ }, \
+ .event_spec = intel_adc_events, \
+ .num_event_specs = ARRAY_SIZE(intel_adc_events),\
+ .datasheet_name = "ain"#c, \
+}
+
+static struct iio_chan_spec const intel_adc_channels[] = {
+ INTEL_ADC_DIFF_CHAN(0, 1),
+ INTEL_ADC_DIFF_CHAN(2, 3),
+ INTEL_ADC_DIFF_CHAN(4, 5),
+ INTEL_ADC_DIFF_CHAN(6, 7),
+ INTEL_ADC_SINGLE_CHAN(0),
+ INTEL_ADC_SINGLE_CHAN(1),
+ INTEL_ADC_SINGLE_CHAN(2),
+ INTEL_ADC_SINGLE_CHAN(3),
+ INTEL_ADC_SINGLE_CHAN(4),
+ INTEL_ADC_SINGLE_CHAN(5),
+ INTEL_ADC_SINGLE_CHAN(6),
+ INTEL_ADC_SINGLE_CHAN(7),
+};
+
+static irqreturn_t intel_adc_irq(int irq, void *_adc)
+{
+ struct intel_adc *adc = _adc;
+ u32 status;
+
+ status = intel_adc_readl(adc->regs, ADC_MIS);
+
+ if (!status)
+ return IRQ_NONE;
+
+ intel_adc_writel(adc->regs, ADC_IMSC, ADC_INTR_ALL_MASK);
+ adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
+ complete(&adc->completion);
+
+ return IRQ_HANDLED;
+}
+
+static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+ struct intel_adc *adc;
+ struct iio_dev *iio;
+ int ret;
+ int irq;
+
+ iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
+ if (!iio)
+ return -ENOMEM;
+
+ adc = iio_priv(iio);
+ adc->pci = pci;
+ adc->iio = iio;
+
+ ret = pcim_enable_device(pci);
+ if (ret)
+ return ret;
+
+ pci_set_master(pci);
+
+ ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+ if (ret)
+ return ret;
+
+ adc->regs = pcim_iomap_table(pci)[0];
+ if (!adc->regs) {
+ ret = -EFAULT;
+ return ret;
+ }
+
+ pci_set_drvdata(pci, adc);
+ init_completion(&adc->completion);
+ iio->dev.parent = &pci->dev;
+ iio->name = dev_name(&pci->dev);
+ iio->modes = INDIO_DIRECT_MODE;
+ iio->info = &intel_adc_info;
+ iio->channels = intel_adc_channels;
+ iio->num_channels = ARRAY_SIZE(intel_adc_channels);
+
+ ret = devm_iio_device_register(&pci->dev, iio);
+ if (ret)
+ return ret;
+
+ ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+ if (ret < 0)
+ return ret;
+
+ irq = pci_irq_vector(pci, 0);
+ ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
+ IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
+ "intel-adc", adc);
+ if (ret)
+ goto err;
+
+ pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
+ pm_runtime_use_autosuspend(&pci->dev);
+ pm_runtime_put_autosuspend(&pci->dev);
+ pm_runtime_allow(&pci->dev);
+
+ return 0;
+
+err:
+ pci_free_irq_vectors(pci);
+ return ret;
+}
+
+static void intel_adc_remove(struct pci_dev *pci)
+{
+ pm_runtime_forbid(&pci->dev);
+ pm_runtime_get_noresume(&pci->dev);
+
+ pci_free_irq_vectors(pci);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int intel_adc_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int intel_adc_resume(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(intel_adc_pm_ops, intel_adc_suspend, intel_adc_resume);
+
+static const struct pci_device_id intel_adc_id_table[] = {
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP), },
+ { } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
+
+static struct pci_driver intel_adc_driver = {
+ .name = "intel-adc",
+ .probe = intel_adc_probe,
+ .remove = intel_adc_remove,
+ .id_table = intel_adc_id_table,
+ .driver = {
+ .pm = &intel_adc_pm_ops,
+ }
+};
+module_pci_driver(intel_adc_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_DESCRIPTION("Intel ADC");
+MODULE_LICENSE("GPL v2");
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: adc: add support for Intel ADC
2019-09-16 10:34 [PATCH] iio: adc: add support for Intel ADC Felipe Balbi
@ 2019-09-17 13:38 ` Jonathan Cameron
2019-09-27 10:57 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2019-09-17 13:38 UTC (permalink / raw
To: Felipe Balbi
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
On Mon, 16 Sep 2019 13:34:00 +0300
Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> Recent Intel SoCs have an integrated 14-bit, 4 MS/sec ADC. This patch
> adds support for that controller.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Hi Felipe,
Mostly fine, but a few bits to clean up.
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 9 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/intel-adc.c | 482 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 492 insertions(+)
> create mode 100644 drivers/iio/adc/intel-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7e3286265a38..e4810a38b25f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -432,6 +432,15 @@ config INGENIC_ADC
> This driver can also be built as a module. If so, the module will be
> called ingenic_adc.
>
> +config INTEL_ADC
> + tristate "Intel ADC IIO driver"
> + depends on PCI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say yes here to build support for Intel ADC available on
> + recent SoCs.
> +
> config IMX7D_ADC
> tristate "Freescale IMX7D ADC driver"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ef9cc485fb67..f04e1bf89826 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_HX711) += hx711.o
> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
> obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
> +obj-$(CONFIG_INTEL_ADC) += intel-adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
> diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
> new file mode 100644
> index 000000000000..381958668563
> --- /dev/null
> +++ b/drivers/iio/adc/intel-adc.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * intel-adc.c - Intel ADC Driver
> + *
> + * Copyright (C) 2018 Intel Corporation
> + *
> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/buffer.h>
You aren't currently supporting the buffered interface
or triggers so a few headers to clean out.
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#define PCI_DEVICE_ID_INTEL_EHLLP 0x4bb8
Perhaps just put this inline as it's obvious what it is from
context so doesn't really need a 'name'.
> +
> +#define ADC_DMA_CTRL 0x0000
> +#define ADC_FIFO_STTS 0x0004
> +#define ADC_DMA_DEBUG 0x0008
> +#define ADC_PWR_STAT 0x000c
> +
> +#define ADC_CTRL 0x0400
> +#define ADC_LOOP_CTRL 0x0404
> +#define ADC_LOOP_SEQ 0x0408
> +#define ADC_LOOP_DELAY_0 0x040c
> +#define ADC_LOOP_DELAY_1 0x0410
> +#define ADC_LOOP_DELAY_2 0x0414
> +#define ADC_LOOP_DELAY_3 0x0418
> +#define ADC_LOOP_DELAY_4 0x041c
> +#define ADC_LOOP_DELAY_5 0x0420
> +#define ADC_LOOP_DELAY_6 0x0424
> +#define ADC_LOOP_DELAY_7 0x0428
> +#define ADC_CAL_CTRL 0x042c
> +#define ADC_CONV_CTRL 0x0430
> +#define ADC_CONV_DELAY 0x0434
> +#define ADC_CONFIG1 0x0438
> +#define ADC_CONFIG2 0x043c
> +#define ADC_FIFO_CTRL 0x0440
> +#define ADC_STAT 0x0444
> +#define ADC_FIFO_RD_POINTER 0x0448
> +#define ADC_RAW_DATA 0x044c
> +#define ADC_DATA_THRESHOLD_0 0x0450
> +#define ADC_DATA_THRESHOLD_1 0x0454
> +#define ADC_DATA_THRESHOLD_2 0x0458
> +#define ADC_DATA_THRESHOLD_3 0x045c
> +#define ADC_DATA_THRESHOLD_4 0x0460
> +#define ADC_DATA_THRESHOLD_5 0x0464
> +#define ADC_DATA_THRESHOLD_6 0x0468
> +#define ADC_DATA_THRESHOLD_7 0x046c
> +#define ADC_THRESHOLD_CONFIG 0x0470
> +#define ADC_RIS 0x0474
> +#define ADC_IMSC 0x0478
> +#define ADC_MIS 0x047c
> +#define ADC_LOOP_CFG_0 0x0480
> +#define ADC_LOOP_CFG_1 0x0484
> +#define ADC_LOOP_CFG_2 0x0488
> +#define ADC_LOOP_CFG_3 0x048c
> +#define ADC_LOOP_CFG_4 0x0490
> +#define ADC_LOOP_CFG_5 0x0494
> +#define ADC_LOOP_CFG_6 0x0498
> +#define ADC_LOOP_CFG_7 0x049c
> +#define ADC_FIFO_DATA 0x0800
> +
> +#define ADC_BITS 14
> +
> +/* ADC DMA Ctrl */
> +#define ADC_DMA_CTRL_EN BIT(0)
> +#define ADC_DMA_CTRL_BRST_THRSLD GENMASK(10, 1)
> +
> +/* ADC FIFO Status */
> +#define ADC_FIFO_STTS_COUNT GENMASK(9, 0)
> +
> +/* ADC Ctrl */
> +#define ADC_CTRL_EN BIT(0)
> +#define ADC_CTRL_DATA_THRSHLD_MODE(r) (((r) >> 1) & 3)
> +
> +/* ADC Conversion Ctrl */
> +#define ADC_CONV_CTRL_NUM_SMPL_MASK GENMASK(17, 8)
> +#define ADC_CONV_CTRL_NUM_SMPL(n) (((n) - 1) << 8)
> +#define ADC_CONV_CTRL_CONV_MODE BIT(4)
> +#define ADC_CONV_CTRL_REQ BIT(0)
> +
> +/* ADC Config1 */
> +#define ADC_CONFIG1_ATTEN_TRIM GENMASK(31, 30)
> +#define ADC_CONFIG1_INBUF_CUR GENMASK(29, 28)
> +#define ADC_CONFIG1_BG_BYPASS BIT(24)
> +#define ADC_CONFIG1_BG_TRIM GENMASK(23, 19)
> +#define ADC_CONFIG1_BG_CTRIM GENMASK(18, 16)
> +#define ADC_CONFIG1_REF_TRIM GENMASK(15, 8)
> +#define ADC_CONFIG1_ADC_RESET BIT(6)
> +#define ADC_CONFIG1_REF_BYPASS_EN BIT(5)
> +#define ADC_CONFIG1_REF_EN BIT(4)
> +#define ADC_CONFIG1_CNL_SEL_MASK GENMASK(3, 1)
> +#define ADC_CONFIG1_CNL_SEL(ch) ((ch) << 1)
> +#define ADC_CONFIG1_DIFF_SE_SEL BIT(0)
> +
> +/* ADC Interrupt Mask Register */
> +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
> +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
> +#define ADC_INTR_DMA_DONE_INTR BIT(20)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
> +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
> +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
> +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
Seems to be a mixture of aligned spacing and non aligned.
I don't mind which, but consistency is good.
> +
> +#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
> + ADC_INTR_FIFO_EMPTY_INTR | \
> + ADC_INTR_DMA_DONE_INTR | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
> + ADC_INTR_PWR_DWN_EXIT_INTR | \
> + ADC_INTR_FIFO_FULL_INTR | \
> + ADC_INTR_SMPL_DONE_INTR)
> +
> +#define ADC_VREF_UV 1600000 /* uV */
Units are in the define name (which is nice btw) so probably no need for
the comment.
> +#define ADC_DEFAULT_CONVERSION_TIMEOUT 5000 /* ms */
Give this one explicit units in it's naming as well.
The ADC prefix is a bit generic, but I suppose it's unlikely to get
used in standard headers etc...
> +
> +struct intel_adc {
> + struct completion completion;
> + struct pci_dev *pci;
> + struct iio_dev *iio;
As noted below, this pointer appears unused. I'm not sure the
pci one is used either...
> +
> + void __iomem *regs;
> +
> + u32 value;
> +};
> +
> +static inline void intel_adc_writel(void __iomem *base, u32 offset, u32 value)
> +{
> + writel(value, base + offset);
> +}
> +
> +static inline u32 intel_adc_readl(void __iomem *base, u32 offset)
> +{
> + return readl(base + offset);
> +}
> +
> +static void intel_adc_enable(struct intel_adc *adc)
> +{
> + u32 ctrl;
> + u32 cfg1;
> +
> + cfg1 = intel_adc_readl(adc->regs, ADC_CONFIG1);
> + cfg1 &= ~ADC_CONFIG1_ADC_RESET;
> + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> + ctrl |= ADC_CTRL_EN;
> + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> +
> + cfg1 |= ADC_CONFIG1_REF_EN;
> + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> +
> + /* must wait 1ms before allowing any further accesses */
> + usleep_range(1000, 1500);
> +}
> +
> +static void intel_adc_disable(struct intel_adc *adc)
> +{
> + u32 ctrl;
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> + ctrl &= ~ADC_CTRL_EN;
> + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> +}
> +
> +static int intel_adc_single_channel_conversion(struct intel_adc *adc,
> + struct iio_chan_spec const *channel, int *val)
> +{
> + u32 ctrl;
> + u32 reg;
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CONV_CTRL);
> + ctrl |= ADC_CONV_CTRL_CONV_MODE;
> + ctrl &= ~ADC_CONV_CTRL_NUM_SMPL_MASK;
> + ctrl |= ADC_CONV_CTRL_NUM_SMPL(1);
> + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> +
> + reg = intel_adc_readl(adc->regs, ADC_CONFIG1);
> + reg &= ~ADC_CONFIG1_CNL_SEL_MASK;
> + reg |= ADC_CONFIG1_CNL_SEL(channel->scan_index);
> +
> + if (channel->differential)
> + reg &= ~ADC_CONFIG1_DIFF_SE_SEL;
> + else
> + reg |= ADC_CONFIG1_DIFF_SE_SEL;
> +
> + intel_adc_writel(adc->regs, ADC_CONFIG1, reg);
> +
> + ctrl |= ADC_CONV_CTRL_REQ;
> + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> +
> + /* enable sample done IRQ event */
> + reg = intel_adc_readl(adc->regs, ADC_IMSC);
> + reg &= ~ADC_INTR_SMPL_DONE_INTR;
> + intel_adc_writel(adc->regs, ADC_IMSC, reg);
> +
> + usleep_range(1000, 5000);
> + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> +
> + return 0;
> +}
> +
> +static int intel_adc_read_raw(struct iio_dev *iio,
> + struct iio_chan_spec const *channel, int *val, int *val2,
> + long mask)
> +{
> + struct intel_adc *adc = iio_priv(iio);
> + int shift;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + shift = channel->scan_type.shift;
> +
> + ret = iio_device_claim_direct_mode(iio);
> + if (ret)
> + break;
> +
> + intel_adc_enable(adc);
> +
> + ret = intel_adc_single_channel_conversion(adc, channel, val);
> + if (ret) {
> + intel_adc_disable(adc);
> + iio_device_release_direct_mode(iio);
> + break;
nitpick (feel free to ignore).
It might be nice to pull this case block as a separate function, then you
could cleanly use goto to do the unwinding.
> + }
> + intel_adc_disable(adc);
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(iio);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info intel_adc_info = {
> + .read_raw = intel_adc_read_raw,
> +};
> +
> +static const struct iio_event_spec intel_adc_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_PERIOD),
> + },
> +};
> +
> +#define INTEL_ADC_DIFF_CHAN(c1, c2) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .differential = true, \
> + .indexed = 1, \
> + .channel = (c1), \
> + .channel2 = (c2), \
> + .scan_index = (c1), \
I think we get overlapping index values between these and
the SINGLE_CHAN ones. These should be unique.
Also, without buffered interface support they don't actually
do anything so drop them for now. Same with scan_type.
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 14, \
> + .storagebits = 32, \
> + .endianness = IIO_CPU, \
> + }, \
> + .event_spec = intel_adc_events, \
> + .num_event_specs = ARRAY_SIZE(intel_adc_events),\
> + .datasheet_name = "ain"#c1"-ain"#c2, \
> +}
> +
> +#define INTEL_ADC_SINGLE_CHAN(c) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (c), \
> + .scan_index = (c), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 14, \
> + .storagebits = 32, \
> + .shift = 0, \
No need to specify shift of 0 as that's the 'obviousish' default.
> + .endianness = IIO_CPU, \
> + }, \
> + .event_spec = intel_adc_events, \
> + .num_event_specs = ARRAY_SIZE(intel_adc_events),\
> + .datasheet_name = "ain"#c, \
> +}
> +
> +static struct iio_chan_spec const intel_adc_channels[] = {
> + INTEL_ADC_DIFF_CHAN(0, 1),
> + INTEL_ADC_DIFF_CHAN(2, 3),
> + INTEL_ADC_DIFF_CHAN(4, 5),
> + INTEL_ADC_DIFF_CHAN(6, 7),
> + INTEL_ADC_SINGLE_CHAN(0),
> + INTEL_ADC_SINGLE_CHAN(1),
> + INTEL_ADC_SINGLE_CHAN(2),
> + INTEL_ADC_SINGLE_CHAN(3),
> + INTEL_ADC_SINGLE_CHAN(4),
> + INTEL_ADC_SINGLE_CHAN(5),
> + INTEL_ADC_SINGLE_CHAN(6),
> + INTEL_ADC_SINGLE_CHAN(7),
> +};
> +
> +static irqreturn_t intel_adc_irq(int irq, void *_adc)
> +{
> + struct intel_adc *adc = _adc;
> + u32 status;
> +
> + status = intel_adc_readl(adc->regs, ADC_MIS);
> +
> + if (!status)
> + return IRQ_NONE;
> +
> + intel_adc_writel(adc->regs, ADC_IMSC, ADC_INTR_ALL_MASK);
> + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> + complete(&adc->completion);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> + struct intel_adc *adc;
> + struct iio_dev *iio;
> + int ret;
> + int irq;
> +
> + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
> + if (!iio)
> + return -ENOMEM;
> +
> + adc = iio_priv(iio);
> + adc->pci = pci;
> + adc->iio = iio;
This pointer look usually means that the driver could be slightly
adjusted to remove the need to go from iio_dev -> private
and private-> iio_dev.
In this case I can't find a user of adc->iio so get rid of it.
> +
> + ret = pcim_enable_device(pci);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pci);
> +
> + ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> + if (ret)
> + return ret;
> +
> + adc->regs = pcim_iomap_table(pci)[0];
> + if (!adc->regs) {
> + ret = -EFAULT;
> + return ret;
> + }
> +
> + pci_set_drvdata(pci, adc);
> + init_completion(&adc->completion);
> + iio->dev.parent = &pci->dev;
> + iio->name = dev_name(&pci->dev);
> + iio->modes = INDIO_DIRECT_MODE;
> + iio->info = &intel_adc_info;
> + iio->channels = intel_adc_channels;
> + iio->num_channels = ARRAY_SIZE(intel_adc_channels);
> +
> + ret = devm_iio_device_register(&pci->dev, iio);
> + if (ret)
> + return ret;
> +
> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + irq = pci_irq_vector(pci, 0);
> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> + "intel-adc", adc);
Requesting the interrupt only after exposing userspace and in kernel
interfaces seems liable to cause problem.
> + if (ret)
> + goto err;
> +
> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> + pm_runtime_use_autosuspend(&pci->dev);
> + pm_runtime_put_autosuspend(&pci->dev);
> + pm_runtime_allow(&pci->dev);
> +
> + return 0;
> +
> +err:
> + pci_free_irq_vectors(pci);
> + return ret;
> +}
> +
> +static void intel_adc_remove(struct pci_dev *pci)
> +{
> + pm_runtime_forbid(&pci->dev);
> + pm_runtime_get_noresume(&pci->dev);
> +
> + pci_free_irq_vectors(pci);
There is a theoretical race here. We have freed the irq vectors
before removing the userspace and in kernel interfaces.
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int intel_adc_suspend(struct device *dev)
> +{
Why provide empty sleep and resume functions?
> + return 0;
> +}
> +
> +static int intel_adc_resume(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(intel_adc_pm_ops, intel_adc_suspend, intel_adc_resume);
> +
> +static const struct pci_device_id intel_adc_id_table[] = {
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP), },
> + { } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
> +
> +static struct pci_driver intel_adc_driver = {
> + .name = "intel-adc",
> + .probe = intel_adc_probe,
> + .remove = intel_adc_remove,
> + .id_table = intel_adc_id_table,
> + .driver = {
> + .pm = &intel_adc_pm_ops,
.pm should be indented one more level.
> + }
> +};
> +module_pci_driver(intel_adc_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel ADC");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: adc: add support for Intel ADC
2019-09-17 13:38 ` Jonathan Cameron
@ 2019-09-27 10:57 ` Felipe Balbi
2019-10-01 9:25 ` [PATCH v2] " Felipe Balbi
2019-10-03 13:23 ` [PATCH] " Jonathan Cameron
0 siblings, 2 replies; 9+ messages in thread
From: Felipe Balbi @ 2019-09-27 10:57 UTC (permalink / raw
To: Jonathan Cameron
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Hi,
Jonathan Cameron <jonathan.cameron@huawei.com> writes:
>> diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
>> new file mode 100644
>> index 000000000000..381958668563
>> --- /dev/null
>> +++ b/drivers/iio/adc/intel-adc.c
>> @@ -0,0 +1,482 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * intel-adc.c - Intel ADC Driver
>> + *
>> + * Copyright (C) 2018 Intel Corporation
>> + *
>> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/iio/buffer.h>
>
> You aren't currently supporting the buffered interface
> or triggers so a few headers to clean out.
removed
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +#define PCI_DEVICE_ID_INTEL_EHLLP 0x4bb8
>
> Perhaps just put this inline as it's obvious what it is from
> context so doesn't really need a 'name'.
removed
>> +/* ADC Interrupt Mask Register */
>> +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
>> +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
>> +#define ADC_INTR_DMA_DONE_INTR BIT(20)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
>> +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
>> +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
>> +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
>
> Seems to be a mixture of aligned spacing and non aligned.
> I don't mind which, but consistency is good.
I did it like this because otherwise I would need another tab for all
defines and some of them would cross 80-columns. I can change, no
worries, just let me know.
>> +#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
>> + ADC_INTR_FIFO_EMPTY_INTR | \
>> + ADC_INTR_DMA_DONE_INTR | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
>> + ADC_INTR_PWR_DWN_EXIT_INTR | \
>> + ADC_INTR_FIFO_FULL_INTR | \
>> + ADC_INTR_SMPL_DONE_INTR)
>> +
>> +#define ADC_VREF_UV 1600000 /* uV */
>
> Units are in the define name (which is nice btw) so probably no need for
> the comment.
>
>> +#define ADC_DEFAULT_CONVERSION_TIMEOUT 5000 /* ms */
>
> Give this one explicit units in it's naming as well.
done
> The ADC prefix is a bit generic, but I suppose it's unlikely to get
> used in standard headers etc...
okay
>> +
>> +struct intel_adc {
>> + struct completion completion;
>> + struct pci_dev *pci;
>> + struct iio_dev *iio;
>
> As noted below, this pointer appears unused. I'm not sure the
> pci one is used either...
removed both
>> +static int intel_adc_read_raw(struct iio_dev *iio,
>> + struct iio_chan_spec const *channel, int *val, int *val2,
>> + long mask)
>> +{
>> + struct intel_adc *adc = iio_priv(iio);
>> + int shift;
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + shift = channel->scan_type.shift;
>> +
>> + ret = iio_device_claim_direct_mode(iio);
>> + if (ret)
>> + break;
>> +
>> + intel_adc_enable(adc);
>> +
>> + ret = intel_adc_single_channel_conversion(adc, channel, val);
>> + if (ret) {
>> + intel_adc_disable(adc);
>> + iio_device_release_direct_mode(iio);
>> + break;
>
> nitpick (feel free to ignore).
> It might be nice to pull this case block as a separate function, then you
> could cleanly use goto to do the unwinding.
you mean something like below:
static int intel_adc_read_info_raw(...)
{
....
}
static int intel_adc_read_raw(...)
{
switch (mask) {
case IIO_CHAN_INFO_RAW:
ret = intel_adc_read_info_raw(...);
break;
default:
ret = -EINVAL;
}
}
??
>> +#define INTEL_ADC_DIFF_CHAN(c1, c2) \
>> +{ \
>> + .type = IIO_VOLTAGE, \
>> + .differential = true, \
>> + .indexed = 1, \
>> + .channel = (c1), \
>> + .channel2 = (c2), \
>> + .scan_index = (c1), \
>
> I think we get overlapping index values between these and
> the SINGLE_CHAN ones. These should be unique.
>
> Also, without buffered interface support they don't actually
> do anything so drop them for now. Same with scan_type.
removed
>> +#define INTEL_ADC_SINGLE_CHAN(c) \
>> +{ \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (c), \
>> + .scan_index = (c), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .scan_type = { \
>> + .sign = 's', \
>> + .realbits = 14, \
>> + .storagebits = 32, \
>> + .shift = 0, \
>
> No need to specify shift of 0 as that's the 'obviousish' default.
removed
>> +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
>> +{
>> + struct intel_adc *adc;
>> + struct iio_dev *iio;
>> + int ret;
>> + int irq;
>> +
>> + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
>> + if (!iio)
>> + return -ENOMEM;
>> +
>> + adc = iio_priv(iio);
>> + adc->pci = pci;
>> + adc->iio = iio;
>
> This pointer look usually means that the driver could be slightly
> adjusted to remove the need to go from iio_dev -> private
> and private-> iio_dev.
>
> In this case I can't find a user of adc->iio so get rid of it.
removed
>> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
>> + if (ret < 0)
>> + return ret;
>> +
>> + irq = pci_irq_vector(pci, 0);
>> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
>> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
>> + "intel-adc", adc);
>
> Requesting the interrupt only after exposing userspace and in kernel
> interfaces seems liable to cause problem.
It goes the other way around, rather. If I request the interrupt before,
then I could get interrupts before IIO subsystem knows about the device,
no?
>> + if (ret)
>> + goto err;
>> +
>> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
>> + pm_runtime_use_autosuspend(&pci->dev);
>> + pm_runtime_put_autosuspend(&pci->dev);
>> + pm_runtime_allow(&pci->dev);
>> +
>> + return 0;
>> +
>> +err:
>> + pci_free_irq_vectors(pci);
>> + return ret;
>> +}
>> +
>> +static void intel_adc_remove(struct pci_dev *pci)
>> +{
>> + pm_runtime_forbid(&pci->dev);
>> + pm_runtime_get_noresume(&pci->dev);
>> +
>> + pci_free_irq_vectors(pci);
>
> There is a theoretical race here. We have freed the irq vectors
> before removing the userspace and in kernel interfaces.
There's no way to sort this out, though. Is there? Apart from switching
away from device managed resources.
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int intel_adc_suspend(struct device *dev)
>> +{
>
> Why provide empty sleep and resume functions?
no reason, removed.
>> + return 0;
>> +}
>> +
>> +static int intel_adc_resume(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(intel_adc_pm_ops, intel_adc_suspend, intel_adc_resume);
then removed this
>> +static const struct pci_device_id intel_adc_id_table[] = {
>> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP), },
>> + { } /* Terminating Entry */
>> +};
>> +MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
>> +
>> +static struct pci_driver intel_adc_driver = {
>> + .name = "intel-adc",
>> + .probe = intel_adc_probe,
>> + .remove = intel_adc_remove,
>> + .id_table = intel_adc_id_table,
>> + .driver = {
>> + .pm = &intel_adc_pm_ops,
>
> .pm should be indented one more level.
and this
--
balbi
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] iio: adc: add support for Intel ADC
2019-09-27 10:57 ` Felipe Balbi
@ 2019-10-01 9:25 ` Felipe Balbi
2019-10-03 13:23 ` Jonathan Cameron
2019-10-03 13:23 ` [PATCH] " Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2019-10-01 9:25 UTC (permalink / raw
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, Felipe Balbi
Recent Intel SoCs have an integrated 14-bit, 4 MS/sec ADC. This patch
adds support for that controller.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
Changes since v1:
- removed unnecessary includes
- removed name, pci and iio fields from private structure
- added _MS to timeout macro name
- removed scan_type and anything related to buffering
- removed empty sleep functions
drivers/iio/adc/Kconfig | 9 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/intel-adc.c | 426 ++++++++++++++++++++++++++++++++++++
3 files changed, 436 insertions(+)
create mode 100644 drivers/iio/adc/intel-adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f0af3a42f53c..eb305f30c6d5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -432,6 +432,15 @@ config INGENIC_ADC
This driver can also be built as a module. If so, the module will be
called ingenic_adc.
+config INTEL_ADC
+ tristate "Intel ADC IIO driver"
+ depends on PCI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Intel ADC available on
+ recent SoCs.
+
config IMX7D_ADC
tristate "Freescale IMX7D ADC driver"
depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ef9cc485fb67..f04e1bf89826 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_HX711) += hx711.o
obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
+obj-$(CONFIG_INTEL_ADC) += intel-adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
new file mode 100644
index 000000000000..9c834cba762b
--- /dev/null
+++ b/drivers/iio/adc/intel-adc.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * intel-adc.c - Intel ADC Driver
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#define ADC_DMA_CTRL 0x0000
+#define ADC_FIFO_STTS 0x0004
+#define ADC_DMA_DEBUG 0x0008
+#define ADC_PWR_STAT 0x000c
+
+#define ADC_CTRL 0x0400
+#define ADC_LOOP_CTRL 0x0404
+#define ADC_LOOP_SEQ 0x0408
+#define ADC_LOOP_DELAY_0 0x040c
+#define ADC_LOOP_DELAY_1 0x0410
+#define ADC_LOOP_DELAY_2 0x0414
+#define ADC_LOOP_DELAY_3 0x0418
+#define ADC_LOOP_DELAY_4 0x041c
+#define ADC_LOOP_DELAY_5 0x0420
+#define ADC_LOOP_DELAY_6 0x0424
+#define ADC_LOOP_DELAY_7 0x0428
+#define ADC_CAL_CTRL 0x042c
+#define ADC_CONV_CTRL 0x0430
+#define ADC_CONV_DELAY 0x0434
+#define ADC_CONFIG1 0x0438
+#define ADC_CONFIG2 0x043c
+#define ADC_FIFO_CTRL 0x0440
+#define ADC_STAT 0x0444
+#define ADC_FIFO_RD_POINTER 0x0448
+#define ADC_RAW_DATA 0x044c
+#define ADC_DATA_THRESHOLD_0 0x0450
+#define ADC_DATA_THRESHOLD_1 0x0454
+#define ADC_DATA_THRESHOLD_2 0x0458
+#define ADC_DATA_THRESHOLD_3 0x045c
+#define ADC_DATA_THRESHOLD_4 0x0460
+#define ADC_DATA_THRESHOLD_5 0x0464
+#define ADC_DATA_THRESHOLD_6 0x0468
+#define ADC_DATA_THRESHOLD_7 0x046c
+#define ADC_THRESHOLD_CONFIG 0x0470
+#define ADC_RIS 0x0474
+#define ADC_IMSC 0x0478
+#define ADC_MIS 0x047c
+#define ADC_LOOP_CFG_0 0x0480
+#define ADC_LOOP_CFG_1 0x0484
+#define ADC_LOOP_CFG_2 0x0488
+#define ADC_LOOP_CFG_3 0x048c
+#define ADC_LOOP_CFG_4 0x0490
+#define ADC_LOOP_CFG_5 0x0494
+#define ADC_LOOP_CFG_6 0x0498
+#define ADC_LOOP_CFG_7 0x049c
+#define ADC_FIFO_DATA 0x0800
+
+#define ADC_BITS 14
+
+/* ADC DMA Ctrl */
+#define ADC_DMA_CTRL_EN BIT(0)
+#define ADC_DMA_CTRL_BRST_THRSLD GENMASK(10, 1)
+
+/* ADC FIFO Status */
+#define ADC_FIFO_STTS_COUNT GENMASK(9, 0)
+
+/* ADC Ctrl */
+#define ADC_CTRL_EN BIT(0)
+#define ADC_CTRL_DATA_THRSHLD_MODE(r) (((r) >> 1) & 3)
+
+/* ADC Conversion Ctrl */
+#define ADC_CONV_CTRL_NUM_SMPL_MASK GENMASK(17, 8)
+#define ADC_CONV_CTRL_NUM_SMPL(n) (((n) - 1) << 8)
+#define ADC_CONV_CTRL_CONV_MODE BIT(4)
+#define ADC_CONV_CTRL_REQ BIT(0)
+
+/* ADC Config1 */
+#define ADC_CONFIG1_ATTEN_TRIM GENMASK(31, 30)
+#define ADC_CONFIG1_INBUF_CUR GENMASK(29, 28)
+#define ADC_CONFIG1_BG_BYPASS BIT(24)
+#define ADC_CONFIG1_BG_TRIM GENMASK(23, 19)
+#define ADC_CONFIG1_BG_CTRIM GENMASK(18, 16)
+#define ADC_CONFIG1_REF_TRIM GENMASK(15, 8)
+#define ADC_CONFIG1_ADC_RESET BIT(6)
+#define ADC_CONFIG1_REF_BYPASS_EN BIT(5)
+#define ADC_CONFIG1_REF_EN BIT(4)
+#define ADC_CONFIG1_CNL_SEL_MASK GENMASK(3, 1)
+#define ADC_CONFIG1_CNL_SEL(ch) ((ch) << 1)
+#define ADC_CONFIG1_DIFF_SE_SEL BIT(0)
+
+/* ADC Interrupt Mask Register */
+#define ADC_INTR_LOOP_DONE_INTR BIT(22)
+#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
+#define ADC_INTR_DMA_DONE_INTR BIT(20)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
+#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
+#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
+#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
+#define ADC_INTR_FIFO_FULL_INTR BIT(2)
+#define ADC_INTR_SMPL_DONE_INTR BIT(0)
+
+#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
+ ADC_INTR_FIFO_EMPTY_INTR | \
+ ADC_INTR_DMA_DONE_INTR | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
+ ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
+ ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
+ ADC_INTR_PWR_DWN_EXIT_INTR | \
+ ADC_INTR_FIFO_FULL_INTR | \
+ ADC_INTR_SMPL_DONE_INTR)
+
+#define ADC_VREF_UV 1600000
+#define ADC_DEFAULT_CONVERSION_TIMEOUT_MS 5000
+
+struct intel_adc {
+ struct completion completion;
+ void __iomem *regs;
+ u32 value;
+};
+
+static inline void intel_adc_writel(void __iomem *base, u32 offset, u32 value)
+{
+ writel(value, base + offset);
+}
+
+static inline u32 intel_adc_readl(void __iomem *base, u32 offset)
+{
+ return readl(base + offset);
+}
+
+static void intel_adc_enable(struct intel_adc *adc)
+{
+ u32 ctrl;
+ u32 cfg1;
+
+ cfg1 = intel_adc_readl(adc->regs, ADC_CONFIG1);
+ cfg1 &= ~ADC_CONFIG1_ADC_RESET;
+ intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
+ ctrl |= ADC_CTRL_EN;
+ intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
+
+ cfg1 |= ADC_CONFIG1_REF_EN;
+ intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
+
+ /* must wait 1ms before allowing any further accesses */
+ usleep_range(1000, 1500);
+}
+
+static void intel_adc_disable(struct intel_adc *adc)
+{
+ u32 ctrl;
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
+ ctrl &= ~ADC_CTRL_EN;
+ intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
+}
+
+static int intel_adc_single_channel_conversion(struct intel_adc *adc,
+ struct iio_chan_spec const *channel, int *val)
+{
+ u32 ctrl;
+ u32 reg;
+
+ ctrl = intel_adc_readl(adc->regs, ADC_CONV_CTRL);
+ ctrl |= ADC_CONV_CTRL_CONV_MODE;
+ ctrl &= ~ADC_CONV_CTRL_NUM_SMPL_MASK;
+ ctrl |= ADC_CONV_CTRL_NUM_SMPL(1);
+ intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
+
+ reg = intel_adc_readl(adc->regs, ADC_CONFIG1);
+ reg &= ~ADC_CONFIG1_CNL_SEL_MASK;
+ reg |= ADC_CONFIG1_CNL_SEL(channel->scan_index);
+
+ if (channel->differential)
+ reg &= ~ADC_CONFIG1_DIFF_SE_SEL;
+ else
+ reg |= ADC_CONFIG1_DIFF_SE_SEL;
+
+ intel_adc_writel(adc->regs, ADC_CONFIG1, reg);
+
+ ctrl |= ADC_CONV_CTRL_REQ;
+ intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
+
+ /* enable sample done IRQ event */
+ reg = intel_adc_readl(adc->regs, ADC_IMSC);
+ reg &= ~ADC_INTR_SMPL_DONE_INTR;
+ intel_adc_writel(adc->regs, ADC_IMSC, reg);
+
+ usleep_range(1000, 5000);
+ adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
+
+ return 0;
+}
+
+static int intel_adc_read_raw(struct iio_dev *iio,
+ struct iio_chan_spec const *channel, int *val, int *val2,
+ long mask)
+{
+ struct intel_adc *adc = iio_priv(iio);
+ int shift;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ shift = channel->scan_type.shift;
+
+ ret = iio_device_claim_direct_mode(iio);
+ if (ret)
+ break;
+
+ intel_adc_enable(adc);
+
+ ret = intel_adc_single_channel_conversion(adc, channel, val);
+ if (ret) {
+ intel_adc_disable(adc);
+ iio_device_release_direct_mode(iio);
+ break;
+ }
+ intel_adc_disable(adc);
+ ret = IIO_VAL_INT;
+ iio_device_release_direct_mode(iio);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static const struct iio_info intel_adc_info = {
+ .read_raw = intel_adc_read_raw,
+};
+
+static const struct iio_event_spec intel_adc_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_PERIOD),
+ },
+};
+
+#define INTEL_ADC_SINGLE_CHAN(c) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = (c), \
+ .scan_index = (c), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 14, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }, \
+ .event_spec = intel_adc_events, \
+ .num_event_specs = ARRAY_SIZE(intel_adc_events),\
+ .datasheet_name = "ain"#c, \
+}
+
+static struct iio_chan_spec const intel_adc_channels[] = {
+ INTEL_ADC_SINGLE_CHAN(0),
+ INTEL_ADC_SINGLE_CHAN(1),
+ INTEL_ADC_SINGLE_CHAN(2),
+ INTEL_ADC_SINGLE_CHAN(3),
+ INTEL_ADC_SINGLE_CHAN(4),
+ INTEL_ADC_SINGLE_CHAN(5),
+ INTEL_ADC_SINGLE_CHAN(6),
+ INTEL_ADC_SINGLE_CHAN(7),
+};
+
+static irqreturn_t intel_adc_irq(int irq, void *_adc)
+{
+ struct intel_adc *adc = _adc;
+ u32 status;
+
+ status = intel_adc_readl(adc->regs, ADC_MIS);
+
+ if (!status)
+ return IRQ_NONE;
+
+ intel_adc_writel(adc->regs, ADC_IMSC, ADC_INTR_ALL_MASK);
+ adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
+ complete(&adc->completion);
+
+ return IRQ_HANDLED;
+}
+
+static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+ struct intel_adc *adc;
+ struct iio_dev *iio;
+ int ret;
+ int irq;
+
+ iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
+ if (!iio)
+ return -ENOMEM;
+
+ adc = iio_priv(iio);
+ ret = pcim_enable_device(pci);
+ if (ret)
+ return ret;
+
+ pci_set_master(pci);
+
+ ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+ if (ret)
+ return ret;
+
+ adc->regs = pcim_iomap_table(pci)[0];
+ if (!adc->regs) {
+ ret = -EFAULT;
+ return ret;
+ }
+
+ pci_set_drvdata(pci, adc);
+ init_completion(&adc->completion);
+ iio->dev.parent = &pci->dev;
+ iio->name = dev_name(&pci->dev);
+ iio->modes = INDIO_DIRECT_MODE;
+ iio->info = &intel_adc_info;
+ iio->channels = intel_adc_channels;
+ iio->num_channels = ARRAY_SIZE(intel_adc_channels);
+
+ ret = devm_iio_device_register(&pci->dev, iio);
+ if (ret)
+ return ret;
+
+ ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+ if (ret < 0)
+ return ret;
+
+ irq = pci_irq_vector(pci, 0);
+ ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
+ IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
+ "intel-adc", adc);
+ if (ret)
+ goto err;
+
+ pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
+ pm_runtime_use_autosuspend(&pci->dev);
+ pm_runtime_put_autosuspend(&pci->dev);
+ pm_runtime_allow(&pci->dev);
+
+ return 0;
+
+err:
+ pci_free_irq_vectors(pci);
+ return ret;
+}
+
+static void intel_adc_remove(struct pci_dev *pci)
+{
+ pm_runtime_forbid(&pci->dev);
+ pm_runtime_get_noresume(&pci->dev);
+
+ pci_free_irq_vectors(pci);
+}
+
+static const struct pci_device_id intel_adc_id_table[] = {
+ { PCI_VDEVICE(INTEL, 0x4bb8), },
+ { } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
+
+static struct pci_driver intel_adc_driver = {
+ .name = "intel-adc",
+ .probe = intel_adc_probe,
+ .remove = intel_adc_remove,
+ .id_table = intel_adc_id_table,
+};
+module_pci_driver(intel_adc_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_DESCRIPTION("Intel ADC");
+MODULE_LICENSE("GPL v2");
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: adc: add support for Intel ADC
2019-09-27 10:57 ` Felipe Balbi
2019-10-01 9:25 ` [PATCH v2] " Felipe Balbi
@ 2019-10-03 13:23 ` Jonathan Cameron
2019-10-04 6:39 ` Felipe Balbi
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2019-10-03 13:23 UTC (permalink / raw
To: Felipe Balbi
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
On Fri, 27 Sep 2019 13:57:46 +0300
Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> Hi,
Late reply obviously so you may well have resolved queries in your
v2 patch.
...
>
> >> +/* ADC Interrupt Mask Register */
> >> +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
> >> +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
> >> +#define ADC_INTR_DMA_DONE_INTR BIT(20)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
> >> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
> >> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
> >> +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
> >> +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
> >> +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
> >
> > Seems to be a mixture of aligned spacing and non aligned.
> > I don't mind which, but consistency is good.
>
> I did it like this because otherwise I would need another tab for all
> defines and some of them would cross 80-columns. I can change, no
> worries, just let me know.
I'll go with whatever you did as don't care that strongly!
..
> >> +static int intel_adc_read_raw(struct iio_dev *iio,
> >> + struct iio_chan_spec const *channel, int *val, int *val2,
> >> + long mask)
> >> +{
> >> + struct intel_adc *adc = iio_priv(iio);
> >> + int shift;
> >> + int ret;
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_RAW:
> >> + shift = channel->scan_type.shift;
> >> +
> >> + ret = iio_device_claim_direct_mode(iio);
> >> + if (ret)
> >> + break;
> >> +
> >> + intel_adc_enable(adc);
> >> +
> >> + ret = intel_adc_single_channel_conversion(adc, channel, val);
> >> + if (ret) {
> >> + intel_adc_disable(adc);
> >> + iio_device_release_direct_mode(iio);
> >> + break;
> >
> > nitpick (feel free to ignore).
> > It might be nice to pull this case block as a separate function, then you
> > could cleanly use goto to do the unwinding.
>
> you mean something like below:
>
> static int intel_adc_read_info_raw(...)
> {
> ....
> }
>
> static int intel_adc_read_raw(...)
> {
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> ret = intel_adc_read_info_raw(...);
> break;
> default:
> ret = -EINVAL;
> }
> }
>
> ??
Yes, exactly that.
...
>
> >> +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
> >> +{
> >> + struct intel_adc *adc;
> >> + struct iio_dev *iio;
> >> + int ret;
> >> + int irq;
> >> +
> >> + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
> >> + if (!iio)
> >> + return -ENOMEM;
> >> +
> >> + adc = iio_priv(iio);
> >> + adc->pci = pci;
> >> + adc->iio = iio;
> >
> > This pointer look usually means that the driver could be slightly
> > adjusted to remove the need to go from iio_dev -> private
> > and private-> iio_dev.
> >
> > In this case I can't find a user of adc->iio so get rid of it.
>
> removed
>
> >> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + irq = pci_irq_vector(pci, 0);
> >> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> >> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> >> + "intel-adc", adc);
> >
> > Requesting the interrupt only after exposing userspace and in kernel
> > interfaces seems liable to cause problem.
>
> It goes the other way around, rather. If I request the interrupt before,
> then I could get interrupts before IIO subsystem knows about the device,
> no?
Only if your device comes up with interrupts already enabled. Normally they
only turn on in response to some userspace interaction, such as enabling
a threshold. Unless there is a hardware limitation, then at startup no
such interrupt sources should be enabled.
>
> >> + if (ret)
> >> + goto err;
> >> +
> >> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> >> + pm_runtime_use_autosuspend(&pci->dev);
> >> + pm_runtime_put_autosuspend(&pci->dev);
> >> + pm_runtime_allow(&pci->dev);
> >> +
> >> + return 0;
> >> +
> >> +err:
> >> + pci_free_irq_vectors(pci);
> >> + return ret;
> >> +}
> >> +
> >> +static void intel_adc_remove(struct pci_dev *pci)
> >> +{
> >> + pm_runtime_forbid(&pci->dev);
> >> + pm_runtime_get_noresume(&pci->dev);
> >> +
> >> + pci_free_irq_vectors(pci);
> >
> > There is a theoretical race here. We have freed the irq vectors
> > before removing the userspace and in kernel interfaces.
>
> There's no way to sort this out, though. Is there? Apart from switching
> away from device managed resources.
There is the rather helpful,
devm_add_action_or_reset() that allows you to define additional cleanup
actions to be automatically run. It's either that, or stop using
device managed resources from the point at which something that isn't
device managed occurs in probe.
..
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: adc: add support for Intel ADC
2019-10-01 9:25 ` [PATCH v2] " Felipe Balbi
@ 2019-10-03 13:23 ` Jonathan Cameron
2019-10-03 13:38 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2019-10-03 13:23 UTC (permalink / raw
To: Felipe Balbi
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
On Tue, 1 Oct 2019 12:25:52 +0300
Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> Recent Intel SoCs have an integrated 14-bit, 4 MS/sec ADC. This patch
> adds support for that controller.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>
> Changes since v1:
> - removed unnecessary includes
> - removed name, pci and iio fields from private structure
> - added _MS to timeout macro name
> - removed scan_type and anything related to buffering
> - removed empty sleep functions
>
> drivers/iio/adc/Kconfig | 9 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/intel-adc.c | 426 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 436 insertions(+)
> create mode 100644 drivers/iio/adc/intel-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f0af3a42f53c..eb305f30c6d5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -432,6 +432,15 @@ config INGENIC_ADC
> This driver can also be built as a module. If so, the module will be
> called ingenic_adc.
>
> +config INTEL_ADC
> + tristate "Intel ADC IIO driver"
> + depends on PCI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say yes here to build support for Intel ADC available on
> + recent SoCs.
> +
> config IMX7D_ADC
> tristate "Freescale IMX7D ADC driver"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ef9cc485fb67..f04e1bf89826 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_HX711) += hx711.o
> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
> obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
> +obj-$(CONFIG_INTEL_ADC) += intel-adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
> diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
> new file mode 100644
> index 000000000000..9c834cba762b
> --- /dev/null
> +++ b/drivers/iio/adc/intel-adc.c
> @@ -0,0 +1,426 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * intel-adc.c - Intel ADC Driver
> + *
> + * Copyright (C) 2018 Intel Corporation
> + *
> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#define ADC_DMA_CTRL 0x0000
> +#define ADC_FIFO_STTS 0x0004
> +#define ADC_DMA_DEBUG 0x0008
> +#define ADC_PWR_STAT 0x000c
> +
> +#define ADC_CTRL 0x0400
> +#define ADC_LOOP_CTRL 0x0404
> +#define ADC_LOOP_SEQ 0x0408
> +#define ADC_LOOP_DELAY_0 0x040c
> +#define ADC_LOOP_DELAY_1 0x0410
> +#define ADC_LOOP_DELAY_2 0x0414
> +#define ADC_LOOP_DELAY_3 0x0418
> +#define ADC_LOOP_DELAY_4 0x041c
> +#define ADC_LOOP_DELAY_5 0x0420
> +#define ADC_LOOP_DELAY_6 0x0424
> +#define ADC_LOOP_DELAY_7 0x0428
> +#define ADC_CAL_CTRL 0x042c
> +#define ADC_CONV_CTRL 0x0430
> +#define ADC_CONV_DELAY 0x0434
> +#define ADC_CONFIG1 0x0438
> +#define ADC_CONFIG2 0x043c
> +#define ADC_FIFO_CTRL 0x0440
> +#define ADC_STAT 0x0444
> +#define ADC_FIFO_RD_POINTER 0x0448
> +#define ADC_RAW_DATA 0x044c
> +#define ADC_DATA_THRESHOLD_0 0x0450
> +#define ADC_DATA_THRESHOLD_1 0x0454
> +#define ADC_DATA_THRESHOLD_2 0x0458
> +#define ADC_DATA_THRESHOLD_3 0x045c
> +#define ADC_DATA_THRESHOLD_4 0x0460
> +#define ADC_DATA_THRESHOLD_5 0x0464
> +#define ADC_DATA_THRESHOLD_6 0x0468
> +#define ADC_DATA_THRESHOLD_7 0x046c
> +#define ADC_THRESHOLD_CONFIG 0x0470
> +#define ADC_RIS 0x0474
> +#define ADC_IMSC 0x0478
> +#define ADC_MIS 0x047c
> +#define ADC_LOOP_CFG_0 0x0480
> +#define ADC_LOOP_CFG_1 0x0484
> +#define ADC_LOOP_CFG_2 0x0488
> +#define ADC_LOOP_CFG_3 0x048c
> +#define ADC_LOOP_CFG_4 0x0490
> +#define ADC_LOOP_CFG_5 0x0494
> +#define ADC_LOOP_CFG_6 0x0498
> +#define ADC_LOOP_CFG_7 0x049c
> +#define ADC_FIFO_DATA 0x0800
> +
> +#define ADC_BITS 14
> +
> +/* ADC DMA Ctrl */
> +#define ADC_DMA_CTRL_EN BIT(0)
> +#define ADC_DMA_CTRL_BRST_THRSLD GENMASK(10, 1)
> +
> +/* ADC FIFO Status */
> +#define ADC_FIFO_STTS_COUNT GENMASK(9, 0)
> +
> +/* ADC Ctrl */
> +#define ADC_CTRL_EN BIT(0)
> +#define ADC_CTRL_DATA_THRSHLD_MODE(r) (((r) >> 1) & 3)
> +
> +/* ADC Conversion Ctrl */
> +#define ADC_CONV_CTRL_NUM_SMPL_MASK GENMASK(17, 8)
> +#define ADC_CONV_CTRL_NUM_SMPL(n) (((n) - 1) << 8)
> +#define ADC_CONV_CTRL_CONV_MODE BIT(4)
> +#define ADC_CONV_CTRL_REQ BIT(0)
> +
> +/* ADC Config1 */
> +#define ADC_CONFIG1_ATTEN_TRIM GENMASK(31, 30)
> +#define ADC_CONFIG1_INBUF_CUR GENMASK(29, 28)
> +#define ADC_CONFIG1_BG_BYPASS BIT(24)
> +#define ADC_CONFIG1_BG_TRIM GENMASK(23, 19)
> +#define ADC_CONFIG1_BG_CTRIM GENMASK(18, 16)
> +#define ADC_CONFIG1_REF_TRIM GENMASK(15, 8)
> +#define ADC_CONFIG1_ADC_RESET BIT(6)
> +#define ADC_CONFIG1_REF_BYPASS_EN BIT(5)
> +#define ADC_CONFIG1_REF_EN BIT(4)
> +#define ADC_CONFIG1_CNL_SEL_MASK GENMASK(3, 1)
> +#define ADC_CONFIG1_CNL_SEL(ch) ((ch) << 1)
> +#define ADC_CONFIG1_DIFF_SE_SEL BIT(0)
> +
> +/* ADC Interrupt Mask Register */
> +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
> +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
> +#define ADC_INTR_DMA_DONE_INTR BIT(20)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
> +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
> +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
> +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
> +
> +#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
> + ADC_INTR_FIFO_EMPTY_INTR | \
> + ADC_INTR_DMA_DONE_INTR | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
> + ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
> + ADC_INTR_PWR_DWN_EXIT_INTR | \
> + ADC_INTR_FIFO_FULL_INTR | \
> + ADC_INTR_SMPL_DONE_INTR)
> +
> +#define ADC_VREF_UV 1600000
> +#define ADC_DEFAULT_CONVERSION_TIMEOUT_MS 5000
> +
> +struct intel_adc {
> + struct completion completion;
> + void __iomem *regs;
> + u32 value;
> +};
> +
> +static inline void intel_adc_writel(void __iomem *base, u32 offset, u32 value)
> +{
> + writel(value, base + offset);
> +}
> +
> +static inline u32 intel_adc_readl(void __iomem *base, u32 offset)
> +{
> + return readl(base + offset);
> +}
> +
> +static void intel_adc_enable(struct intel_adc *adc)
> +{
> + u32 ctrl;
> + u32 cfg1;
> +
> + cfg1 = intel_adc_readl(adc->regs, ADC_CONFIG1);
> + cfg1 &= ~ADC_CONFIG1_ADC_RESET;
> + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> + ctrl |= ADC_CTRL_EN;
> + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> +
> + cfg1 |= ADC_CONFIG1_REF_EN;
> + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> +
> + /* must wait 1ms before allowing any further accesses */
> + usleep_range(1000, 1500);
> +}
> +
> +static void intel_adc_disable(struct intel_adc *adc)
> +{
> + u32 ctrl;
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> + ctrl &= ~ADC_CTRL_EN;
> + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> +}
> +
> +static int intel_adc_single_channel_conversion(struct intel_adc *adc,
> + struct iio_chan_spec const *channel, int *val)
> +{
> + u32 ctrl;
> + u32 reg;
> +
> + ctrl = intel_adc_readl(adc->regs, ADC_CONV_CTRL);
> + ctrl |= ADC_CONV_CTRL_CONV_MODE;
> + ctrl &= ~ADC_CONV_CTRL_NUM_SMPL_MASK;
> + ctrl |= ADC_CONV_CTRL_NUM_SMPL(1);
> + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> +
> + reg = intel_adc_readl(adc->regs, ADC_CONFIG1);
> + reg &= ~ADC_CONFIG1_CNL_SEL_MASK;
> + reg |= ADC_CONFIG1_CNL_SEL(channel->scan_index);
> +
> + if (channel->differential)
> + reg &= ~ADC_CONFIG1_DIFF_SE_SEL;
> + else
> + reg |= ADC_CONFIG1_DIFF_SE_SEL;
> +
> + intel_adc_writel(adc->regs, ADC_CONFIG1, reg);
> +
> + ctrl |= ADC_CONV_CTRL_REQ;
> + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> +
> + /* enable sample done IRQ event */
> + reg = intel_adc_readl(adc->regs, ADC_IMSC);
> + reg &= ~ADC_INTR_SMPL_DONE_INTR;
> + intel_adc_writel(adc->regs, ADC_IMSC, reg);
> +
> + usleep_range(1000, 5000);
> + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> +
> + return 0;
> +}
> +
> +static int intel_adc_read_raw(struct iio_dev *iio,
> + struct iio_chan_spec const *channel, int *val, int *val2,
> + long mask)
> +{
> + struct intel_adc *adc = iio_priv(iio);
> + int shift;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + shift = channel->scan_type.shift;
> +
> + ret = iio_device_claim_direct_mode(iio);
> + if (ret)
> + break;
> +
> + intel_adc_enable(adc);
> +
> + ret = intel_adc_single_channel_conversion(adc, channel, val);
> + if (ret) {
> + intel_adc_disable(adc);
> + iio_device_release_direct_mode(iio);
> + break;
> + }
> + intel_adc_disable(adc);
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(iio);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info intel_adc_info = {
> + .read_raw = intel_adc_read_raw,
> +};
> +
> +static const struct iio_event_spec intel_adc_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_PERIOD),
> + },
> +};
> +
> +#define INTEL_ADC_SINGLE_CHAN(c) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (c), \
> + .scan_index = (c), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 14, \
> + .storagebits = 32, \
> + .endianness = IIO_CPU, \
> + }, \
> + .event_spec = intel_adc_events, \
> + .num_event_specs = ARRAY_SIZE(intel_adc_events),\
> + .datasheet_name = "ain"#c, \
> +}
> +
> +static struct iio_chan_spec const intel_adc_channels[] = {
> + INTEL_ADC_SINGLE_CHAN(0),
> + INTEL_ADC_SINGLE_CHAN(1),
> + INTEL_ADC_SINGLE_CHAN(2),
> + INTEL_ADC_SINGLE_CHAN(3),
> + INTEL_ADC_SINGLE_CHAN(4),
> + INTEL_ADC_SINGLE_CHAN(5),
> + INTEL_ADC_SINGLE_CHAN(6),
> + INTEL_ADC_SINGLE_CHAN(7),
> +};
> +
> +static irqreturn_t intel_adc_irq(int irq, void *_adc)
> +{
> + struct intel_adc *adc = _adc;
> + u32 status;
> +
> + status = intel_adc_readl(adc->regs, ADC_MIS);
> +
> + if (!status)
> + return IRQ_NONE;
> +
> + intel_adc_writel(adc->regs, ADC_IMSC, ADC_INTR_ALL_MASK);
> + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> + complete(&adc->completion);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> + struct intel_adc *adc;
> + struct iio_dev *iio;
> + int ret;
> + int irq;
> +
> + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
> + if (!iio)
> + return -ENOMEM;
> +
> + adc = iio_priv(iio);
> + ret = pcim_enable_device(pci);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pci);
> +
> + ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> + if (ret)
> + return ret;
> +
> + adc->regs = pcim_iomap_table(pci)[0];
> + if (!adc->regs) {
> + ret = -EFAULT;
> + return ret;
> + }
> +
> + pci_set_drvdata(pci, adc);
> + init_completion(&adc->completion);
> + iio->dev.parent = &pci->dev;
> + iio->name = dev_name(&pci->dev);
> + iio->modes = INDIO_DIRECT_MODE;
> + iio->info = &intel_adc_info;
> + iio->channels = intel_adc_channels;
> + iio->num_channels = ARRAY_SIZE(intel_adc_channels);
> +
> + ret = devm_iio_device_register(&pci->dev, iio);
> + if (ret)
> + return ret;
> +
> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + irq = pci_irq_vector(pci, 0);
> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> + "intel-adc", adc);
> + if (ret)
> + goto err;
> +
> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> + pm_runtime_use_autosuspend(&pci->dev);
> + pm_runtime_put_autosuspend(&pci->dev);
> + pm_runtime_allow(&pci->dev);
> +
> + return 0;
> +
> +err:
> + pci_free_irq_vectors(pci);
> + return ret;
> +}
> +
> +static void intel_adc_remove(struct pci_dev *pci)
> +{
> + pm_runtime_forbid(&pci->dev);
> + pm_runtime_get_noresume(&pci->dev);
> +
> + pci_free_irq_vectors(pci);
> +}
> +
> +static const struct pci_device_id intel_adc_id_table[] = {
> + { PCI_VDEVICE(INTEL, 0x4bb8), },
> + { } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
> +
> +static struct pci_driver intel_adc_driver = {
> + .name = "intel-adc",
> + .probe = intel_adc_probe,
> + .remove = intel_adc_remove,
> + .id_table = intel_adc_id_table,
> +};
> +module_pci_driver(intel_adc_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel ADC");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: adc: add support for Intel ADC
2019-10-03 13:23 ` Jonathan Cameron
@ 2019-10-03 13:38 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2019-10-03 13:38 UTC (permalink / raw
To: Felipe Balbi
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
On Thu, 3 Oct 2019 14:23:51 +0100
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> On Tue, 1 Oct 2019 12:25:52 +0300
> Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
>
> > Recent Intel SoCs have an integrated 14-bit, 4 MS/sec ADC. This patch
> > adds support for that controller.
> >
> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Sorry, previous was a miss-click. Rather than adding more emails to the
thread I'll review here with an extra indent.
To keep everything together I've highlighted places where comments
I just made in the other branch of the thread are relevant.
One personal preference, please don't send new versions as a reply
to a previous thread. It gets very complex to read if we have a lot
of revisions.
Thanks,
Jonathan
> > ---
> >
> > Changes since v1:
> > - removed unnecessary includes
> > - removed name, pci and iio fields from private structure
> > - added _MS to timeout macro name
> > - removed scan_type and anything related to buffering
Seems a few bits got left from this.
> > - removed empty sleep functions
> >
> > drivers/iio/adc/Kconfig | 9 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/intel-adc.c | 426 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 436 insertions(+)
> > create mode 100644 drivers/iio/adc/intel-adc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index f0af3a42f53c..eb305f30c6d5 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -432,6 +432,15 @@ config INGENIC_ADC
> > This driver can also be built as a module. If so, the module will be
> > called ingenic_adc.
> >
> > +config INTEL_ADC
> > + tristate "Intel ADC IIO driver"
> > + depends on PCI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
There doesn't seem to be any buffer setup in this driver currently so these
two selects aren't needed.
> > + help
> > + Say yes here to build support for Intel ADC available on
> > + recent SoCs.
> > +
> > config IMX7D_ADC
> > tristate "Freescale IMX7D ADC driver"
> > depends on ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index ef9cc485fb67..f04e1bf89826 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -42,6 +42,7 @@ obj-$(CONFIG_HX711) += hx711.o
> > obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> > obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
> > obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
> > +obj-$(CONFIG_INTEL_ADC) += intel-adc.o
> > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> > obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> > obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
> > diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
> > new file mode 100644
> > index 000000000000..9c834cba762b
> > --- /dev/null
> > +++ b/drivers/iio/adc/intel-adc.c
> > @@ -0,0 +1,426 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * intel-adc.c - Intel ADC Driver
> > + *
> > + * Copyright (C) 2018 Intel Corporation
> > + *
> > + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> > + */
> > +
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +
> > +#define ADC_DMA_CTRL 0x0000
> > +#define ADC_FIFO_STTS 0x0004
> > +#define ADC_DMA_DEBUG 0x0008
> > +#define ADC_PWR_STAT 0x000c
> > +
> > +#define ADC_CTRL 0x0400
> > +#define ADC_LOOP_CTRL 0x0404
> > +#define ADC_LOOP_SEQ 0x0408
> > +#define ADC_LOOP_DELAY_0 0x040c
> > +#define ADC_LOOP_DELAY_1 0x0410
> > +#define ADC_LOOP_DELAY_2 0x0414
> > +#define ADC_LOOP_DELAY_3 0x0418
> > +#define ADC_LOOP_DELAY_4 0x041c
> > +#define ADC_LOOP_DELAY_5 0x0420
> > +#define ADC_LOOP_DELAY_6 0x0424
> > +#define ADC_LOOP_DELAY_7 0x0428
> > +#define ADC_CAL_CTRL 0x042c
> > +#define ADC_CONV_CTRL 0x0430
> > +#define ADC_CONV_DELAY 0x0434
> > +#define ADC_CONFIG1 0x0438
> > +#define ADC_CONFIG2 0x043c
> > +#define ADC_FIFO_CTRL 0x0440
> > +#define ADC_STAT 0x0444
> > +#define ADC_FIFO_RD_POINTER 0x0448
> > +#define ADC_RAW_DATA 0x044c
> > +#define ADC_DATA_THRESHOLD_0 0x0450
> > +#define ADC_DATA_THRESHOLD_1 0x0454
> > +#define ADC_DATA_THRESHOLD_2 0x0458
> > +#define ADC_DATA_THRESHOLD_3 0x045c
> > +#define ADC_DATA_THRESHOLD_4 0x0460
> > +#define ADC_DATA_THRESHOLD_5 0x0464
> > +#define ADC_DATA_THRESHOLD_6 0x0468
> > +#define ADC_DATA_THRESHOLD_7 0x046c
> > +#define ADC_THRESHOLD_CONFIG 0x0470
> > +#define ADC_RIS 0x0474
> > +#define ADC_IMSC 0x0478
> > +#define ADC_MIS 0x047c
> > +#define ADC_LOOP_CFG_0 0x0480
> > +#define ADC_LOOP_CFG_1 0x0484
> > +#define ADC_LOOP_CFG_2 0x0488
> > +#define ADC_LOOP_CFG_3 0x048c
> > +#define ADC_LOOP_CFG_4 0x0490
> > +#define ADC_LOOP_CFG_5 0x0494
> > +#define ADC_LOOP_CFG_6 0x0498
> > +#define ADC_LOOP_CFG_7 0x049c
> > +#define ADC_FIFO_DATA 0x0800
> > +
> > +#define ADC_BITS 14
> > +
> > +/* ADC DMA Ctrl */
> > +#define ADC_DMA_CTRL_EN BIT(0)
> > +#define ADC_DMA_CTRL_BRST_THRSLD GENMASK(10, 1)
> > +
> > +/* ADC FIFO Status */
> > +#define ADC_FIFO_STTS_COUNT GENMASK(9, 0)
> > +
> > +/* ADC Ctrl */
> > +#define ADC_CTRL_EN BIT(0)
> > +#define ADC_CTRL_DATA_THRSHLD_MODE(r) (((r) >> 1) & 3)
> > +
> > +/* ADC Conversion Ctrl */
> > +#define ADC_CONV_CTRL_NUM_SMPL_MASK GENMASK(17, 8)
> > +#define ADC_CONV_CTRL_NUM_SMPL(n) (((n) - 1) << 8)
> > +#define ADC_CONV_CTRL_CONV_MODE BIT(4)
> > +#define ADC_CONV_CTRL_REQ BIT(0)
> > +
> > +/* ADC Config1 */
> > +#define ADC_CONFIG1_ATTEN_TRIM GENMASK(31, 30)
> > +#define ADC_CONFIG1_INBUF_CUR GENMASK(29, 28)
> > +#define ADC_CONFIG1_BG_BYPASS BIT(24)
> > +#define ADC_CONFIG1_BG_TRIM GENMASK(23, 19)
> > +#define ADC_CONFIG1_BG_CTRIM GENMASK(18, 16)
> > +#define ADC_CONFIG1_REF_TRIM GENMASK(15, 8)
> > +#define ADC_CONFIG1_ADC_RESET BIT(6)
> > +#define ADC_CONFIG1_REF_BYPASS_EN BIT(5)
> > +#define ADC_CONFIG1_REF_EN BIT(4)
> > +#define ADC_CONFIG1_CNL_SEL_MASK GENMASK(3, 1)
> > +#define ADC_CONFIG1_CNL_SEL(ch) ((ch) << 1)
> > +#define ADC_CONFIG1_DIFF_SE_SEL BIT(0)
> > +
> > +/* ADC Interrupt Mask Register */
> > +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
> > +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
> > +#define ADC_INTR_DMA_DONE_INTR BIT(20)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
> > +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
> > +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
> > +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
> > +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
> > +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
> > +
> > +#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
> > + ADC_INTR_FIFO_EMPTY_INTR | \
> > + ADC_INTR_DMA_DONE_INTR | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
> > + ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
> > + ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
> > + ADC_INTR_PWR_DWN_EXIT_INTR | \
> > + ADC_INTR_FIFO_FULL_INTR | \
> > + ADC_INTR_SMPL_DONE_INTR)
> > +
> > +#define ADC_VREF_UV 1600000
> > +#define ADC_DEFAULT_CONVERSION_TIMEOUT_MS 5000
> > +
> > +struct intel_adc {
> > + struct completion completion;
> > + void __iomem *regs;
> > + u32 value;
> > +};
> > +
> > +static inline void intel_adc_writel(void __iomem *base, u32 offset, u32 value)
> > +{
> > + writel(value, base + offset);
> > +}
> > +
> > +static inline u32 intel_adc_readl(void __iomem *base, u32 offset)
> > +{
> > + return readl(base + offset);
> > +}
> > +
> > +static void intel_adc_enable(struct intel_adc *adc)
> > +{
> > + u32 ctrl;
> > + u32 cfg1;
> > +
> > + cfg1 = intel_adc_readl(adc->regs, ADC_CONFIG1);
> > + cfg1 &= ~ADC_CONFIG1_ADC_RESET;
> > + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> > +
> > + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> > + ctrl |= ADC_CTRL_EN;
> > + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> > +
> > + cfg1 |= ADC_CONFIG1_REF_EN;
> > + intel_adc_writel(adc->regs, ADC_CONFIG1, cfg1);
> > +
> > + /* must wait 1ms before allowing any further accesses */
> > + usleep_range(1000, 1500);
> > +}
> > +
> > +static void intel_adc_disable(struct intel_adc *adc)
> > +{
> > + u32 ctrl;
> > +
> > + ctrl = intel_adc_readl(adc->regs, ADC_CTRL);
> > + ctrl &= ~ADC_CTRL_EN;
> > + intel_adc_writel(adc->regs, ADC_CTRL, ctrl);
> > +}
> > +
> > +static int intel_adc_single_channel_conversion(struct intel_adc *adc,
> > + struct iio_chan_spec const *channel, int *val)
> > +{
> > + u32 ctrl;
> > + u32 reg;
> > +
> > + ctrl = intel_adc_readl(adc->regs, ADC_CONV_CTRL);
> > + ctrl |= ADC_CONV_CTRL_CONV_MODE;
> > + ctrl &= ~ADC_CONV_CTRL_NUM_SMPL_MASK;
> > + ctrl |= ADC_CONV_CTRL_NUM_SMPL(1);
> > + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> > +
> > + reg = intel_adc_readl(adc->regs, ADC_CONFIG1);
> > + reg &= ~ADC_CONFIG1_CNL_SEL_MASK;
> > + reg |= ADC_CONFIG1_CNL_SEL(channel->scan_index);
> > +
> > + if (channel->differential)
> > + reg &= ~ADC_CONFIG1_DIFF_SE_SEL;
> > + else
> > + reg |= ADC_CONFIG1_DIFF_SE_SEL;
> > +
> > + intel_adc_writel(adc->regs, ADC_CONFIG1, reg);
> > +
> > + ctrl |= ADC_CONV_CTRL_REQ;
> > + intel_adc_writel(adc->regs, ADC_CONV_CTRL, ctrl);
> > +
> > + /* enable sample done IRQ event */
> > + reg = intel_adc_readl(adc->regs, ADC_IMSC);
> > + reg &= ~ADC_INTR_SMPL_DONE_INTR;
> > + intel_adc_writel(adc->regs, ADC_IMSC, reg);
> > +
> > + usleep_range(1000, 5000);
> > + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> > +
> > + return 0;
> > +}
> > +
> > +static int intel_adc_read_raw(struct iio_dev *iio,
> > + struct iio_chan_spec const *channel, int *val, int *val2,
> > + long mask)
> > +{
> > + struct intel_adc *adc = iio_priv(iio);
> > + int shift;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + shift = channel->scan_type.shift;
Seems to always be 0.
> > +
> > + ret = iio_device_claim_direct_mode(iio);
> > + if (ret)
> > + break;
> > +
> > + intel_adc_enable(adc);
> > +
> > + ret = intel_adc_single_channel_conversion(adc, channel, val);
> > + if (ret) {
> > + intel_adc_disable(adc);
> > + iio_device_release_direct_mode(iio);
> > + break;
> > + }
> > + intel_adc_disable(adc);
> > + ret = IIO_VAL_INT;
> > + iio_device_release_direct_mode(iio);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_info intel_adc_info = {
> > + .read_raw = intel_adc_read_raw,
> > +};
> > +
> > +static const struct iio_event_spec intel_adc_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + }, {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> > + }, {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_EITHER,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_PERIOD),
> > + },
> > +};
> > +
> > +#define INTEL_ADC_SINGLE_CHAN(c) \
> > +{ \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = (c), \
> > + .scan_index = (c), \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .scan_type = { \
This wouldn't normally be present without buffered mode being
supported (push via kfifo / chrdev) but you do use it as
a convenient store for shift.
However, as far as I can see shift is always 0.
> > + .sign = 's', \
> > + .realbits = 14, \
> > + .storagebits = 32, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > + .event_spec = intel_adc_events, \
> > + .num_event_specs = ARRAY_SIZE(intel_adc_events),\
> > + .datasheet_name = "ain"#c, \
> > +}
> > +
> > +static struct iio_chan_spec const intel_adc_channels[] = {
> > + INTEL_ADC_SINGLE_CHAN(0),
> > + INTEL_ADC_SINGLE_CHAN(1),
> > + INTEL_ADC_SINGLE_CHAN(2),
> > + INTEL_ADC_SINGLE_CHAN(3),
> > + INTEL_ADC_SINGLE_CHAN(4),
> > + INTEL_ADC_SINGLE_CHAN(5),
> > + INTEL_ADC_SINGLE_CHAN(6),
> > + INTEL_ADC_SINGLE_CHAN(7),
> > +};
> > +
> > +static irqreturn_t intel_adc_irq(int irq, void *_adc)
> > +{
> > + struct intel_adc *adc = _adc;
> > + u32 status;
> > +
> > + status = intel_adc_readl(adc->regs, ADC_MIS);
> > +
> > + if (!status)
> > + return IRQ_NONE;
> > +
> > + intel_adc_writel(adc->regs, ADC_IMSC, ADC_INTR_ALL_MASK);
> > + adc->value = intel_adc_readl(adc->regs, ADC_FIFO_DATA);
> > + complete(&adc->completion);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
> > +{
> > + struct intel_adc *adc;
> > + struct iio_dev *iio;
> > + int ret;
> > + int irq;
> > +
> > + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
> > + if (!iio)
> > + return -ENOMEM;
> > +
> > + adc = iio_priv(iio);
> > + ret = pcim_enable_device(pci);
> > + if (ret)
> > + return ret;
> > +
> > + pci_set_master(pci);
> > +
> > + ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> > + if (ret)
> > + return ret;
> > +
> > + adc->regs = pcim_iomap_table(pci)[0];
> > + if (!adc->regs) {
> > + ret = -EFAULT;
> > + return ret;
> > + }
> > +
> > + pci_set_drvdata(pci, adc);
> > + init_completion(&adc->completion);
> > + iio->dev.parent = &pci->dev;
> > + iio->name = dev_name(&pci->dev);
> > + iio->modes = INDIO_DIRECT_MODE;
> > + iio->info = &intel_adc_info;
> > + iio->channels = intel_adc_channels;
> > + iio->num_channels = ARRAY_SIZE(intel_adc_channels);
> > +
> > + ret = devm_iio_device_register(&pci->dev, iio);
> > + if (ret)
> > + return ret;
As in other branch of the thread. I would normally assume interrupts
should not occur until after we have caused them in some sense.
However, I see this one is shared, so perhaps we can fire the handler
even if we have done nothing to cause it. However, it looks
to me like the handler would be safe in this case anyway as
we shouldn't have any status bits set. Am I missing something?
> > +
> > + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> > + if (ret < 0)
> > + return ret;
> > +
> > + irq = pci_irq_vector(pci, 0);
> > + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> > + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> > + "intel-adc", adc);
> > + if (ret)
> > + goto err;
> > +
> > + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> > + pm_runtime_use_autosuspend(&pci->dev);
> > + pm_runtime_put_autosuspend(&pci->dev);
> > + pm_runtime_allow(&pci->dev);
> > +
> > + return 0;
> > +
> > +err:
> > + pci_free_irq_vectors(pci);
> > + return ret;
> > +}
> > +
> > +static void intel_adc_remove(struct pci_dev *pci)
> > +{
> > + pm_runtime_forbid(&pci->dev);
> > + pm_runtime_get_noresume(&pci->dev);
> > +
> > + pci_free_irq_vectors(pci);
I was too slow replying to your other email, but would suggest
devm_add_action_or_reset to tidy this up and avoid the potential
race.
> > +}
> > +
> > +static const struct pci_device_id intel_adc_id_table[] = {
> > + { PCI_VDEVICE(INTEL, 0x4bb8), },
> > + { } /* Terminating Entry */
> > +};
> > +MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
> > +
> > +static struct pci_driver intel_adc_driver = {
> > + .name = "intel-adc",
> > + .probe = intel_adc_probe,
> > + .remove = intel_adc_remove,
> > + .id_table = intel_adc_id_table,
> > +};
> > +module_pci_driver(intel_adc_driver);
> > +
> > +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> > +MODULE_DESCRIPTION("Intel ADC");
> > +MODULE_LICENSE("GPL v2");
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: adc: add support for Intel ADC
2019-10-03 13:23 ` [PATCH] " Jonathan Cameron
@ 2019-10-04 6:39 ` Felipe Balbi
2019-10-07 9:15 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2019-10-04 6:39 UTC (permalink / raw
To: Jonathan Cameron
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Hi,
Jonathan Cameron <jonathan.cameron@huawei.com> writes:
>> >> +static int intel_adc_read_raw(struct iio_dev *iio,
>> >> + struct iio_chan_spec const *channel, int *val, int *val2,
>> >> + long mask)
>> >> +{
>> >> + struct intel_adc *adc = iio_priv(iio);
>> >> + int shift;
>> >> + int ret;
>> >> +
>> >> + switch (mask) {
>> >> + case IIO_CHAN_INFO_RAW:
>> >> + shift = channel->scan_type.shift;
>> >> +
>> >> + ret = iio_device_claim_direct_mode(iio);
>> >> + if (ret)
>> >> + break;
>> >> +
>> >> + intel_adc_enable(adc);
>> >> +
>> >> + ret = intel_adc_single_channel_conversion(adc, channel, val);
>> >> + if (ret) {
>> >> + intel_adc_disable(adc);
>> >> + iio_device_release_direct_mode(iio);
>> >> + break;
>> >
>> > nitpick (feel free to ignore).
>> > It might be nice to pull this case block as a separate function, then you
>> > could cleanly use goto to do the unwinding.
>>
>> you mean something like below:
>>
>> static int intel_adc_read_info_raw(...)
>> {
>> ....
>> }
>>
>> static int intel_adc_read_raw(...)
>> {
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> ret = intel_adc_read_info_raw(...);
>> break;
>> default:
>> ret = -EINVAL;
>> }
>> }
>>
>> ??
>
> Yes, exactly that.
I'll change it, no worries.
>> >> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
>> >> + if (ret < 0)
>> >> + return ret;
>> >> +
>> >> + irq = pci_irq_vector(pci, 0);
>> >> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
>> >> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
>> >> + "intel-adc", adc);
>> >
>> > Requesting the interrupt only after exposing userspace and in kernel
>> > interfaces seems liable to cause problem.
>>
>> It goes the other way around, rather. If I request the interrupt before,
>> then I could get interrupts before IIO subsystem knows about the device,
>> no?
>
> Only if your device comes up with interrupts already enabled. Normally they
> only turn on in response to some userspace interaction, such as enabling
> a threshold. Unless there is a hardware limitation, then at startup no
> such interrupt sources should be enabled.
We have FW that _may_ use the hardware and leave it at unpredictable
state. There is a potential for irq status bits being left over by
FW.
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
>> >> + pm_runtime_use_autosuspend(&pci->dev);
>> >> + pm_runtime_put_autosuspend(&pci->dev);
>> >> + pm_runtime_allow(&pci->dev);
>> >> +
>> >> + return 0;
>> >> +
>> >> +err:
>> >> + pci_free_irq_vectors(pci);
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static void intel_adc_remove(struct pci_dev *pci)
>> >> +{
>> >> + pm_runtime_forbid(&pci->dev);
>> >> + pm_runtime_get_noresume(&pci->dev);
>> >> +
>> >> + pci_free_irq_vectors(pci);
>> >
>> > There is a theoretical race here. We have freed the irq vectors
>> > before removing the userspace and in kernel interfaces.
>>
>> There's no way to sort this out, though. Is there? Apart from switching
>> away from device managed resources.
>
> There is the rather helpful,
>
> devm_add_action_or_reset() that allows you to define additional cleanup
> actions to be automatically run. It's either that, or stop using
> device managed resources from the point at which something that isn't
> device managed occurs in probe.
I'll have a look, thanks.
--
balbi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: adc: add support for Intel ADC
2019-10-04 6:39 ` Felipe Balbi
@ 2019-10-07 9:15 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2019-10-07 9:15 UTC (permalink / raw
To: Felipe Balbi
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
On Fri, 4 Oct 2019 09:39:34 +0300
Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> Hi,
>
> Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> >> >> +static int intel_adc_read_raw(struct iio_dev *iio,
> >> >> + struct iio_chan_spec const *channel, int *val, int *val2,
> >> >> + long mask)
> >> >> +{
> >> >> + struct intel_adc *adc = iio_priv(iio);
> >> >> + int shift;
> >> >> + int ret;
> >> >> +
> >> >> + switch (mask) {
> >> >> + case IIO_CHAN_INFO_RAW:
> >> >> + shift = channel->scan_type.shift;
> >> >> +
> >> >> + ret = iio_device_claim_direct_mode(iio);
> >> >> + if (ret)
> >> >> + break;
> >> >> +
> >> >> + intel_adc_enable(adc);
> >> >> +
> >> >> + ret = intel_adc_single_channel_conversion(adc, channel, val);
> >> >> + if (ret) {
> >> >> + intel_adc_disable(adc);
> >> >> + iio_device_release_direct_mode(iio);
> >> >> + break;
> >> >
> >> > nitpick (feel free to ignore).
> >> > It might be nice to pull this case block as a separate function, then you
> >> > could cleanly use goto to do the unwinding.
> >>
> >> you mean something like below:
> >>
> >> static int intel_adc_read_info_raw(...)
> >> {
> >> ....
> >> }
> >>
> >> static int intel_adc_read_raw(...)
> >> {
> >> switch (mask) {
> >> case IIO_CHAN_INFO_RAW:
> >> ret = intel_adc_read_info_raw(...);
> >> break;
> >> default:
> >> ret = -EINVAL;
> >> }
> >> }
> >>
> >> ??
> >
> > Yes, exactly that.
>
> I'll change it, no worries.
>
> >> >> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> >> >> + if (ret < 0)
> >> >> + return ret;
> >> >> +
> >> >> + irq = pci_irq_vector(pci, 0);
> >> >> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> >> >> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> >> >> + "intel-adc", adc);
> >> >
> >> > Requesting the interrupt only after exposing userspace and in kernel
> >> > interfaces seems liable to cause problem.
> >>
> >> It goes the other way around, rather. If I request the interrupt before,
> >> then I could get interrupts before IIO subsystem knows about the device,
> >> no?
> >
> > Only if your device comes up with interrupts already enabled. Normally they
> > only turn on in response to some userspace interaction, such as enabling
> > a threshold. Unless there is a hardware limitation, then at startup no
> > such interrupt sources should be enabled.
>
> We have FW that _may_ use the hardware and leave it at unpredictable
> state. There is a potential for irq status bits being left over by
> FW.
>
If it is only status bits rather than actually leaving the interrupt enabled
I'd do whatever actions are needed to clear those so you are in a
clean state when the driver loads (basically do the equivalent of what
you would get if there was a "soft reset" function.
Unpredictable is nasty! :)
Jonathan
>
> >> >> + if (ret)
> >> >> + goto err;
> >> >> +
> >> >> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> >> >> + pm_runtime_use_autosuspend(&pci->dev);
> >> >> + pm_runtime_put_autosuspend(&pci->dev);
> >> >> + pm_runtime_allow(&pci->dev);
> >> >> +
> >> >> + return 0;
> >> >> +
> >> >> +err:
> >> >> + pci_free_irq_vectors(pci);
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> +static void intel_adc_remove(struct pci_dev *pci)
> >> >> +{
> >> >> + pm_runtime_forbid(&pci->dev);
> >> >> + pm_runtime_get_noresume(&pci->dev);
> >> >> +
> >> >> + pci_free_irq_vectors(pci);
> >> >
> >> > There is a theoretical race here. We have freed the irq vectors
> >> > before removing the userspace and in kernel interfaces.
> >>
> >> There's no way to sort this out, though. Is there? Apart from switching
> >> away from device managed resources.
> >
> > There is the rather helpful,
> >
> > devm_add_action_or_reset() that allows you to define additional cleanup
> > actions to be automatically run. It's either that, or stop using
> > device managed resources from the point at which something that isn't
> > device managed occurs in probe.
>
> I'll have a look, thanks.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-07 9:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-16 10:34 [PATCH] iio: adc: add support for Intel ADC Felipe Balbi
2019-09-17 13:38 ` Jonathan Cameron
2019-09-27 10:57 ` Felipe Balbi
2019-10-01 9:25 ` [PATCH v2] " Felipe Balbi
2019-10-03 13:23 ` Jonathan Cameron
2019-10-03 13:38 ` Jonathan Cameron
2019-10-03 13:23 ` [PATCH] " Jonathan Cameron
2019-10-04 6:39 ` Felipe Balbi
2019-10-07 9:15 ` Jonathan Cameron
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.