[PATCH v2 0/3] MCP3911 fixes

[PATCH v2 0/3] MCP3911 fixes

[Marcus Folkesson]

This series contain two fixes for the MCP3911 driver:
- Add support for reset signal
- Fix wrong mapping for the coversion result registers

The register map for the conversion result registers of the MCP3911
differs from the other variants so make sure we read from the right
register by introducing device-dependent .read_raw() callbacks.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
Changes in v2:
- Fix review comments from Andy Shevchenko (Thanks Andy!)
- Link to v1: https://lore.kernel.org/r/20250423-mcp3911-fixes-v1-0-5bd0b68ec481@gmail.com

---
Marcus Folkesson (3):
      iio: adc: mcp3911: fix device dependent mappings for conversion result registers
      dt-bindings: iio: adc: mcp3911: add reset-gpios
      iio: adc: mcp3911: add reset management

 .../bindings/iio/adc/microchip,mcp3911.yaml        |  5 ++
 drivers/iio/adc/mcp3911.c                          | 58 ++++++++++++++++++++--
 2 files changed, 58 insertions(+), 5 deletions(-)
---
base-commit: 1e26c5e28ca5821a824e90dd359556f5e9e7b89f
change-id: 20250423-mcp3911-fixes-ef3b2145577d

Best regards,
-- 
Marcus Folkesson <marcus.folkesson@gmail.com>

[PATCH v2 1/3] iio: adc: mcp3911: fix device dependent mappings for conversion result registers

[Marcus Folkesson]

The conversion result registers differs between devices. Make sure the
mapping is correct by using a device dependent .get_raw() callback function.

Fixes: 732ad34260d3 ("iio: adc: mcp3911: add support for the whole MCP39xx family")
Co-developed-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 6748b44d568db68120172a950bbfffb6adc7cfa3..60a19c35807ab783505e817898a2405bd8beae34 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -6,7 +6,7 @@
  * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
  */
 #include <linux/bitfield.h>
-#include <linux/bits.h>
+#include <linux/bitops.h>
 #include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -79,6 +79,8 @@
 #define MCP3910_CONFIG1_CLKEXT		BIT(6)
 #define MCP3910_CONFIG1_VREFEXT		BIT(7)
 
+#define MCP3910_CHANNEL(ch)		(MCP3911_REG_CHANNEL0 + (ch))
+
 #define MCP3910_REG_OFFCAL_CH0		0x0f
 #define MCP3910_OFFCAL(ch)		(MCP3910_REG_OFFCAL_CH0 + (ch) * 6)
 
@@ -110,6 +112,7 @@ struct mcp3911_chip_info {
 	int (*get_offset)(struct mcp3911 *adc, int channel, int *val);
 	int (*set_offset)(struct mcp3911 *adc, int channel, int val);
 	int (*set_scale)(struct mcp3911 *adc, int channel, u32 val);
+	int (*get_raw)(struct mcp3911 *adc, int channel, int *val);
 };
 
 struct mcp3911 {
@@ -170,6 +173,18 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, u32 val, u8 len
 	return mcp3911_write(adc, reg, val, len);
 }
 
+static int mcp3911_read_s24(struct mcp3911 *const adc, u8 const reg, s32 *const val)
+{
+	u32 uval;
+	int const ret = mcp3911_read(adc, reg, &uval, 3);
+
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(uval, 23);
+	return ret;
+}
+
 static int mcp3910_enable_offset(struct mcp3911 *adc, bool enable)
 {
 	unsigned int mask = MCP3910_CONFIG0_EN_OFFCAL;
@@ -194,6 +209,11 @@ static int mcp3910_set_offset(struct mcp3911 *adc, int channel, int val)
 	return adc->chip->enable_offset(adc, 1);
 }
 
+static int mcp3910_get_raw(struct mcp3911 *adc, int channel, s32 *val)
+{
+	return mcp3911_read_s24(adc, MCP3910_CHANNEL(channel), val);
+}
+
 static int mcp3911_enable_offset(struct mcp3911 *adc, bool enable)
 {
 	unsigned int mask = MCP3911_STATUSCOM_EN_OFFCAL;
@@ -218,6 +238,11 @@ static int mcp3911_set_offset(struct mcp3911 *adc, int channel, int val)
 	return adc->chip->enable_offset(adc, 1);
 }
 
