[PATCH 0/3] MCP3911 fixes

[PATCH 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>
---
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                          | 54 ++++++++++++++++++++--
 2 files changed, 55 insertions(+), 4 deletions(-)
---
base-commit: 1e26c5e28ca5821a824e90dd359556f5e9e7b89f
change-id: 20250423-mcp3911-fixes-ef3b2145577d

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

[PATCH 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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 6748b44d568db68120172a950bbfffb6adc7cfa3..b72ed4928da88664a00dc143ebf218cb4a7be421 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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

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

[Andy Shevchenko]

On Wed, Apr 23, 2025 at 04:46:49PM +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.
> 
> Fixes: 732ad34260d3 ("iio: adc: mcp3911: add support for the whole MCP39xx family")

> 

Shouldn't be blank line(s) in the tag block.

> 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>
> ---

...

> +	*val = sign_extend32(uval, 23);

With that, replace bits.h by bitops.h at the top of the file.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 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

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

[Krzysztof Kozlowski]

On Wed, Apr 23, 2025 at 04:46:50PM GMT, 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>
> ---
>  Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index b72ed4928da88664a00dc143ebf218cb4a7be421..4c04ce1b3982d84286355ee427a92aa55ff2fc51 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -11,6 +11,7 @@
 #include <linux/clk.h>
 #include <linux/delay.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 +707,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 +752,21 @@ 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);
+		dev_dbg(dev, "gpio reset de-asserted.\n");
+
+		/* 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(680);
+	}
+
 	ret = adc->chip->config(adc, external_vref);
 	if (ret)
 		return ret;

-- 
2.49.0

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

[Andy Shevchenko]

On Wed, Apr 23, 2025 at 04:46:51PM +0200, Marcus Folkesson wrote:
> Add support for optional HW reset.
> If specified, a reset will be asserted during driver probe.

...

> +		return dev_err_probe(dev, PTR_ERR(gpio_reset),
> +				     "Cannot get reset GPIO\n");

+ dev_printk.h

...

> +		dev_dbg(dev, "gpio reset de-asserted.\n");

How useful is this?

...

> +		/* 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... */

/*
 * Please, fix the nulti-line comment
 * style. This one can serve you as an
 * example.
 */

> +		fsleep(680);

Why not simply 500? or 600?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] MCP3911 fixes

[Jonathan Cameron]

On Wed, 23 Apr 2025 16:46:48 +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.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
I took a quick look as well. Nothing to add to Andy's detailed review
so I'll wait for v2.