diff mbox series

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

Message ID 20180724183004.20279-1-marcus.folkesson@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/3] iio: adc: add support for mcp3911 | expand

Commit Message

Marcus Folkesson July 24, 2018, 6:30 p.m. UTC
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

Comments

Jonathan Cameron July 29, 2018, 11:44 a.m. UTC | #1
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");

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcus Folkesson Aug. 2, 2018, 6:59 p.m. UTC | #2
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
diff mbox series

Patch

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");