[PATCH v2 0/2] iio: adc: npcm: add NPCM ADC driver

[PATCH v2 0/2] iio: adc: npcm: add NPCM ADC driver

[Tomer Maimon]

This patch set adds Analog-to-Digital Converter (ADC) support 
for the Nuvoton NPCM Baseboard Management Controller (BMC).

The NPCM ADC is a 10-bit converter for eight channel inputs.

The NPCM ADC driver tested on NPCM750 evaluation board.

Addressed comments from:.
 - Peter Meerwald-Stadler: https://www.spinics.net/lists/linux-iio/msg42159.html
 - Jonathan Cameron: https://www.spinics.net/lists/linux-iio/msg42183.html
				  
Changes since version 1:
 - Add NPCM prefix.
 - Remove unnecessary parameter initialization.
 - Modify read function to avoid racy condition.
 - Reading the reference voltage when needed.
 - Modify dt-binding documentation according Jonathan comments.
 
Tomer Maimon (2):
  dt-binding: iio: add NPCM ADC documentation
  iio: adc: add NPCM ADC driver

 .../bindings/iio/adc/nuvoton,npcm-adc.txt          |  35 +++
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/npcm_adc.c                         | 338 +++++++++++++++++++++
 4 files changed, 384 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
 create mode 100644 drivers/iio/adc/npcm_adc.c

-- 
2.14.1

[PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Tomer Maimon]

Added device tree binding documentation for Nuvoton BMC
NPCM Analog-to-Digital Converter(ADC).

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/iio/adc/nuvoton,npcm-adc.txt          | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
new file mode 100644
index 000000000000..1b8132cd9060
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
@@ -0,0 +1,35 @@
+Nuvoton NPCM Analog to Digital Converter (ADC)
+
+The NPCM ADC is a 10-bit converter for eight channel inputs.
+
+Required properties:
+- compatible: "nuvoton,npcm750-adc" for the NPCM7XX BMC.
+- reg: specifies physical base address and size of the registers.
+- interrupts: Contain the ADC interrupt with flags for falling edge.
+
+Optional properties:
+- clocks: phandle of ADC reference clock, in case the clock is not
+		  added the ADC will use the default ADC sample rate.
+- vref-supply: The regulator supply ADC reference voltage, in case the
+			   vref-supply is not added the ADC will use internal voltage
+			   reference.
+
+Required Node in the NPCM7xx BMC:
+An additional register is present in the NPCM7xx SOC which is
+assumed to be in the same device tree, with and marked as
+compatible with "nuvoton,npcm750-rst".
+
+Example:
+
+adc: adc@f000c000 {
+	compatible = "nuvoton,npcm750-adc";
+	reg = <0xf000c000 0x8>;
+	interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&clk NPCM7XX_CLK_ADC>;
+};
+
+rst: rst@f0801000 {
+	compatible = "nuvoton,npcm750-rst", "syscon",
+	"simple-mfd";
+	reg = <0xf0801000 0x6C>;
+};
-- 
2.14.1

Re: [PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Jonathan Cameron]

On Wed,  9 Jan 2019 18:43:42 +0200
Tomer Maimon <tmaimon77@gmail.com> wrote:

> Added device tree binding documentation for Nuvoton BMC
> NPCM Analog-to-Digital Converter(ADC).
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
This looks fine to me, but I would like Rob's confirmation that
he is happy with the reset part in particular.

Thanks,

Jonathan

