[PATCH v2 1/3] iio: adc: add support for mcp3911

[PATCH v2 1/3] iio: adc: add support for mcp3911

[Marcus Folkesson]

MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v2:
    	- cleanups and bugfixes (thanks Peter Meerwald-Stadler)
    	- drop hardware gain
    	- use the presence or lack of regulator to indicate if we go for internal or external voltage reference
    	- do not store device node in private struct
    	- drop support to set width in devicetree
    	- use the presence or lack of clock to indicate if we go for internal or external clock

 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/mcp3911.c | 366 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/iio/adc/mcp3911.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 15606f237480..f9a41fa96fcc 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -501,6 +501,16 @@ config MCP3422
 	  This driver can also be built as a module. If so, the module will be
 	  called mcp3422.
 
+config MCP3911
+	tristate "Microchip Technology MCP3911 driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Microchip Technology's MCP3911
+	  analog to digital converter.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called mcp3911.
+
 config MEDIATEK_MT6577_AUXADC
         tristate "MediaTek AUXADC driver"
         depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 28a9423997f3..3cfebfff7d26 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
+obj-$(CONFIG_MCP3911) += mcp3911.o
 obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
 obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
 obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
new file mode 100644
index 000000000000..29aa39930ead
--- /dev/null
+++ b/drivers/iio/adc/mcp3911.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Microchip MCP3911, Two-channel Analog Front End
+ *
+ * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
+ * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
+ *
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define MCP3911_REG_CHANNEL0		0x00
+#define MCP3911_REG_CHANNEL1		0x03
+#define MCP3911_REG_MOD			0x06
+#define MCP3911_REG_PHASE		0x07
+#define MCP3911_REG_GAIN		0x09
+
+#define MCP3911_REG_STATUSCOM		0x0a
+#define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
+#define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
+#define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
+#define MCP3911_STATUSCOM_EN_GAINCAL	BIT(1)
+
+#define MCP3911_REG_CONFIG		0x0c
+#define MCP3911_CONFIG_CLKEXT		BIT(1)
+#define MCP3911_CONFIG_VREFEXT		BIT(2)
+
+#define MCP3911_REG_OFFCAL_CH0		0x0e
+#define MCP3911_REG_GAINCAL_CH0		0x11
+#define MCP3911_REG_OFFCAL_CH1		0x14
+#define MCP3911_REG_GAINCAL_CH1		0x17
+#define MCP3911_REG_VREFCAL		0x1a
+
+#define MCP3911_CHANNEL(x)		(MCP3911_REG_CHANNEL0 + x * 3)
+#define MCP3911_OFFCAL(x)		(MCP3911_REG_OFFCAL_CH0 + x * 6)
+
+/* Internal voltage reference in uV */
+#define MCP3911_INT_VREF_UV		1200000
+
+#define MCP3911_REG_READ(reg, id)	((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
+#define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
+
+#define MCP3911_NUM_CHANNELS		2
+
+struct mcp3911 {
+	struct spi_device *spi;
+	struct mutex lock;
+	struct regulator *vref;
+	struct clk *adc_clk;
+	u32 dev_addr;
+};
+
+static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
+{
+	int ret;
+
+	reg = MCP3911_REG_READ(reg, adc->dev_addr);
+	ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
+	if (ret < 0)
+		return ret;
+
+	be32_to_cpus(val);
+	*val >>= ((4 - len) * 8);
+	dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val,
+			reg>>1);
+	return ret;
+}
+
+static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
+{
+	dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg);
+
+	val <<= (3 - len) * 8;
+	cpu_to_be32s(&val);
+	val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
+
+	return spi_write(adc->spi, &val, len+1);
+}
+
+static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
+		u32 val, u8 len)
+{
+	u32 tmp;
+	int ret;
+
+	ret = mcp3911_read(adc, reg, &tmp, len);
+	if (ret)
+		return ret;
+
+	val &= mask;
+	val |= tmp & ~mask;
+	return mcp3911_write(adc, reg, val, len);
+}
+
+static int mcp3911_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+	struct mcp3911 *adc = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&adc->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = mcp3911_read(adc,
+				MCP3911_CHANNEL(channel->channel), val, 3);
+		if (ret)
+			goto out;
+
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_OFFSET:
+		ret = mcp3911_read(adc,
+				MCP3911_OFFCAL(channel->channel), val, 3);
+		if (ret)
+			goto out;
+
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		if (adc->vref) {
+			ret = regulator_get_voltage(adc->vref);
+			if (ret < 0) {
+				dev_err(indio_dev->dev.parent,
+					"failed to get vref voltage: %d\n",
+				       ret);
+				goto out;
+			}
+
+			*val = ret / 1000;
+		} else {
+			*val = MCP3911_INT_VREF_UV;
+		}
+
+		*val2 = 24;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
+		break;
+	}
+
+out:
+	mutex_unlock(&adc->lock);
+	return ret;
+}
+
+static int mcp3911_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *channel, int val,
+			    int val2, long mask)
+{
+	struct mcp3911 *adc = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&adc->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		if (val2 != 0) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* Write offset */
+		ret = mcp3911_write(adc, MCP3911_OFFCAL(channel->channel), val,
+				3);
+		if (ret)
+			goto out;
+
+		/* Enable offset*/
+		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM,
+				MCP3911_STATUSCOM_EN_OFFCAL,
+				MCP3911_STATUSCOM_EN_OFFCAL, 2);
+		break;
+	}
+
+out:
+	mutex_unlock(&adc->lock);
+	return ret;
+}
+
+#define MCP3911_CHAN(idx) {					\
+		.type = IIO_VOLTAGE,				\
+		.indexed = 1,					\
+		.channel = idx,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+			BIT(IIO_CHAN_INFO_OFFSET) |		\
+			BIT(IIO_CHAN_INFO_SCALE),		\
+}
+
+static const struct iio_chan_spec mcp3911_channels[] = {
+	MCP3911_CHAN(0),
+	MCP3911_CHAN(1),
+};
+
+static const struct iio_info mcp3911_info = {
+	.read_raw = mcp3911_read_raw,
+	.write_raw = mcp3911_write_raw,
+};
+
+static int mcp3911_config(struct mcp3911 *adc, struct device_node *of_node)
+{
+	u32 configreg;
+	int ret;
+
+	of_property_read_u32(of_node, "device-addr", &adc->dev_addr);
+	if (adc->dev_addr > 3) {
+		dev_err(&adc->spi->dev,
+				"invalid device address (%i). Must be in range 0-3.\n",
+				adc->dev_addr);
+		return -EINVAL;
+	}
+	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
+
+	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
+	if (ret)
+		return ret;
+
+	if (adc->vref) {
+		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
+		configreg |= MCP3911_CONFIG_VREFEXT;
+
+	} else {
+		dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
+		configreg &= ~MCP3911_CONFIG_VREFEXT;
+	}
+
+	if (adc->adc_clk) {
+		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
+		configreg |= MCP3911_CONFIG_CLKEXT;
+	} else {
+		dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
+		configreg &= ~MCP3911_CONFIG_CLKEXT;
+	}
+
+	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
+}
+
+static int mcp3911_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp3911 *adc;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+
+	adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
+	if (IS_ERR(adc->vref)) {
+
+		if (PTR_ERR(adc->vref) == -ENODEV) {
+			adc->vref = NULL;
+		} else {
+			dev_err(&adc->spi->dev,
+					"failed to get regulator (%ld)\n",
+					PTR_ERR(adc->vref));
+			return PTR_ERR(adc->vref);
+		}
+
+	} else {
+		ret = regulator_enable(adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	adc->adc_clk = devm_clk_get(&adc->spi->dev, "adc_clk");
+	if (IS_ERR(adc->adc_clk)) {
+		if (PTR_ERR(adc->adc_clk) == -ENOENT) {
+			adc->adc_clk = NULL;
+		} else {
+			dev_err(&adc->spi->dev,
+					"failed to get adc clk (%ld)\n",
+					PTR_ERR(adc->adc_clk));
+			ret = PTR_ERR(adc->adc_clk);
+			goto reg_disable;
+		}
+	} else {
+		ret = clk_prepare_enable(adc->adc_clk);
+		if (ret < 0) {
+			dev_err(&adc->spi->dev,
+				"Failed to enable adc_clk: %d\n", ret);
+			goto reg_disable;
+		}
+	}
+
+	ret = mcp3911_config(adc, spi->dev.of_node);
+	if (ret)
+		goto clk_disable;
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &mcp3911_info;
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->channels = mcp3911_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
+
+	mutex_init(&adc->lock);
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto clk_disable;
+
+	return ret;
+
+clk_disable:
+	if (adc->adc_clk)
+		clk_disable_unprepare(adc->adc_clk);
+reg_disable:
+	if (adc->vref)
+		regulator_disable(adc->vref);
+
+	return ret;
+}
+
+static int mcp3911_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct mcp3911 *adc = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	if (adc->vref)
+		regulator_disable(adc->vref);
+
+	return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id mcp3911_dt_ids[] = {
+	{ .compatible = "microchip,mcp3911" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
+#endif
+
+static const struct spi_device_id mcp3911_id[] = {
+	{ "mcp3911", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, mcp3911_id);
+
+static struct spi_driver mcp3911_driver = {
+	.driver = {
+		.name = "mcp3911",
+		.of_match_table = of_match_ptr(mcp3911_dt_ids),
+	},
+	.probe = mcp3911_probe,
+	.remove = mcp3911_remove,
+	.id_table = mcp3911_id,
+};
+module_spi_driver(mcp3911_driver);
+
+MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
+MODULE_AUTHOR("Kent Gustavsson <kent@minoris.se>");
+MODULE_DESCRIPTION("Microchip Technology MCP3911");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0.rc2

[PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Marcus Folkesson]

MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v2:
    	- drop channel width
    	- drop `external_vref`
    	- replace `external-clock` with proper clock bindings

 .../devicetree/bindings/iio/adc/mcp3911.txt        | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
new file mode 100644
index 000000000000..af5472f51a84
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
@@ -0,0 +1,28 @@
+* Microchip MCP3911 Dual channel analog front end (ADC)
+
+Required properties:
+ - compatible: Should be "microchip,mcp3911"
+ - reg: SPI chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+	 Documentation/devicetree/bindings/spi/spi-bus.txt.
+	 Max frequency for this chip is 20MHz.
+
+Optional properties:
+ - device-addr: Device address when multiple MCP3911 chips are present on the
+	same SPI bus. Valid values are 0-3. Defaults to 0.
+ - vref-supply: Phandle to the external reference voltage supply.
+ - clocks: Phandle and clock identifier (see clock-names)
+ - clock-names: "adc_clk" for the ADC (sampling) clock
+
+Example:
+adc@0 {
+	compatible = "microchip,mcp3911";
+	reg = <0>;
+	spi-max-frequency = <20000000>;
+	device-addr = <0>;
+	vref-supply = <&vref_reg>;
+	clocks = <&xtal>;
+	clock-names = "adc_clk";
+};
-- 
2.11.0.rc2

[PATCH v2 3/3] MAINTAINERS: Add entry for mcp3911 ADC driver

[Marcus Folkesson]

Add an entry for mcp3911 ADC driver and add myself and
Kent Gustavsson as maintainers of this driver.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v2:
    	- no changes

 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 79bb02ff812f..9276da915d9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9271,6 +9271,14 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP / ATMEL MCP3911 ADC DRIVER
+M:	Marcus Folkesson <marcus.folkesson@gmail.com>
+M:	Kent Gustavsson <kent@minoris.se>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/adc/mcp3911.c
+F:	Documentation/devicetree/bindings/iio/adc/mcp3911.txt
+
 MICROCHIP USB251XB DRIVER
 M:	Richard Leitner <richard.leitner@skidata.com>
 L:	linux-usb@vger.kernel.org
-- 
2.11.0.rc2

Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Rob Herring]

On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote:
> MCP3911 is a dual channel Analog Front End (AFE) containing two
> synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Signed-off-by: Kent Gustavsson <kent@minoris.se>
> ---
> 
> Notes:
>     v2:
>     	- drop channel width
>     	- drop `external_vref`
>     	- replace `external-clock` with proper clock bindings
> 
>  .../devicetree/bindings/iio/adc/mcp3911.txt        | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> new file mode 100644
> index 000000000000..af5472f51a84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> @@ -0,0 +1,28 @@
> +* Microchip MCP3911 Dual channel analog front end (ADC)
> +
> +Required properties:
> + - compatible: Should be "microchip,mcp3911"
> + - reg: SPI chip select number for the device
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> +	 Documentation/devicetree/bindings/spi/spi-bus.txt.
> +	 Max frequency for this chip is 20MHz.
> +
> +Optional properties:
> + - device-addr: Device address when multiple MCP3911 chips are present on the
> +	same SPI bus. Valid values are 0-3. Defaults to 0.

Isn't this the reg value?

> + - vref-supply: Phandle to the external reference voltage supply.
> + - clocks: Phandle and clock identifier (see clock-names)
> + - clock-names: "adc_clk" for the ADC (sampling) clock

Datasheet calls this clki (or mclk internally). Or just drop clock-names 
as it is pointless 
when there is only 1 clock. 

Also DR handling as an IRQ is missing.

Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]

Hello Rob,

Thank you for the review.

On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote:
> On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote:
> > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > Signed-off-by: Kent Gustavsson <kent@minoris.se>
> > ---
> > 
> > Notes:
> >     v2:
> >     	- drop channel width
> >     	- drop `external_vref`
> >     	- replace `external-clock` with proper clock bindings
> > 
> >  .../devicetree/bindings/iio/adc/mcp3911.txt        | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > new file mode 100644
> > index 000000000000..af5472f51a84
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > @@ -0,0 +1,28 @@
> > +* Microchip MCP3911 Dual channel analog front end (ADC)
> > +
> > +Required properties:
> > + - compatible: Should be "microchip,mcp3911"
> > + - reg: SPI chip select number for the device
> > +
> > +Recommended properties:
> > + - spi-max-frequency: Definition as per
> > +	 Documentation/devicetree/bindings/spi/spi-bus.txt.
> > +	 Max frequency for this chip is 20MHz.
> > +
> > +Optional properties:
> > + - device-addr: Device address when multiple MCP3911 chips are present on the
> > +	same SPI bus. Valid values are 0-3. Defaults to 0.
> 
> Isn't this the reg value?
> 

The reg value defines which chip select the chip hangs on.
The chip has support to connect up to four chips on the same SPI bus,
including the same chip select signal.

The chips are separated by the device-addr as it is part of the
communication protocol.

When reading the description for device-addr, I agree that it fits well
for the reg property as well..


> > + - vref-supply: Phandle to the external reference voltage supply.
> > + - clocks: Phandle and clock identifier (see clock-names)
> > + - clock-names: "adc_clk" for the ADC (sampling) clock
> 
> Datasheet calls this clki (or mclk internally). Or just drop clock-names 
> as it is pointless 
> when there is only 1 clock. 

I will drop the clock-names, thanks!

> 
> Also DR handling as an IRQ is missing.

I have considered using the DR signal, but as we not support buffering
yet I did not see any point in using it.

From the datasheet, section 7.1 ::

	These registers [channel registers] are latched when an
	ADC read communication occurs. When a data ready
	event occurs during a read communication, the most
	current ADC data is also latched to avoid data
	corruption issues. The three bytes of each channel are
	updated synchronously at a DRCLK rate. The three
	bytes can be accessed separately if needed but are
	refreshed synchronously.

So it should be safe to read the values independently of the DR signal.
Or maybe I'm missing something.


Best regards
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Rob Herring]

On Wed, Jul 25, 2018 at 1:04 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Hello Rob,
>
> Thank you for the review.
>
> On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote:
> > On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote:
> > > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > Signed-off-by: Kent Gustavsson <kent@minoris.se>
> > > ---
> > >
> > > Notes:
> > >     v2:
> > >             - drop channel width
> > >             - drop `external_vref`
> > >             - replace `external-clock` with proper clock bindings
> > >
> > >  .../devicetree/bindings/iio/adc/mcp3911.txt        | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > > new file mode 100644
> > > index 000000000000..af5472f51a84
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> > > @@ -0,0 +1,28 @@
> > > +* Microchip MCP3911 Dual channel analog front end (ADC)
> > > +
> > > +Required properties:
> > > + - compatible: Should be "microchip,mcp3911"
> > > + - reg: SPI chip select number for the device
> > > +
> > > +Recommended properties:
> > > + - spi-max-frequency: Definition as per
> > > +    Documentation/devicetree/bindings/spi/spi-bus.txt.
> > > +    Max frequency for this chip is 20MHz.
> > > +
> > > +Optional properties:
> > > + - device-addr: Device address when multiple MCP3911 chips are present on the
> > > +   same SPI bus. Valid values are 0-3. Defaults to 0.
> >
> > Isn't this the reg value?
> >
>
> The reg value defines which chip select the chip hangs on.
> The chip has support to connect up to four chips on the same SPI bus,
> including the same chip select signal.
>
> The chips are separated by the device-addr as it is part of the
> communication protocol.

Okay, seems pretty specific to this chip. Just add a vendor prefix.

> When reading the description for device-addr, I agree that it fits well
> for the reg property as well..

Except that reg format is already defined for SPI devices, so it's
going to have to be separate property.

> > > + - vref-supply: Phandle to the external reference voltage supply.
> > > + - clocks: Phandle and clock identifier (see clock-names)
> > > + - clock-names: "adc_clk" for the ADC (sampling) clock
> >
> > Datasheet calls this clki (or mclk internally). Or just drop clock-names
> > as it is pointless
> > when there is only 1 clock.
>
> I will drop the clock-names, thanks!
>
> >
> > Also DR handling as an IRQ is missing.
>
> I have considered using the DR signal, but as we not support buffering
> yet I did not see any point in using it.

For the binding it doesn't matter if a particular driver uses it or
not. If it's connected in a h/w design it should be in DT.

Rob

Re: [PATCH v2 1/3] iio: adc: add support for mcp3911

[Jonathan Cameron]

On Tue, 24 Jul 2018 20:30:02 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> MCP3911 is a dual channel Analog Front End (AFE) containing two
> synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Signed-off-by: Kent Gustavsson <kent@minoris.se>
Hi

I think you missed cleaning up the clock enable in the remove.
If it is safe to not do so it should also be safe in the error path
of probe.

A couple of trivial other things jumped out at me whilst reading.

Thanks,

Jonathan
> ---
> 
> Notes:
>     v2:
>     	- cleanups and bugfixes (thanks Peter Meerwald-Stadler)
>     	- drop hardware gain
>     	- use the presence or lack of regulator to indicate if we go for internal or external voltage reference
>     	- do not store device node in private struct
>     	- drop support to set width in devicetree
>     	- use the presence or lack of clock to indicate if we go for internal or external clock
> 
>  drivers/iio/adc/Kconfig   |  10 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/mcp3911.c | 366 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 drivers/iio/adc/mcp3911.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 15606f237480..f9a41fa96fcc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -501,6 +501,16 @@ config MCP3422
>  	  This driver can also be built as a module. If so, the module will be
>  	  called mcp3422.
>  
> +config MCP3911
> +	tristate "Microchip Technology MCP3911 driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Microchip Technology's MCP3911
> +	  analog to digital converter.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called mcp3911.
> +
>  config MEDIATEK_MT6577_AUXADC
>          tristate "MediaTek AUXADC driver"
>          depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 28a9423997f3..3cfebfff7d26 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> +obj-$(CONFIG_MCP3911) += mcp3911.o
>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>  obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>  obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> new file mode 100644
> index 000000000000..29aa39930ead
> --- /dev/null
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Microchip MCP3911, Two-channel Analog Front End
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
> + *
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#define MCP3911_REG_CHANNEL0		0x00
> +#define MCP3911_REG_CHANNEL1		0x03
> +#define MCP3911_REG_MOD			0x06
> +#define MCP3911_REG_PHASE		0x07
> +#define MCP3911_REG_GAIN		0x09
> +
> +#define MCP3911_REG_STATUSCOM		0x0a
> +#define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
> +#define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
> +#define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
> +#define MCP3911_STATUSCOM_EN_GAINCAL	BIT(1)
> +
> +#define MCP3911_REG_CONFIG		0x0c
> +#define MCP3911_CONFIG_CLKEXT		BIT(1)
> +#define MCP3911_CONFIG_VREFEXT		BIT(2)
> +
> +#define MCP3911_REG_OFFCAL_CH0		0x0e
> +#define MCP3911_REG_GAINCAL_CH0		0x11
> +#define MCP3911_REG_OFFCAL_CH1		0x14
> +#define MCP3911_REG_GAINCAL_CH1		0x17
> +#define MCP3911_REG_VREFCAL		0x1a
> +
> +#define MCP3911_CHANNEL(x)		(MCP3911_REG_CHANNEL0 + x * 3)
> +#define MCP3911_OFFCAL(x)		(MCP3911_REG_OFFCAL_CH0 + x * 6)
> +
> +/* Internal voltage reference in uV */
> +#define MCP3911_INT_VREF_UV		1200000
> +
> +#define MCP3911_REG_READ(reg, id)	((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
> +#define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
> +
> +#define MCP3911_NUM_CHANNELS		2
> +
> +struct mcp3911 {
> +	struct spi_device *spi;
> +	struct mutex lock;
> +	struct regulator *vref;
> +	struct clk *adc_clk;
> +	u32 dev_addr;
> +};
> +
> +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> +{
> +	int ret;
> +
> +	reg = MCP3911_REG_READ(reg, adc->dev_addr);
> +	ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	be32_to_cpus(val);
> +	*val >>= ((4 - len) * 8);
> +	dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val,
> +			reg>>1);
> +	return ret;
> +}
> +
> +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> +{
> +	dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg);
> +
> +	val <<= (3 - len) * 8;
> +	cpu_to_be32s(&val);
> +	val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> +
> +	return spi_write(adc->spi, &val, len+1);

