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

Commit Message

Marcus Folkesson Aug. 2, 2018, 7:15 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 <[e-mail]>
Signed-off-by: Kent Gustavsson <[e-mail]>
---

Notes:
    v3:
    	- rename adc_clk to clki
    	- add error handling/cleanup for clock
    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 | 367 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/iio/adc/mcp3911.c

Comments

Andy Shevchenko Aug. 2, 2018, 7:52 p.m. UTC | #1

On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
<[e-mail]> 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 <[e-mail]>

> Signed-off-by: Kent Gustavsson <[e-mail]>

What is this? Why it's here (presense and location in the message)?

> + * Copyright (C) 2018 Kent Gustavsson <[e-mail]>

> + *

Redundant.

> +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);
> +}

Can't you use REGMAP_SPI ?

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

Isn't what we called CS (chip select)?

> +       if (IS_ERR(adc->vref)) {

> +

Redundant.

> +       if (adc->clki)

Seems to be redundant if clock is optional (it should be NULL and API
is NULL-aware).

> +               clk_disable_unprepare(adc->clki);

> +       if (adc->clki)
> +               clk_disable_unprepare(adc->clki);

Ditto.

> +#if defined(CONFIG_OF)

This prevents playing with the device on ACPI enabled platforms.

> +               .of_match_table = of_match_ptr(mcp3911_dt_ids),

Ditto for macro.

Jonathan Cameron Aug. 3, 2018, 10:09 p.m. UTC | #2

On Thu, 2 Aug 2018 22:52:00 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> <[e-mail]> 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 <[e-mail]>  
> 
> > Signed-off-by: Kent Gustavsson <[e-mail]>  
> 
> What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting
as gatekeeper / upstreamer you should set the mail to be from Kent
and put your own Signed-off after his to basically act as a submaintainer
certifying you believe his sign off and all that entails.

If it is a bit of a joint effort then that's fine but for copyright
purposes there should be some indication of the split.


> 
> > + * Copyright (C) 2018 Kent Gustavsson <[e-mail]>  
> 
> > + *  
> 
> Redundant.
> 
> > +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);
> > +}  
> 
> Can't you use REGMAP_SPI ?
My instinct is not really. The magic device-addr doesn't help.
You could work around it but it's not that nice and you'd have
to create the regmap mapping on the fly or carry static ones
for each value of htat.



> 
> > +       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);  
> 
> Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted
in the control byte to allow you to 'share' a chip select line between multiple
chips (crazy but true).

> 
> > +       if (IS_ERR(adc->vref)) {  
> 
> > +  
> 
> Redundant.
> 
> > +       if (adc->clki)  
> 
> Seems to be redundant if clock is optional (it should be NULL and API
> is NULL-aware).
> 
> > +               clk_disable_unprepare(adc->clki);  
> 
> > +       if (adc->clki)
> > +               clk_disable_unprepare(adc->clki);  
> 
> Ditto.
> 
> > +#if defined(CONFIG_OF)  
> 
> This prevents playing with the device on ACPI enabled platforms.
Yeah, that trickery is still little known (and I forget about it from
time to time).

The upshot for those who have missed this is you can use a magic
acpi device to instantiate based on device tree bindings :)

So we need to strip all this 'obvious' protection stuff out of drivers.

> 
> > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),  
> 
> Ditto for macro.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marcus Folkesson Aug. 4, 2018, 6:55 a.m. UTC | #3

Hi Andy and Jonathan,


On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
> On Thu, 2 Aug 2018 22:52:00 +0300
> Andy Shevchenko <[e-mail]> wrote:
> 
> > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> > <[e-mail]> 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 <[e-mail]>  
> > 
> > > Signed-off-by: Kent Gustavsson <[e-mail]>  
> > 
> > What is this? Why it's here (presense and location in the message)?
> To clarify... If Kent wrote the patch and you are simply acting
> as gatekeeper / upstreamer you should set the mail to be from Kent
> and put your own Signed-off after his to basically act as a submaintainer
> certifying you believe his sign off and all that entails.
> 
> If it is a bit of a joint effort then that's fine but for copyright
> purposes there should be some indication of the split.

First, thank you Andy for noticing.

I actually intended to use Co-Developed-by (a pretty new tag)
in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..

From Documentation/process/submitting-patches.rst ::

	A Co-Developed-by: states that the patch was also created by another developer
	along with the original author.  This is useful at times when multiple people
	work on a single patch.  Note, this person also needs to have a Signed-off-by:
	line in the patch as well.

I will switch order and add the Co-Developed-by-tag.
Is this correct?

Co-Developed-by: Kent Gustavsson <[e-mail]>   
Signed-off-by: Kent Gustavsson <[e-mail]>  
Signed-off-by: Marcus Folkesson <[e-mail]>  