> ---
>  .../bindings/iio/adc/nuvoton,npcm-adc.txt          | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
> new file mode 100644
> index 000000000000..1b8132cd9060
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
> @@ -0,0 +1,35 @@
> +Nuvoton NPCM Analog to Digital Converter (ADC)
> +
> +The NPCM ADC is a 10-bit converter for eight channel inputs.
> +
> +Required properties:
> +- compatible: "nuvoton,npcm750-adc" for the NPCM7XX BMC.
> +- reg: specifies physical base address and size of the registers.
> +- interrupts: Contain the ADC interrupt with flags for falling edge.
> +
> +Optional properties:
> +- clocks: phandle of ADC reference clock, in case the clock is not
> +		  added the ADC will use the default ADC sample rate.
> +- vref-supply: The regulator supply ADC reference voltage, in case the
> +			   vref-supply is not added the ADC will use internal voltage
> +			   reference.
> +
> +Required Node in the NPCM7xx BMC:
> +An additional register is present in the NPCM7xx SOC which is
> +assumed to be in the same device tree, with and marked as
> +compatible with "nuvoton,npcm750-rst".
> +
> +Example:
> +
> +adc: adc@f000c000 {
> +	compatible = "nuvoton,npcm750-adc";
> +	reg = <0xf000c000 0x8>;
> +	interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&clk NPCM7XX_CLK_ADC>;
> +};
> +
> +rst: rst@f0801000 {
> +	compatible = "nuvoton,npcm750-rst", "syscon",
> +	"simple-mfd";
> +	reg = <0xf0801000 0x6C>;
> +};

Re: [PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Rob Herring]

On Wed,  9 Jan 2019 18:43:42 +0200, Tomer Maimon wrote:
> Added device tree binding documentation for Nuvoton BMC
> NPCM Analog-to-Digital Converter(ADC).
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/iio/adc/nuvoton,npcm-adc.txt          | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,npcm-adc.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Joel Stanley]

On Thu, 10 Jan 2019 at 03:44, Tomer Maimon <tmaimon77@gmail.com> wrote:

> +Required Node in the NPCM7xx BMC:
> +An additional register is present in the NPCM7xx SOC which is
> +assumed to be in the same device tree, with and marked as
> +compatible with "nuvoton,npcm750-rst".

Is there a reason you don't include a phandle to the reset node?

I think doing that would make more sense.

> +adc: adc@f000c000 {
> +       compatible = "nuvoton,npcm750-adc";
> +       reg = <0xf000c000 0x8>;
> +       interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +       clocks = <&clk NPCM7XX_CLK_ADC>;
> +};

> +
> +rst: rst@f0801000 {
> +       compatible = "nuvoton,npcm750-rst", "syscon",
> +       "simple-mfd";
> +       reg = <0xf0801000 0x6C>;
> +};

Re: [PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Joel Stanley]

On Sat, 19 Jan 2019 at 02:12, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Hi Joel,
>
> Thanks for bringing this to my attention,
>
> I think I will leave it the same way it is now because I will like to develop the reset driver and to handle the NPCM7xx SOC resets.

You could also do that.

But I was suggesting you use a phandle, so you could then find the
node you want without searching the entire device tree for the node
with the correct compatible.

Re: [PATCH v2 1/2] dt-binding: iio: add NPCM ADC documentation

[Rob Herring]

On Sun, Jan 20, 2019 at 7:48 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Sat, 19 Jan 2019 at 02:12, Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Hi Joel,
> >
> > Thanks for bringing this to my attention,
> >
> > I think I will leave it the same way it is now because I will like to develop the reset driver and to handle the NPCM7xx SOC resets.
>
> You could also do that.
>
> But I was suggesting you use a phandle, so you could then find the
> node you want without searching the entire device tree for the node
> with the correct compatible.

That's not really any more efficient. You just search the entire tree
for the matching phandle number instead. Well, that was true until we
recently added the phandle cache.

In any case, it you plan to move to the reset binding (which would be
good), then it's better to have nothing in the DT and add something
rather than change the DT binding.

Rob

[PATCH v2 2/2] iio: adc: add NPCM ADC driver

[Tomer Maimon]

Add Nuvoton NPCM BMC Analog-to-Digital Converter(ADC) driver.

The NPCM ADC is a 10-bit converter for eight channel inputs.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/iio/adc/Kconfig    |  10 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/npcm_adc.c | 338 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/iio/adc/npcm_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7a3ca4ec0cb7..2d1e70a7d5c4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -576,6 +576,16 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config NPCM_ADC
+	tristate "Nuvoton NPCM ADC driver"
+	depends on ARCH_NPCM || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to build support for Nuvoton NPCM ADC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called npcm_adc.
+
 config PALMAS_GPADC
 	tristate "TI Palmas General Purpose ADC"
 	depends on MFD_PALMAS
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 07df37f621bd..3337eb1f4c30 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
 obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
 obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
