iio: adc: add support for mcp3911
Subsystems (3)
Marcus Folkesson
maintainerPATCH v2 1/3 iio: adc: add support for mcp3911
MCP3911 is a dual channel Analog Front End (AFE) containing two synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
---
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
drivers/iio/adc/Kconfig +10 -0
@@ -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
drivers/iio/adc/Makefile +1 -0
@@ -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
drivers/iio/adc/mcp3911.c new file +366 -0
366 lines not on this page · raw message
Signature · 1 line
2.11.0.rc2
Marcus Folkesson
maintainerPATCH v2 2/3 dt-bindings: iio: adc: add bindings for mcp3911
MCP3911 is a dual channel Analog Front End (AFE) containing two synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
---
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
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +28 -0
@@ -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"; +};
Signature · 1 line
2.11.0.rc2
Rob Herring
maintainerRe: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Quoted 27 lines · 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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>---
Notes:
v2:
- drop channel width
- dropexternal_vref
- replaceexternal-clockwith proper clock bindings.../devicetree/bindings/iio/adc/mcp3911.txt | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txtDocumentation/devicetree/bindings/iio/adc/mcp3911.txt new file +4
-0@@ -0,0 +1,28 @@ +* Microchip MCP3911 Dual channel analog front end (ADC) + +Required properties: + - compatible: Should be "microchip,mcp3911"
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 14
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +10
-0+ - 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?
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 17
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +3
-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
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.
Marcus Folkesson
maintainerRe: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Hello Rob,
Thank you for the review.
Quoted 32 lines · Rob Herring
Quoted 31 lines · 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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>---
Notes:
v2:
- drop channel width
- dropexternal_vref
- replaceexternal-clockwith proper clock bindings.../devicetree/bindings/iio/adc/mcp3911.txt | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txtDocumentation/devicetree/bindings/iio/adc/mcp3911.txt new file +8
-0@@ -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
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 14
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +6
-0+ 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..
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 17
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +3
-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) clockDatasheet 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!
Quoted 2 lines
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
Rob Herring
maintainerRe: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Quoted 48 lines · Marcus Folkesson
Hello Rob,
Thank you for the review.
Quoted 32 lines · Rob Herring
Quoted 31 lines · 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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>---
Notes:
v2:
- drop channel width
- dropexternal_vref
- replaceexternal-clockwith proper clock bindings.../devicetree/bindings/iio/adc/mcp3911.txt | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txtDocumentation/devicetree/bindings/iio/adc/mcp3911.txt new file +8
-0@@ -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 perQuoted 9 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 14
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +6
-0+ 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?
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 17
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +3
-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) clockDatasheet calls this clki (or mclk internally). Or just drop clock-names as it is pointless
when there is only 1 clock.
Quoted 7 lines
I will drop the clock-names, thanks!
Quoted 2 lines
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
Marcus Folkesson
maintainerPATCH v2 3/3 MAINTAINERS: Add entry for mcp3911 ADC driver
In reply to Marcus FolkessonAdd an entry for mcp3911 ADC driver and add myself and Kent Gustavsson as maintainers of this driver.
---
Notes:
v2:
- no changes
MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+)
MAINTAINERS +8 -0
@@ -9271,6 +9271,14 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/microchip/lan743x_* +MICROCHIP / ATMEL MCP3911 ADC DRIVER +M: Marcus Folkesson <[email protected]> +M: Kent Gustavsson <[email protected]> +L: [email protected] +S: Supported +F: drivers/iio/adc/mcp3911.c +F: Documentation/devicetree/bindings/iio/adc/mcp3911.txt + MICROCHIP USB251XB DRIVER M: Richard Leitner <[email protected]> L: [email protected]
Signature · 1 line
2.11.0.rc2
Jonathan Cameron
maintainerQuoted 5 lines · On Tue, 24 Jul 2018 20:30:02 +0200 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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>
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
Quoted 128 lines
---
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 clockdrivers/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.cdrivers/iio/adc/Kconfig +10
-0@@ -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_TESTdrivers/iio/adc/Makefile +1
-0@@ -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.odrivers/iio/adc/mcp3911.c new file +72
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for Microchip MCP3911, Two-channel Analog Front End +* +* Copyright (C) 2018 Marcus Folkesson <[email protected]> +* Copyright (C) 2018 Kent Gustavsson <[email protected]> +* +*/ +#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, ®,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; +}
drivers/iio/adc/mcp3911.c line 82
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+ +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.
Quoted 133 lines
drivers/iio/adc/mcp3911.c new file +133
-0+} + +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 ; + }
drivers/iio/adc/mcp3911.c line 225
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+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 ;)
Quoted 96 lines
drivers/iio/adc/mcp3911.c new file +96
-0+ }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); +
drivers/iio/adc/mcp3911.c line 331
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+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.
Quoted 35 lines
drivers/iio/adc/mcp3911.c new file +35
-0+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 <[email protected]>" ); +MODULE_AUTHOR ("Kent Gustavsson <[email protected]>" ); +MODULE_DESCRIPTION ("Microchip Technology MCP3911" ); +MODULE_LICENSE ("GPL v2" );
Marcus Folkesson
maintainerHi Jonathan,
Sorry for the delay, I've been away from keyboard for a few days.
Quoted 6 lines · Jonathan Cameron
Quoted 3 lines · On Tue, 24 Jul 2018 20:30:02 +0200 Marcus Folkesson…
MCP3911 is a dual channel Analog Front End (AFE) containing two synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
Quoted 7 lines
Quoted 2 lines
Signed-off-by Marcus Folkesson <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>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 :-)
Quoted 2 lines
A couple of trivial other things jumped out at me whilst reading.
Thank you so much, I will fix these.
Quoted 426 lines
Thanks,
Jonathan
Quoted 128 lines
---
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 clockdrivers/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.cdrivers/iio/adc/Kconfig +10
-0@@ -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_TESTdrivers/iio/adc/Makefile +1
-0@@ -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.odrivers/iio/adc/mcp3911.c new file +72
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for Microchip MCP3911, Two-channel Analog Front End +* +* Copyright (C) 2018 Marcus Folkesson <[email protected]> +* Copyright (C) 2018 Kent Gustavsson <[email protected]> +* +*/ +#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, ®,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; +}drivers/iio/adc/mcp3911.c line 82
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+ +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.
Quoted 133 lines
drivers/iio/adc/mcp3911.c new file +133
-0+} + +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 ; + }drivers/iio/adc/mcp3911.c line 225
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+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 ;)
Quoted 96 lines
drivers/iio/adc/mcp3911.c new file +96
-0+ }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); +drivers/iio/adc/mcp3911.c line 331
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+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_unpreparethe clock in remove as well.Quoted 27 lines
drivers/iio/adc/mcp3911.c new file +27
-0+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,
Quoted 9 lines
drivers/iio/adc/mcp3911.c line 366
Quoted 8 lines
drivers/iio/adc/mcp3911.c new file +8
-0+ .id_table = mcp3911_id, +}; +module_spi_driver (mcp3911_driver); + +MODULE_AUTHOR ("Marcus Folkesson <[email protected]>" ); +MODULE_AUTHOR ("Kent Gustavsson <[email protected]>" ); +MODULE_DESCRIPTION ("Microchip Technology MCP3911" ); +MODULE_LICENSE ("GPL v2" );
Best regards
Marcus Folkesson