> 
> 
> > 
> > > + * Copyright (C) 2018 Kent Gustavsson <[e-mail]>  
> > 
> > > + *  
> > 
> > Redundant.
> > 
> > > +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);
> > > +}  
> > 
> > Can't you use REGMAP_SPI ?
> My instinct is not really. The magic device-addr doesn't help.
> You could work around it but it's not that nice and you'd have
> to create the regmap mapping on the fly or carry static ones
> for each value of htat.
> 
> 
> 
> > 
> > > +       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);  
> > 
> > Isn't what we called CS (chip select)?
> Nope. We went around this in an earlier revision. It's an address transmitted
> in the control byte to allow you to 'share' a chip select line between multiple
> chips (crazy but true).
> 
> > 
> > > +       if (IS_ERR(adc->vref)) {  
> > 
> > > +  
> > 
> > Redundant.
> > 
> > > +       if (adc->clki)  
> > 
> > Seems to be redundant if clock is optional (it should be NULL and API
> > is NULL-aware).
> > 
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > > +       if (adc->clki)
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > Ditto.
> > 
> > > +#if defined(CONFIG_OF)  
> > 
> > This prevents playing with the device on ACPI enabled platforms.

I will remove the defined(CONFIG_OF) and not use the of_match_ptr()
macro. 

> Yeah, that trickery is still little known (and I forget about it from
> time to time).
> 
> The upshot for those who have missed this is you can use a magic
> acpi device to instantiate based on device tree bindings :)
> 
> So we need to strip all this 'obvious' protection stuff out of drivers.

Jonathan,
Do you want me to do the same changes for drivers in iio/ ?
I'm probably not the only one looking at other drivers when writing my
own, so I guess this is a rather frequent issue for new drivers?


> 
> > 
> > > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),  
> > 
> > Ditto for macro.
> > 
> 

Best regards,
Marcus Folkesson

Andy Shevchenko Aug. 4, 2018, 8:56 p.m. UTC | #4

On Sat, Aug 4, 2018 at 9:55 AM, Marcus Folkesson
<[e-mail]> wrote:
> On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
>> On Thu, 2 Aug 2018 22:52:00 +0300
>> Andy Shevchenko <[e-mail]> wrote:
>>
>> > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
>> > <[e-mail]> 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 <[e-mail]>
>> >
>> > > Signed-off-by: Kent Gustavsson <[e-mail]>
>> >
>> > What is this? Why it's here (presense and location in the message)?
>> To clarify... If Kent wrote the patch and you are simply acting
>> as gatekeeper / upstreamer you should set the mail to be from Kent
>> and put your own Signed-off after his to basically act as a submaintainer
>> certifying you believe his sign off and all that entails.

Yep. And here the ordering is incorrect, right?

>> If it is a bit of a joint effort then that's fine but for copyright
>> purposes there should be some indication of the split.
>
> First, thank you Andy for noticing.
>
> I actually intended to use Co-Developed-by (a pretty new tag)
> in combination with Signed-off-by.
> But the tag must have disappeared in some preparation stage..
>
> From Documentation/process/submitting-patches.rst ::
>
>         A Co-Developed-by: states that the patch was also created by another developer
>         along with the original author.  This is useful at times when multiple people
>         work on a single patch.  Note, this person also needs to have a Signed-off-by:
>         line in the patch as well.
>
> I will switch order and add the Co-Developed-by-tag.
> Is this correct?
>
> Co-Developed-by: Kent Gustavsson <[e-mail]>
> Signed-off-by: Kent Gustavsson <[e-mail]>
> Signed-off-by: Marcus Folkesson <[e-mail]>

Yep, this looks good!

>> > > +       of_property_read_u32(of_node, "device-addr", &adc->dev_addr);

>> > Isn't what we called CS (chip select)?
>> Nope. We went around this in an earlier revision. It's an address transmitted
>> in the control byte to allow you to 'share' a chip select line between multiple
>> chips (crazy but true).

OK.

Jonathan Cameron Aug. 18, 2018, 4:56 p.m. UTC | #5

On Sat, 4 Aug 2018 08:55:06 +0200
Marcus Folkesson <[e-mail]> wrote:

> Hi Andy and Jonathan,
> 
> 
> On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
> > On Thu, 2 Aug 2018 22:52:00 +0300
> > Andy Shevchenko <[e-mail]> wrote:
> >   
> > > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> > > <[e-mail]> 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 <[e-mail]>    
> > >   
> > > > Signed-off-by: Kent Gustavsson <[e-mail]>    
> > > 
> > > What is this? Why it's here (presense and location in the message)?  
> > To clarify... If Kent wrote the patch and you are simply acting
> > as gatekeeper / upstreamer you should set the mail to be from Kent
> > and put your own Signed-off after his to basically act as a submaintainer
> > certifying you believe his sign off and all that entails.
> > 
> > If it is a bit of a joint effort then that's fine but for copyright
> > purposes there should be some indication of the split.  
> 
> First, thank you Andy for noticing.
> 
> I actually intended to use Co-Developed-by (a pretty new tag)
> in combination with Signed-off-by.
> But the tag must have disappeared in some preparation stage..
> 
> From Documentation/process/submitting-patches.rst ::
> 
> 	A Co-Developed-by: states that the patch was also created by another developer
> 	along with the original author.  This is useful at times when multiple people
> 	work on a single patch.  Note, this person also needs to have a Signed-off-by:
> 	line in the patch as well.
> 
> I will switch order and add the Co-Developed-by-tag.
> Is this correct?
> 
> Co-Developed-by: Kent Gustavsson <[e-mail]>   
> Signed-off-by: Kent Gustavsson <[e-mail]>  
> Signed-off-by: Marcus Folkesson <[e-mail]>  
> 
> > 
> >   
> > >   
> > > > + * Copyright (C) 2018 Kent Gustavsson <[e-mail]>    
> > >   
> > > > + *    
> > > 
> > > Redundant.
> > >   
> > > > +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);
> > > > +}    
> > > 
> > > Can't you use REGMAP_SPI ?  
> > My instinct is not really. The magic device-addr doesn't help.
> > You could work around it but it's not that nice and you'd have
> > to create the regmap mapping on the fly or carry static ones
> > for each value of htat.
> > 
> > 
> >   
> > >   
> > > > +       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);    
> > > 
> > > Isn't what we called CS (chip select)?  
> > Nope. We went around this in an earlier revision. It's an address transmitted
> > in the control byte to allow you to 'share' a chip select line between multiple
> > chips (crazy but true).
> >   
> > >   
> > > > +       if (IS_ERR(adc->vref)) {    
> > >   
> > > > +    
> > > 
> > > Redundant.
> > >   
> > > > +       if (adc->clki)    
> > > 
> > > Seems to be redundant if clock is optional (it should be NULL and API
> > > is NULL-aware).
> > >   
> > > > +               clk_disable_unprepare(adc->clki);    
> > >   
> > > > +       if (adc->clki)
> > > > +               clk_disable_unprepare(adc->clki);    
> > > 
> > > Ditto.
> > >   
> > > > +#if defined(CONFIG_OF)    
> > > 
> > > This prevents playing with the device on ACPI enabled platforms.  
> 
> I will remove the defined(CONFIG_OF) and not use the of_match_ptr()
> macro. 
> 
> > Yeah, that trickery is still little known (and I forget about it from
> > time to time).
> > 
> > The upshot for those who have missed this is you can use a magic
> > acpi device to instantiate based on device tree bindings :)
> > 
> > So we need to strip all this 'obvious' protection stuff out of drivers.  
> 
> Jonathan,
> Do you want me to do the same changes for drivers in iio/ ?
> I'm probably not the only one looking at other drivers when writing my
> own, so I guess this is a rather frequent issue for new drivers?

A good question.   Hmm. I'm a bit in two minds.  It creates a lot
of churn for something that I don't think is actually very heavily
used.  As you say, the biggest reason to change it will be to stop
others from copying the existing drivers and getting it 'wrong' going
forward.

I don't have strong feelings either way, but don't really want to
see a huge number of patches around this (I haven't checked how
many drivers are effected).  If you want to look at this, then perhaps
do a small set at a time rather than a mass fixup to keep things
manageable?

Thanks,

Jonathan

> 
> 
> >   
> > >   
> > > > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),    
> > > 
> > > Ditto for macro.
> > >   
> >   
> 
> Best regards,
> Marcus Folkesson

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..fca8e6a304ec
--- /dev/null
+++ b/drivers/iio/adc/mcp3911.c
@@ -0,0 +1,367 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Microchip MCP3911, Two-channel Analog Front End
+ *
+ * Copyright (C) 2018 Marcus Folkesson <[e-mail]>
+ * Copyright (C) 2018 Kent Gustavsson <[e-mail]>
+ *
+ */
+#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 *clki;
+	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->clki) {
+		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->clki = devm_clk_get(&adc->spi->dev, NULL);
+	if (IS_ERR(adc->clki)) {
+		if (PTR_ERR(adc->clki) == -ENOENT) {
+			adc->clki = NULL;
+		} else {
+			dev_err(&adc->spi->dev,
+					"failed to get adc clk (%ld)\n",
+					PTR_ERR(adc->clki));
+			ret = PTR_ERR(adc->clki);
+			goto reg_disable;
+		}
+	} else {
+		ret = clk_prepare_enable(adc->clki);
+		if (ret < 0) {
+			dev_err(&adc->spi->dev,
+				"Failed to enable clki: %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->clki)
+		clk_disable_unprepare(adc->clki);
+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);
+	if (adc->clki)
+		clk_disable_unprepare(adc->clki);
+
+	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 <[e-mail]>");
+MODULE_AUTHOR("Kent Gustavsson <[e-mail]>");
+MODULE_DESCRIPTION("Microchip Technology MCP3911");
+MODULE_LICENSE("GPL v2");