diff --git a/drivers/iio/adc/npcm_adc.c b/drivers/iio/adc/npcm_adc.c
new file mode 100644
index 000000000000..c364f2dbd702
--- /dev/null
+++ b/drivers/iio/adc/npcm_adc.c
@@ -0,0 +1,338 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Nuvoton Technology corporation.
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/io.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+
+struct npcm_adc {
+	bool int_status;
+	u32 adc_sample_hz;
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *adc_clk;
+	wait_queue_head_t wq;
+	struct regulator *vref;
+	struct regmap *rst_regmap;
+};
+
+/* NPCM7xx reset module */
+#define NPCM7XX_IPSRST1_OFFSET		0x020
+#define NPCM7XX_IPSRST1_ADC_RST		BIT(27)
+
+/* ADC registers */
+#define NPCM_ADCCON	 0x00
+#define NPCM_ADCDATA	 0x04
+
+/* ADCCON Register Bits */
+#define NPCM_ADCCON_ADC_INT_EN		BIT(21)
+#define NPCM_ADCCON_REFSEL		BIT(19)
+#define NPCM_ADCCON_ADC_INT_ST		BIT(18)
+#define NPCM_ADCCON_ADC_EN		BIT(17)
+#define NPCM_ADCCON_ADC_RST		BIT(16)
+#define NPCM_ADCCON_ADC_CONV		BIT(13)
+
+#define NPCM_ADCCON_CH_MASK		GENMASK(27, 24)
+#define NPCM_ADCCON_CH(x)		((x) << 24)
+#define NPCM_ADCCON_DIV_SHIFT		1
+#define NPCM_ADCCON_DIV_MASK		GENMASK(8, 1)
+#define NPCM_ADC_DATA_MASK(x)		((x) & GENMASK(9, 0))
+
+#define NPCM_ADC_ENABLE		(NPCM_ADCCON_ADC_EN | NPCM_ADCCON_ADC_INT_EN)
+
+/* ADC General Definition */
+#define NPCM_RESOLUTION_BITS		10
+#define NPCM_ADC_DEFAULT_SAMPLE_RATE	12500000
+#define NPCM_INT_VREF_MV		2000
+
+#define NPCM_ADC_CHAN(ch) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = ch,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+}
+
+static const struct iio_chan_spec npcm_adc_iio_channels[] = {
+	NPCM_ADC_CHAN(0),
+	NPCM_ADC_CHAN(1),
+	NPCM_ADC_CHAN(2),
+	NPCM_ADC_CHAN(3),
+	NPCM_ADC_CHAN(4),
+	NPCM_ADC_CHAN(5),
+	NPCM_ADC_CHAN(6),
+	NPCM_ADC_CHAN(7),
+};
+
+static irqreturn_t npcm_adc_isr(int irq, void *data)
+{
+	u32 regtemp;
+	struct iio_dev *indio_dev = data;
+	struct npcm_adc *info = iio_priv(indio_dev);
+
+	regtemp = ioread32(info->regs + NPCM_ADCCON);
+	if (regtemp & NPCM_ADCCON_ADC_INT_ST) {
+		iowrite32(regtemp, info->regs + NPCM_ADCCON);
+		wake_up_interruptible(&info->wq);
+		info->int_status = true;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int npcm_adc_read(struct npcm_adc *info, int *val, u8 channel)
+{
+	int ret;
+	u32 regtemp;
+
+	/* Select ADC channel */
+	regtemp = ioread32(info->regs + NPCM_ADCCON);
+	regtemp &= ~NPCM_ADCCON_CH_MASK;
+	info->int_status = false;
+	iowrite32(regtemp | NPCM_ADCCON_CH(channel) |
+		  NPCM_ADCCON_ADC_CONV, info->regs + NPCM_ADCCON);
+
+	ret = wait_event_interruptible_timeout(info->wq, info->int_status,
+					       msecs_to_jiffies(10));
+	if (ret == 0) {
+		regtemp = ioread32(info->regs + NPCM_ADCCON);
+		if ((regtemp & NPCM_ADCCON_ADC_CONV) && info->rst_regmap) {
+			/* if conversion failed - reset ADC module */
+			regmap_write(info->rst_regmap, NPCM7XX_IPSRST1_OFFSET,
+				     NPCM7XX_IPSRST1_ADC_RST);
+			msleep(100);
+			regmap_write(info->rst_regmap, NPCM7XX_IPSRST1_OFFSET,
+				     0x0);
+			msleep(100);
+
+			/* Enable ADC and start conversion module */
+			iowrite32(NPCM_ADC_ENABLE | NPCM_ADCCON_ADC_CONV,
+				  info->regs + NPCM_ADCCON);
+			dev_err(info->dev, "RESET ADC Complete\n");
+		}
+		return -ETIMEDOUT;
+	}
+	if (ret < 0)
+		return ret;
+
+	*val = NPCM_ADC_DATA_MASK(ioread32(info->regs + NPCM_ADCDATA));
+
+	return 0;
+}
+
+static int npcm_adc_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val,
+			     int *val2, long mask)
+{
+	int ret;
+	int vref_uv;
+	struct npcm_adc *info = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		ret = npcm_adc_read(info, val, chan->channel);
+		mutex_unlock(&indio_dev->mlock);
+		if (ret) {
+			dev_err(info->dev, "NPCM ADC read failed\n");
+			return ret;
+		}
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (info->vref) {
+			vref_uv = regulator_get_voltage(info->vref);
+			*val = vref_uv / 1000;
+		} else {
+			*val = NPCM_INT_VREF_MV;
+		}
+		*val2 = NPCM_RESOLUTION_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->adc_sample_hz;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct iio_info npcm_adc_iio_info = {
+	.read_raw = &npcm_adc_read_raw,
+};
+
+static const struct of_device_id npcm_adc_match[] = {
+	{ .compatible = "nuvoton,npcm750-adc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, npcm_adc_match);
+
+static int npcm_adc_probe(struct platform_device *pdev)
+{
+	int ret;
+	int irq;
+	u32 div;
+	u32 reg_con;
+	struct resource *res;
+	struct npcm_adc *info;
+	struct iio_dev *indio_dev;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+	info = iio_priv(indio_dev);
+
+	info->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(info->regs))
+		return PTR_ERR(info->regs);
+
+	info->adc_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(info->adc_clk)) {
+		dev_warn(&pdev->dev, "ADC clock failed: can't read clk, assuming ADC clock rate 12.5MHz\n");
+		info->adc_sample_hz = NPCM_ADC_DEFAULT_SAMPLE_RATE;
+	} else {
+		/* calculate ADC clock sample rate */
+		reg_con = ioread32(info->regs + NPCM_ADCCON);
+		div = reg_con & NPCM_ADCCON_DIV_MASK;
+		div = div >> NPCM_ADCCON_DIV_SHIFT;
+		info->adc_sample_hz = clk_get_rate(info->adc_clk) /
+			((div + 1) * 2);
+	}
+
+	if (of_device_is_compatible(np, "nuvoton,npcm750-adc")) {
+		info->rst_regmap = syscon_regmap_lookup_by_compatible
+			("nuvoton,npcm750-rst");
+		if (IS_ERR(info->rst_regmap)) {
+			dev_err(&pdev->dev, "Failed to find nuvoton,npcm750-rst\n");
+			ret = PTR_ERR(info->rst_regmap);
+			goto err_disable_clk;
+		}
+	}
+
+	reg_con = ioread32(info->regs + NPCM_ADCCON);
+	info->vref = devm_regulator_get_optional(&pdev->dev, "vref");
+	if (!IS_ERR(info->vref)) {
+		ret = regulator_enable(info->vref);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't enable ADC reference voltage\n");
+			goto err_disable_clk;
+		}
+
+		iowrite32(reg_con & ~NPCM_ADCCON_REFSEL,
+			  info->regs + NPCM_ADCCON);
+	} else {
+		/*
+		 * Any error which is not ENODEV indicates the regulator
+		 * has been specified and so is a failure case.
+		 */
+		if (PTR_ERR(info->vref) != -ENODEV) {
+			ret = PTR_ERR(info->vref);
+			goto err_disable_clk;
+		}
+
+		/* Use internal reference */
+		iowrite32(reg_con | NPCM_ADCCON_REFSEL,
+			  info->regs + NPCM_ADCCON);
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(dev, "failed getting interrupt resource\n");
+		ret = -EINVAL;
+		goto err_disable_vref_reg;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, npcm_adc_isr, 0,
+			       "NPCM_ADC", indio_dev);
+	if (ret < 0) {
+		dev_err(dev, "failed requesting interrupt\n");
+		goto err_disable_vref_reg;
+	}
+
+	init_waitqueue_head(&info->wq);
+
+	reg_con = ioread32(info->regs + NPCM_ADCCON);
+	reg_con |= NPCM_ADC_ENABLE;
+
+	/* Enable the ADC Module */
+	iowrite32(reg_con, info->regs + NPCM_ADCCON);
+
+	/* Start ADC conversion */
+	iowrite32(reg_con | NPCM_ADCCON_ADC_CONV, info->regs + NPCM_ADCCON);
+
+	platform_set_drvdata(pdev, indio_dev);
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->info = &npcm_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = npcm_adc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(npcm_adc_iio_channels);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't register the device.\n");
+		goto err_disable_vref_reg;
+	}
+
+	pr_info("NPCM ADC driver probed\n");
+
+	return 0;
+
+err_disable_vref_reg:
+	if (!IS_ERR(info->vref))
+		regulator_disable(info->vref);
+err_disable_clk:
+	clk_disable_unprepare(info->adc_clk);
+
+	return ret;
+}
+
+static int npcm_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct npcm_adc *info = iio_priv(indio_dev);
+	u32 regtemp;
+
+	regtemp = ioread32(info->regs + NPCM_ADCCON);
+
+	/* Disable the ADC Module */
+	iowrite32(regtemp & ~NPCM_ADCCON_ADC_EN, info->regs + NPCM_ADCCON);
+
+	iio_device_unregister(indio_dev);
+	clk_disable_unprepare(info->adc_clk);
+	if (!IS_ERR(info->vref))
+		regulator_disable(info->vref);
+
+	return 0;
+}
+
+static struct platform_driver npcm_adc_driver = {
+	.probe		= npcm_adc_probe,
+	.remove		= npcm_adc_remove,
+	.driver		= {
+		.name	= "npcm_adc",
+		.of_match_table = npcm_adc_match,
+	},
+};
+
+module_platform_driver(npcm_adc_driver);
+
+MODULE_DESCRIPTION("Nuvoton NPCM ADC Driver");
+MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.14.1