+static int mcp3911_get_raw(struct mcp3911 *adc, int channel, s32 *val)
+{
+	return mcp3911_read_s24(adc, MCP3911_CHANNEL(channel), val);
+}
+
 static int mcp3910_get_osr(struct mcp3911 *adc, u32 *val)
 {
 	int ret;
@@ -321,12 +346,9 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 	guard(mutex)(&adc->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = mcp3911_read(adc,
-				   MCP3911_CHANNEL(channel->channel), val, 3);
+		ret = adc->chip->get_raw(adc, channel->channel, val);
 		if (ret)
 			return ret;
-
-		*val = sign_extend32(*val, 23);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
 		ret = adc->chip->get_offset(adc, channel->channel, val);
@@ -799,6 +821,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 	[MCP3911] = {
 		.channels = mcp3911_channels,
@@ -810,6 +833,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3911_get_offset,
 		.set_offset = mcp3911_set_offset,
 		.set_scale = mcp3911_set_scale,
+		.get_raw = mcp3911_get_raw,
 	},
 	[MCP3912] = {
 		.channels = mcp3912_channels,
@@ -821,6 +845,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 	[MCP3913] = {
 		.channels = mcp3913_channels,
@@ -832,6 +857,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 	[MCP3914] = {
 		.channels = mcp3914_channels,
@@ -843,6 +869,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 	[MCP3918] = {
 		.channels = mcp3918_channels,
@@ -854,6 +881,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 	[MCP3919] = {
 		.channels = mcp3919_channels,
@@ -865,6 +893,7 @@ static const struct mcp3911_chip_info mcp3911_chip_info[] = {
 		.get_offset = mcp3910_get_offset,
 		.set_offset = mcp3910_set_offset,
 		.set_scale = mcp3910_set_scale,
+		.get_raw = mcp3910_get_raw,
 	},
 };
 static const struct of_device_id mcp3911_dt_ids[] = {

-- 
2.49.0

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

[Marcus Folkesson]

The MCP391X family provides an active low reset signal that is still not
described in the bindings.

Add reset-gpios to the bindings and the example.

Co-developed-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index 06951ec5f5da381a9bb942d0ac7416128eebd3bc..3a69ec60edb915ae16312b94fddd32f5c87f37a7 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -32,6 +32,9 @@ properties:
   spi-max-frequency:
     maximum: 20000000
 
+  reset-gpios:
+    maxItems: 1
+
   clocks:
     description: |
       Phandle and clock identifier for external sampling clock.
@@ -71,6 +74,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
       #address-cells = <1>;
       #size-cells = <0>;
@@ -80,6 +84,7 @@ examples:
         reg = <0>;
         interrupt-parent = <&gpio5>;
         interrupts = <15 2>;
+        reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
         spi-max-frequency = <20000000>;
         microchip,device-addr = <0>;
         vref-supply = <&vref_reg>;

-- 
2.49.0

[PATCH v2 3/3] iio: adc: mcp3911: add reset management

[Marcus Folkesson]

Add support for optional HW reset.
If specified, a reset will be asserted during driver probe.

Co-developed-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Lukas Rauber <lukas.rauber@janitza.de>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 60a19c35807ab783505e817898a2405bd8beae34..76b13a73543493c4c5d446dc47f8bf1f9d02af39 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -10,7 +10,9 @@
 #include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dev_printk.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
@@ -706,6 +708,7 @@ static const struct iio_trigger_ops mcp3911_trigger_ops = {
 static int mcp3911_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct gpio_desc *gpio_reset;
 	struct iio_dev *indio_dev;
 	struct mcp3911 *adc;
 	bool external_vref;
@@ -750,6 +753,22 @@ static int mcp3911_probe(struct spi_device *spi)
 	}
 	dev_dbg(dev, "use device address %i\n", adc->dev_addr);
 
+	gpio_reset = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio_reset))
+		return dev_err_probe(dev, PTR_ERR(gpio_reset),
+				     "Cannot get reset GPIO\n");
+
+	if (gpio_reset) {
+		gpiod_set_value_cansleep(gpio_reset, 0);
+
+		/*
+		 * Settling time after Hard Reset Mode (determined experimentally):
+		 * 330 micro-seconds are too few; 470 micro-seconds are sufficient.
+		 * Just in case, we add some safety factor...
+		 */
+		fsleep(600);
+	}
+
 	ret = adc->chip->config(adc, external_vref);
 	if (ret)
 		return ret;

-- 
2.49.0

Re: [PATCH v2 1/3] iio: adc: mcp3911: fix device dependent mappings for conversion result registers

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 08:54:11AM +0200, Marcus Folkesson wrote:
> The conversion result registers differs between devices. Make sure the
> mapping is correct by using a device dependent .get_raw() callback function.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] iio: adc: mcp3911: add reset management

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 08:54:13AM +0200, Marcus Folkesson wrote:
> Add support for optional HW reset.
> If specified, a reset will be asserted during driver probe.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Conor Dooley]

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

On Mon, Apr 28, 2025 at 08:54:12AM +0200, Marcus Folkesson wrote:
> The MCP391X family provides an active low reset signal that is still not
> described in the bindings.
> 
> Add reset-gpios to the bindings and the example.
> 
> Co-developed-by: Lukas Rauber <lukas.rauber@janitza.de>
> Signed-off-by: Lukas Rauber <lukas.rauber@janitza.de>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> index 06951ec5f5da381a9bb942d0ac7416128eebd3bc..3a69ec60edb915ae16312b94fddd32f5c87f37a7 100644
> --- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> @@ -32,6 +32,9 @@ properties:
>    spi-max-frequency:
>      maximum: 20000000
>  
> +  reset-gpios:
> +    maxItems: 1
> +
>    clocks:
>      description: |
>        Phandle and clock identifier for external sampling clock.
> @@ -71,6 +74,7 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
>      spi {
>        #address-cells = <1>;
>        #size-cells = <0>;
> @@ -80,6 +84,7 @@ examples:
>          reg = <0>;
>          interrupt-parent = <&gpio5>;
>          interrupts = <15 2>;
> +        reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
>          spi-max-frequency = <20000000>;
>          microchip,device-addr = <0>;
>          vref-supply = <&vref_reg>;
> 
> -- 
> 2.49.0
> 

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

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

[Jonathan Cameron]

On Mon, 28 Apr 2025 17:40:38 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Mon, Apr 28, 2025 at 08:54:12AM +0200, Marcus Folkesson wrote:
> > The MCP391X family provides an active low reset signal that is still not
> > described in the bindings.
> > 
> > Add reset-gpios to the bindings and the example.
> > 
> > Co-developed-by: Lukas Rauber <lukas.rauber@janitza.de>
> > Signed-off-by: Lukas Rauber <lukas.rauber@janitza.de>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>  
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Just for reference this also had an ack from Krzysztof on v1 which
raced with this version being posted.

b4 found both and confused me :)

Jonathan

> 
> > ---
> >  Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> > index 06951ec5f5da381a9bb942d0ac7416128eebd3bc..3a69ec60edb915ae16312b94fddd32f5c87f37a7 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> > @@ -32,6 +32,9 @@ properties:
> >    spi-max-frequency:
> >      maximum: 20000000
> >  
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> >    clocks:
> >      description: |
> >        Phandle and clock identifier for external sampling clock.
> > @@ -71,6 +74,7 @@ unevaluatedProperties: false
> >  
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> >      spi {
> >        #address-cells = <1>;
> >        #size-cells = <0>;
> > @@ -80,6 +84,7 @@ examples:
> >          reg = <0>;
> >          interrupt-parent = <&gpio5>;
> >          interrupts = <15 2>;
> > +        reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> >          spi-max-frequency = <20000000>;
> >          microchip,device-addr = <0>;
> >          vref-supply = <&vref_reg>;
> > 
> > -- 
> > 2.49.0
> >   

Re: [PATCH v2 0/3] MCP3911 fixes

[Jonathan Cameron]

On Mon, 28 Apr 2025 08:54:10 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> This series contain two fixes for the MCP3911 driver:
> - Add support for reset signal
> - Fix wrong mapping for the coversion result registers
> 
> The register map for the conversion result registers of the MCP3911
> differs from the other variants so make sure we read from the right
> register by introducing device-dependent .read_raw() callbacks.
Applied to the togreg branch of iio.git and initially pushed out as testing
for 0-day to take a look.

Thanks,

Jonathan

> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
> Changes in v2:
> - Fix review comments from Andy Shevchenko (Thanks Andy!)
> - Link to v1: https://lore.kernel.org/r/20250423-mcp3911-fixes-v1-0-5bd0b68ec481@gmail.com
> 
> ---
> Marcus Folkesson (3):
>       iio: adc: mcp3911: fix device dependent mappings for conversion result registers
>       dt-bindings: iio: adc: mcp3911: add reset-gpios
>       iio: adc: mcp3911: add reset management
> 
>  .../bindings/iio/adc/microchip,mcp3911.yaml        |  5 ++
>  drivers/iio/adc/mcp3911.c                          | 58 ++++++++++++++++++++--
>  2 files changed, 58 insertions(+), 5 deletions(-)
> ---
> base-commit: 1e26c5e28ca5821a824e90dd359556f5e9e7b89f
> change-id: 20250423-mcp3911-fixes-ef3b2145577d
> 
> Best regards,