Spaces around operators needed everywhere.  Please check the 
kernel coding style docs.

> +}
> +
> +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> +		u32 val, u8 len)
> +{
> +	u32 tmp;
> +	int ret;
> +
> +	ret = mcp3911_read(adc, reg, &tmp, len);
> +	if (ret)
> +		return ret;
> +
> +	val &= mask;
> +	val |= tmp & ~mask;
> +	return mcp3911_write(adc, reg, val, len);
> +}
> +
> +static int mcp3911_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct mcp3911 *adc = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = mcp3911_read(adc,
> +				MCP3911_CHANNEL(channel->channel), val, 3);
> +		if (ret)
> +			goto out;
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		ret = mcp3911_read(adc,
> +				MCP3911_OFFCAL(channel->channel), val, 3);
> +		if (ret)
> +			goto out;
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		if (adc->vref) {
> +			ret = regulator_get_voltage(adc->vref);
> +			if (ret < 0) {
> +				dev_err(indio_dev->dev.parent,
> +					"failed to get vref voltage: %d\n",
> +				       ret);
> +				goto out;
> +			}
> +
> +			*val = ret / 1000;
> +		} else {
> +			*val = MCP3911_INT_VREF_UV;
> +		}
> +
> +		*val2 = 24;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;
> +		break;
> +	}
> +
> +out:
> +	mutex_unlock(&adc->lock);
> +	return ret;
> +}
> +
> +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int val,
> +			    int val2, long mask)
> +{
> +	struct mcp3911 *adc = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (val2 != 0) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		/* Write offset */
> +		ret = mcp3911_write(adc, MCP3911_OFFCAL(channel->channel), val,
> +				3);
> +		if (ret)
> +			goto out;
> +
> +		/* Enable offset*/
> +		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM,
> +				MCP3911_STATUSCOM_EN_OFFCAL,
> +				MCP3911_STATUSCOM_EN_OFFCAL, 2);
> +		break;
> +	}
> +
> +out:
> +	mutex_unlock(&adc->lock);
> +	return ret;
> +}
> +
> +#define MCP3911_CHAN(idx) {					\
> +		.type = IIO_VOLTAGE,				\
> +		.indexed = 1,					\
> +		.channel = idx,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +			BIT(IIO_CHAN_INFO_OFFSET) |		\
> +			BIT(IIO_CHAN_INFO_SCALE),		\
> +}
> +
> +static const struct iio_chan_spec mcp3911_channels[] = {
> +	MCP3911_CHAN(0),
> +	MCP3911_CHAN(1),
> +};
> +
> +static const struct iio_info mcp3911_info = {
> +	.read_raw = mcp3911_read_raw,
> +	.write_raw = mcp3911_write_raw,
> +};
> +
> +static int mcp3911_config(struct mcp3911 *adc, struct device_node *of_node)
> +{
> +	u32 configreg;
> +	int ret;
> +
> +	of_property_read_u32(of_node, "device-addr", &adc->dev_addr);
> +	if (adc->dev_addr > 3) {
> +		dev_err(&adc->spi->dev,
> +				"invalid device address (%i). Must be in range 0-3.\n",
> +				adc->dev_addr);
> +		return -EINVAL;
> +	}
> +	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
> +
> +	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
> +	if (ret)
> +		return ret;
> +
> +	if (adc->vref) {
> +		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> +		configreg |= MCP3911_CONFIG_VREFEXT;
> +
The blank line here is inconsistent with other near by blocks (nitpick ;)
> +	} else {
> +		dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> +		configreg &= ~MCP3911_CONFIG_VREFEXT;
> +	}
> +
> +	if (adc->adc_clk) {
> +		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> +		configreg |= MCP3911_CONFIG_CLKEXT;
> +	} else {
> +		dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> +		configreg &= ~MCP3911_CONFIG_CLKEXT;
> +	}
> +
> +	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> +}
> +
> +static int mcp3911_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp3911 *adc;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
> +	if (IS_ERR(adc->vref)) {
> +
> +		if (PTR_ERR(adc->vref) == -ENODEV) {
> +			adc->vref = NULL;
> +		} else {
> +			dev_err(&adc->spi->dev,
> +					"failed to get regulator (%ld)\n",
> +					PTR_ERR(adc->vref));
> +			return PTR_ERR(adc->vref);
> +		}
> +
> +	} else {
> +		ret = regulator_enable(adc->vref);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	adc->adc_clk = devm_clk_get(&adc->spi->dev, "adc_clk");
> +	if (IS_ERR(adc->adc_clk)) {
> +		if (PTR_ERR(adc->adc_clk) == -ENOENT) {
> +			adc->adc_clk = NULL;
> +		} else {
> +			dev_err(&adc->spi->dev,
> +					"failed to get adc clk (%ld)\n",
> +					PTR_ERR(adc->adc_clk));
> +			ret = PTR_ERR(adc->adc_clk);
> +			goto reg_disable;
> +		}
> +	} else {
> +		ret = clk_prepare_enable(adc->adc_clk);
> +		if (ret < 0) {
> +			dev_err(&adc->spi->dev,
> +				"Failed to enable adc_clk: %d\n", ret);
> +			goto reg_disable;
> +		}
> +	}
> +
> +	ret = mcp3911_config(adc, spi->dev.of_node);
> +	if (ret)
> +		goto clk_disable;
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &mcp3911_info;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->channels = mcp3911_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> +
> +	mutex_init(&adc->lock);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto clk_disable;
> +
> +	return ret;
> +
> +clk_disable:
> +	if (adc->adc_clk)
> +		clk_disable_unprepare(adc->adc_clk);
> +reg_disable:
> +	if (adc->vref)
> +		regulator_disable(adc->vref);
> +
> +	return ret;
> +}
> +
> +static int mcp3911_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct mcp3911 *adc = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +

You need to also disable_unprepare the clock in remove as well.

> +	if (adc->vref)
> +		regulator_disable(adc->vref);
> +
> +	return 0;
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mcp3911_dt_ids[] = {
> +	{ .compatible = "microchip,mcp3911" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> +#endif
> +
> +static const struct spi_device_id mcp3911_id[] = {
> +	{ "mcp3911", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> +
> +static struct spi_driver mcp3911_driver = {
> +	.driver = {
> +		.name = "mcp3911",
> +		.of_match_table = of_match_ptr(mcp3911_dt_ids),
> +	},
> +	.probe = mcp3911_probe,
> +	.remove = mcp3911_remove,
> +	.id_table = mcp3911_id,
> +};
> +module_spi_driver(mcp3911_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_AUTHOR("Kent Gustavsson <kent@minoris.se>");
> +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/3] iio: adc: add support for mcp3911

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 14063 bytes --]

Hi Jonathan,

Sorry for the delay, I've been away from keyboard for a few days.

On Sun, Jul 29, 2018 at 12:44:22PM +0100, Jonathan Cameron wrote:
> On Tue, 24 Jul 2018 20:30:02 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > Signed-off-by: Kent Gustavsson <kent@minoris.se>
> Hi
> 
> I think you missed cleaning up the clock enable in the remove.
> If it is safe to not do so it should also be safe in the error path
> of probe.

I've actually noticed this and had it prepared in v4 :-)

> 
> A couple of trivial other things jumped out at me whilst reading.

Thank you so much, I will fix these.
> 
> Thanks,
> 
> Jonathan
> > ---
> > 
> > Notes:
> >     v2:
> >     	- cleanups and bugfixes (thanks Peter Meerwald-Stadler)
> >     	- drop hardware gain
> >     	- use the presence or lack of regulator to indicate if we go for internal or external voltage reference
> >     	- do not store device node in private struct
> >     	- drop support to set width in devicetree
> >     	- use the presence or lack of clock to indicate if we go for internal or external clock
> > 
> >  drivers/iio/adc/Kconfig   |  10 ++
> >  drivers/iio/adc/Makefile  |   1 +
> >  drivers/iio/adc/mcp3911.c | 366 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 377 insertions(+)
> >  create mode 100644 drivers/iio/adc/mcp3911.c
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 15606f237480..f9a41fa96fcc 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -501,6 +501,16 @@ config MCP3422
> >  	  This driver can also be built as a module. If so, the module will be
> >  	  called mcp3422.
> >  
> > +config MCP3911
> > +	tristate "Microchip Technology MCP3911 driver"
> > +	depends on SPI
> > +	help
> > +	  Say yes here to build support for Microchip Technology's MCP3911
> > +	  analog to digital converter.
> > +
> > +	  This driver can also be built as a module. If so, the module will be
> > +	  called mcp3911.
> > +
> >  config MEDIATEK_MT6577_AUXADC
> >          tristate "MediaTek AUXADC driver"
> >          depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 28a9423997f3..3cfebfff7d26 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> >  obj-$(CONFIG_MAX9611) += max9611.o
> >  obj-$(CONFIG_MCP320X) += mcp320x.o
> >  obj-$(CONFIG_MCP3422) += mcp3422.o
> > +obj-$(CONFIG_MCP3911) += mcp3911.o
> >  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> >  obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> >  obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > new file mode 100644
> > index 000000000000..29aa39930ead
> > --- /dev/null
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -0,0 +1,366 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Microchip MCP3911, Two-channel Analog Front End
> > + *
> > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define MCP3911_REG_CHANNEL0		0x00
> > +#define MCP3911_REG_CHANNEL1		0x03
> > +#define MCP3911_REG_MOD			0x06
> > +#define MCP3911_REG_PHASE		0x07
> > +#define MCP3911_REG_GAIN		0x09
> > +
> > +#define MCP3911_REG_STATUSCOM		0x0a
> > +#define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
> > +#define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
> > +#define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
> > +#define MCP3911_STATUSCOM_EN_GAINCAL	BIT(1)
> > +
> > +#define MCP3911_REG_CONFIG		0x0c
> > +#define MCP3911_CONFIG_CLKEXT		BIT(1)
> > +#define MCP3911_CONFIG_VREFEXT		BIT(2)
> > +
> > +#define MCP3911_REG_OFFCAL_CH0		0x0e
> > +#define MCP3911_REG_GAINCAL_CH0		0x11
> > +#define MCP3911_REG_OFFCAL_CH1		0x14
> > +#define MCP3911_REG_GAINCAL_CH1		0x17
> > +#define MCP3911_REG_VREFCAL		0x1a
> > +
> > +#define MCP3911_CHANNEL(x)		(MCP3911_REG_CHANNEL0 + x * 3)
> > +#define MCP3911_OFFCAL(x)		(MCP3911_REG_OFFCAL_CH0 + x * 6)
> > +
> > +/* Internal voltage reference in uV */
> > +#define MCP3911_INT_VREF_UV		1200000
> > +
> > +#define MCP3911_REG_READ(reg, id)	((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
> > +#define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
> > +
> > +#define MCP3911_NUM_CHANNELS		2
> > +
> > +struct mcp3911 {
> > +	struct spi_device *spi;
> > +	struct mutex lock;
> > +	struct regulator *vref;
> > +	struct clk *adc_clk;
> > +	u32 dev_addr;
> > +};
> > +
> > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > +{
> > +	int ret;
> > +
> > +	reg = MCP3911_REG_READ(reg, adc->dev_addr);
> > +	ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	be32_to_cpus(val);
> > +	*val >>= ((4 - len) * 8);
> > +	dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val,
> > +			reg>>1);
> > +	return ret;
> > +}
> > +
> > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > +{
> > +	dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg);
> > +
> > +	val <<= (3 - len) * 8;
> > +	cpu_to_be32s(&val);
> > +	val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> > +
> > +	return spi_write(adc->spi, &val, len+1);
> 
> Spaces around operators needed everywhere.  Please check the 
> kernel coding style docs.
> 
> > +}
> > +
> > +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> > +		u32 val, u8 len)
> > +{
> > +	u32 tmp;
> > +	int ret;
> > +
> > +	ret = mcp3911_read(adc, reg, &tmp, len);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= mask;
> > +	val |= tmp & ~mask;
> > +	return mcp3911_write(adc, reg, val, len);
> > +}
> > +
> > +static int mcp3911_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *channel, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	struct mcp3911 *adc = iio_priv(indio_dev);
> > +	int ret = -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = mcp3911_read(adc,
> > +				MCP3911_CHANNEL(channel->channel), val, 3);
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		ret = mcp3911_read(adc,
> > +				MCP3911_OFFCAL(channel->channel), val, 3);
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (adc->vref) {
> > +			ret = regulator_get_voltage(adc->vref);
> > +			if (ret < 0) {
> > +				dev_err(indio_dev->dev.parent,
> > +					"failed to get vref voltage: %d\n",
> > +				       ret);
> > +				goto out;
> > +			}
> > +
> > +			*val = ret / 1000;
> > +		} else {
> > +			*val = MCP3911_INT_VREF_UV;
> > +		}
> > +
> > +		*val2 = 24;
> > +		ret = IIO_VAL_FRACTIONAL_LOG2;
> > +		break;
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&adc->lock);
> > +	return ret;
> > +}
> > +
> > +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *channel, int val,
> > +			    int val2, long mask)
> > +{
> > +	struct mcp3911 *adc = iio_priv(indio_dev);
> > +	int ret = -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		if (val2 != 0) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		/* Write offset */
> > +		ret = mcp3911_write(adc, MCP3911_OFFCAL(channel->channel), val,
> > +				3);
> > +		if (ret)
> > +			goto out;
> > +
> > +		/* Enable offset*/
> > +		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM,
> > +				MCP3911_STATUSCOM_EN_OFFCAL,
> > +				MCP3911_STATUSCOM_EN_OFFCAL, 2);
> > +		break;
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&adc->lock);
> > +	return ret;
> > +}
> > +
> > +#define MCP3911_CHAN(idx) {					\
> > +		.type = IIO_VOLTAGE,				\
> > +		.indexed = 1,					\
> > +		.channel = idx,					\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> > +			BIT(IIO_CHAN_INFO_OFFSET) |		\
> > +			BIT(IIO_CHAN_INFO_SCALE),		\
> > +}
> > +
> > +static const struct iio_chan_spec mcp3911_channels[] = {
> > +	MCP3911_CHAN(0),
> > +	MCP3911_CHAN(1),
> > +};
> > +
> > +static const struct iio_info mcp3911_info = {
> > +	.read_raw = mcp3911_read_raw,
> > +	.write_raw = mcp3911_write_raw,
> > +};
> > +
> > +static int mcp3911_config(struct mcp3911 *adc, struct device_node *of_node)
> > +{
> > +	u32 configreg;
> > +	int ret;
> > +
> > +	of_property_read_u32(of_node, "device-addr", &adc->dev_addr);
> > +	if (adc->dev_addr > 3) {
> > +		dev_err(&adc->spi->dev,
> > +				"invalid device address (%i). Must be in range 0-3.\n",
> > +				adc->dev_addr);
> > +		return -EINVAL;
> > +	}
> > +	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
> > +
> > +	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (adc->vref) {
> > +		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> > +		configreg |= MCP3911_CONFIG_VREFEXT;
> > +
> The blank line here is inconsistent with other near by blocks (nitpick ;)
> > +	} else {
> > +		dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> > +		configreg &= ~MCP3911_CONFIG_VREFEXT;
> > +	}
> > +
> > +	if (adc->adc_clk) {
> > +		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> > +		configreg |= MCP3911_CONFIG_CLKEXT;
> > +	} else {
> > +		dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> > +		configreg &= ~MCP3911_CONFIG_CLKEXT;
> > +	}
> > +
> > +	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > +}
> > +
> > +static int mcp3911_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct mcp3911 *adc;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	adc = iio_priv(indio_dev);
> > +	adc->spi = spi;
> > +
> > +	adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
> > +	if (IS_ERR(adc->vref)) {
> > +
> > +		if (PTR_ERR(adc->vref) == -ENODEV) {
> > +			adc->vref = NULL;
> > +		} else {
> > +			dev_err(&adc->spi->dev,
> > +					"failed to get regulator (%ld)\n",
> > +					PTR_ERR(adc->vref));
> > +			return PTR_ERR(adc->vref);
> > +		}
> > +
> > +	} else {
> > +		ret = regulator_enable(adc->vref);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	adc->adc_clk = devm_clk_get(&adc->spi->dev, "adc_clk");
> > +	if (IS_ERR(adc->adc_clk)) {
> > +		if (PTR_ERR(adc->adc_clk) == -ENOENT) {
> > +			adc->adc_clk = NULL;
> > +		} else {
> > +			dev_err(&adc->spi->dev,
> > +					"failed to get adc clk (%ld)\n",
> > +					PTR_ERR(adc->adc_clk));
> > +			ret = PTR_ERR(adc->adc_clk);
> > +			goto reg_disable;
> > +		}
> > +	} else {
> > +		ret = clk_prepare_enable(adc->adc_clk);
> > +		if (ret < 0) {
> > +			dev_err(&adc->spi->dev,
> > +				"Failed to enable adc_clk: %d\n", ret);
> > +			goto reg_disable;
> > +		}
> > +	}
> > +
> > +	ret = mcp3911_config(adc, spi->dev.of_node);
> > +	if (ret)
> > +		goto clk_disable;
> > +
> > +	indio_dev->dev.parent = &spi->dev;
> > +	indio_dev->dev.of_node = spi->dev.of_node;
> > +	indio_dev->name = spi_get_device_id(spi)->name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &mcp3911_info;
> > +	spi_set_drvdata(spi, indio_dev);
> > +
> > +	indio_dev->channels = mcp3911_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> > +
> > +	mutex_init(&adc->lock);
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret)
> > +		goto clk_disable;
> > +
> > +	return ret;
> > +
> > +clk_disable:
> > +	if (adc->adc_clk)
> > +		clk_disable_unprepare(adc->adc_clk);
> > +reg_disable:
> > +	if (adc->vref)
> > +		regulator_disable(adc->vref);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mcp3911_remove(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > +	struct mcp3911 *adc = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> 
> You need to also disable_unprepare the clock in remove as well.
> 
> > +	if (adc->vref)
> > +		regulator_disable(adc->vref);
> > +
> > +	return 0;
> > +}
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mcp3911_dt_ids[] = {
> > +	{ .compatible = "microchip,mcp3911" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> > +#endif
> > +
> > +static const struct spi_device_id mcp3911_id[] = {
> > +	{ "mcp3911", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> > +
> > +static struct spi_driver mcp3911_driver = {
> > +	.driver = {
> > +		.name = "mcp3911",
> > +		.of_match_table = of_match_ptr(mcp3911_dt_ids),
> > +	},
> > +	.probe = mcp3911_probe,
> > +	.remove = mcp3911_remove,
> > +	.id_table = mcp3911_id,
> > +};
> > +module_spi_driver(mcp3911_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> > +MODULE_AUTHOR("Kent Gustavsson <kent@minoris.se>");
> > +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> > +MODULE_LICENSE("GPL v2");
> 


Best regards
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]