Re: [PATCH v2 2/2] iio: adc: add NPCM ADC driver

[Jonathan Cameron]

On Wed,  9 Jan 2019 18:43:43 +0200
Tomer Maimon <tmaimon77@gmail.com> wrote:

> Add Nuvoton NPCM BMC Analog-to-Digital Converter(ADC) driver.
> 
> The NPCM ADC is a 10-bit converter for eight channel inputs.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
Hi Tomer,

Only remaining element I think needs tidying up is the relative
ordering of probe vs remove and the interaction with devm managed
cleanup.

I really like to see the two elements being as near as possible
to opposite in order. It's a lot harder to review if they aren't.
Chances are there isn't a race condition in here, but if they are
in the 'right' order it becomes much more obvious that there isn't!

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig    |  10 ++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/npcm_adc.c | 338 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/iio/adc/npcm_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7a3ca4ec0cb7..2d1e70a7d5c4 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -576,6 +576,16 @@ config NAU7802
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nau7802.
>  
> +config NPCM_ADC
> +	tristate "Nuvoton NPCM ADC driver"
> +	depends on ARCH_NPCM || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Say yes here to build support for Nuvoton NPCM ADC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called npcm_adc.
> +
>  config PALMAS_GPADC
>  	tristate "TI Palmas General Purpose ADC"
>  	depends on MFD_PALMAS
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 07df37f621bd..3337eb1f4c30 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>  obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
>  obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
>  obj-$(CONFIG_NAU7802) += nau7802.o
> +obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
> diff --git a/drivers/iio/adc/npcm_adc.c b/drivers/iio/adc/npcm_adc.c
> new file mode 100644
> index 000000000000..c364f2dbd702
> --- /dev/null
> +++ b/drivers/iio/adc/npcm_adc.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Nuvoton Technology corporation.
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/io.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spinlock.h>
> +#include <linux/uaccess.h>
> +
> +struct npcm_adc {
> +	bool int_status;
> +	u32 adc_sample_hz;
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct clk *adc_clk;
> +	wait_queue_head_t wq;
> +	struct regulator *vref;
> +	struct regmap *rst_regmap;
> +};
> +
> +/* NPCM7xx reset module */
> +#define NPCM7XX_IPSRST1_OFFSET		0x020
> +#define NPCM7XX_IPSRST1_ADC_RST		BIT(27)
> +
> +/* ADC registers */
> +#define NPCM_ADCCON	 0x00
> +#define NPCM_ADCDATA	 0x04
> +
> +/* ADCCON Register Bits */
> +#define NPCM_ADCCON_ADC_INT_EN		BIT(21)
> +#define NPCM_ADCCON_REFSEL		BIT(19)
> +#define NPCM_ADCCON_ADC_INT_ST		BIT(18)
> +#define NPCM_ADCCON_ADC_EN		BIT(17)
> +#define NPCM_ADCCON_ADC_RST		BIT(16)
> +#define NPCM_ADCCON_ADC_CONV		BIT(13)
> +
> +#define NPCM_ADCCON_CH_MASK		GENMASK(27, 24)
> +#define NPCM_ADCCON_CH(x)		((x) << 24)
> +#define NPCM_ADCCON_DIV_SHIFT		1
> +#define NPCM_ADCCON_DIV_MASK		GENMASK(8, 1)
> +#define NPCM_ADC_DATA_MASK(x)		((x) & GENMASK(9, 0))
> +
> +#define NPCM_ADC_ENABLE		(NPCM_ADCCON_ADC_EN | NPCM_ADCCON_ADC_INT_EN)
> +
> +/* ADC General Definition */
> +#define NPCM_RESOLUTION_BITS		10
> +#define NPCM_ADC_DEFAULT_SAMPLE_RATE	12500000
> +#define NPCM_INT_VREF_MV		2000
> +
> +#define NPCM_ADC_CHAN(ch) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = ch,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +}
> +
> +static const struct iio_chan_spec npcm_adc_iio_channels[] = {
> +	NPCM_ADC_CHAN(0),
> +	NPCM_ADC_CHAN(1),
> +	NPCM_ADC_CHAN(2),
> +	NPCM_ADC_CHAN(3),
> +	NPCM_ADC_CHAN(4),
> +	NPCM_ADC_CHAN(5),
> +	NPCM_ADC_CHAN(6),
> +	NPCM_ADC_CHAN(7),
> +};
> +
> +static irqreturn_t npcm_adc_isr(int irq, void *data)
> +{
> +	u32 regtemp;
> +	struct iio_dev *indio_dev = data;
> +	struct npcm_adc *info = iio_priv(indio_dev);
> +
> +	regtemp = ioread32(info->regs + NPCM_ADCCON);
> +	if (regtemp & NPCM_ADCCON_ADC_INT_ST) {
> +		iowrite32(regtemp, info->regs + NPCM_ADCCON);
> +		wake_up_interruptible(&info->wq);
> +		info->int_status = true;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int npcm_adc_read(struct npcm_adc *info, int *val, u8 channel)
> +{
> +	int ret;
> +	u32 regtemp;
> +
> +	/* Select ADC channel */
> +	regtemp = ioread32(info->regs + NPCM_ADCCON);
> +	regtemp &= ~NPCM_ADCCON_CH_MASK;
> +	info->int_status = false;
> +	iowrite32(regtemp | NPCM_ADCCON_CH(channel) |
> +		  NPCM_ADCCON_ADC_CONV, info->regs + NPCM_ADCCON);
> +
> +	ret = wait_event_interruptible_timeout(info->wq, info->int_status,
> +					       msecs_to_jiffies(10));
> +	if (ret == 0) {
> +		regtemp = ioread32(info->regs + NPCM_ADCCON);
> +		if ((regtemp & NPCM_ADCCON_ADC_CONV) && info->rst_regmap) {
> +			/* if conversion failed - reset ADC module */
> +			regmap_write(info->rst_regmap, NPCM7XX_IPSRST1_OFFSET,
> +				     NPCM7XX_IPSRST1_ADC_RST);
> +			msleep(100);
> +			regmap_write(info->rst_regmap, NPCM7XX_IPSRST1_OFFSET,
> +				     0x0);
> +			msleep(100);
> +
> +			/* Enable ADC and start conversion module */
> +			iowrite32(NPCM_ADC_ENABLE | NPCM_ADCCON_ADC_CONV,
> +				  info->regs + NPCM_ADCCON);
> +			dev_err(info->dev, "RESET ADC Complete\n");
> +		}
> +		return -ETIMEDOUT;
> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = NPCM_ADC_DATA_MASK(ioread32(info->regs + NPCM_ADCDATA));
> +
> +	return 0;
> +}
> +
> +static int npcm_adc_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	int ret;
> +	int vref_uv;
> +	struct npcm_adc *info = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		ret = npcm_adc_read(info, val, chan->channel);
> +		mutex_unlock(&indio_dev->mlock);
> +		if (ret) {
> +			dev_err(info->dev, "NPCM ADC read failed\n");
> +			return ret;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (info->vref) {
> +			vref_uv = regulator_get_voltage(info->vref);
> +			*val = vref_uv / 1000;
> +		} else {
> +			*val = NPCM_INT_VREF_MV;
> +		}
> +		*val2 = NPCM_RESOLUTION_BITS;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = info->adc_sample_hz;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info npcm_adc_iio_info = {
> +	.read_raw = &npcm_adc_read_raw,
> +};
> +
> +static const struct of_device_id npcm_adc_match[] = {
> +	{ .compatible = "nuvoton,npcm750-adc", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, npcm_adc_match);
> +
> +static int npcm_adc_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int irq;
> +	u32 div;
> +	u32 reg_con;
> +	struct resource *res;
> +	struct npcm_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	info = iio_priv(indio_dev);
> +
> +	info->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(info->regs))
> +		return PTR_ERR(info->regs);
> +
> +	info->adc_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(info->adc_clk)) {
> +		dev_warn(&pdev->dev, "ADC clock failed: can't read clk, assuming ADC clock rate 12.5MHz\n");
> +		info->adc_sample_hz = NPCM_ADC_DEFAULT_SAMPLE_RATE;

I think this is a device tree bug.  We should have a fixed clock defined
here if nothing else. If there isn't one, then don't load the driver.
We should not be guessing the frequency.


> +	} else {
> +		/* calculate ADC clock sample rate */
> +		reg_con = ioread32(info->regs + NPCM_ADCCON);
> +		div = reg_con & NPCM_ADCCON_DIV_MASK;
> +		div = div >> NPCM_ADCCON_DIV_SHIFT;
> +		info->adc_sample_hz = clk_get_rate(info->adc_clk) /
> +			((div + 1) * 2);
> +	}
> +
> +	if (of_device_is_compatible(np, "nuvoton,npcm750-adc")) {
> +		info->rst_regmap = syscon_regmap_lookup_by_compatible
> +			("nuvoton,npcm750-rst");
> +		if (IS_ERR(info->rst_regmap)) {
> +			dev_err(&pdev->dev, "Failed to find nuvoton,npcm750-rst\n");
> +			ret = PTR_ERR(info->rst_regmap);
> +			goto err_disable_clk;
> +		}
> +	}
> +
> +	reg_con = ioread32(info->regs + NPCM_ADCCON);
> +	info->vref = devm_regulator_get_optional(&pdev->dev, "vref");
> +	if (!IS_ERR(info->vref)) {
> +		ret = regulator_enable(info->vref);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't enable ADC reference voltage\n");
> +			goto err_disable_clk;
> +		}
> +
> +		iowrite32(reg_con & ~NPCM_ADCCON_REFSEL,
> +			  info->regs + NPCM_ADCCON);
> +	} else {
> +		/*
> +		 * Any error which is not ENODEV indicates the regulator
> +		 * has been specified and so is a failure case.
> +		 */
> +		if (PTR_ERR(info->vref) != -ENODEV) {
> +			ret = PTR_ERR(info->vref);
> +			goto err_disable_clk;
> +		}
> +
> +		/* Use internal reference */
> +		iowrite32(reg_con | NPCM_ADCCON_REFSEL,
> +			  info->regs + NPCM_ADCCON);
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0) {
> +		dev_err(dev, "failed getting interrupt resource\n");
> +		ret = -EINVAL;
> +		goto err_disable_vref_reg;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, npcm_adc_isr, 0,
> +			       "NPCM_ADC", indio_dev);
> +	if (ret < 0) {
I'm not keen on mixing the regulator being enabled in a non devm
managed path and the irq being requested using devm after it.

The reason is that the probe and remove paths then take non obvious
ordering.  One nice fix for this is to do the regulator disable
using a devm_add_action_or_reset call to make it part of the
managed unwind.

The same is true for the clk_prepare_enable call.
Alternative is don't use managed requests for the irq etc.


> +		dev_err(dev, "failed requesting interrupt\n");
> +		goto err_disable_vref_reg;
> +	}
> +
> +	init_waitqueue_head(&info->wq);
> +
> +	reg_con = ioread32(info->regs + NPCM_ADCCON);
> +	reg_con |= NPCM_ADC_ENABLE;
> +
> +	/* Enable the ADC Module */
> +	iowrite32(reg_con, info->regs + NPCM_ADCCON);
> +
> +	/* Start ADC conversion */
> +	iowrite32(reg_con | NPCM_ADCCON_ADC_CONV, info->regs + NPCM_ADCCON);
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->info = &npcm_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = npcm_adc_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(npcm_adc_iio_channels);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't register the device.\n");
> +		goto err_disable_vref_reg;
> +	}
> +
> +	pr_info("NPCM ADC driver probed\n");
> +
> +	return 0;
> +
> +err_disable_vref_reg:
> +	if (!IS_ERR(info->vref))
> +		regulator_disable(info->vref);
> +err_disable_clk:
> +	clk_disable_unprepare(info->adc_clk);
> +
> +	return ret;
> +}
> +
> +static int npcm_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct npcm_adc *info = iio_priv(indio_dev);
> +	u32 regtemp;
> +
> +	regtemp = ioread32(info->regs + NPCM_ADCCON);
> +
> +	/* Disable the ADC Module */
> +	iowrite32(regtemp & ~NPCM_ADCCON_ADC_EN, info->regs + NPCM_ADCCON);
> +
> +	iio_device_unregister(indio_dev);
> +	clk_disable_unprepare(info->adc_clk);
> +	if (!IS_ERR(info->vref))
> +		regulator_disable(info->vref);
This should look like the error paths above.  The fact it doesn't
means that the ordering at least needs more thought in review than it
should.  

> +
> +	return 0;
> +}
> +
> +static struct platform_driver npcm_adc_driver = {
> +	.probe		= npcm_adc_probe,
> +	.remove		= npcm_adc_remove,
> +	.driver		= {
> +		.name	= "npcm_adc",
> +		.of_match_table = npcm_adc_match,
> +	},
> +};
> +
> +module_platform_driver(npcm_adc_driver);
> +
> +MODULE_DESCRIPTION("Nuvoton NPCM ADC Driver");
> +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> +MODULE_LICENSE("GPL v2");