[PATCH v4 0/9] Improve MCP3911 driver

[PATCH v4 0/9] Improve MCP3911 driver

[Marcus Folkesson]

Hi,

This patch series intend to fix bugs and improve functionality of the
MCP3911 driver.
The main features added are
- Support for buffers
- Interrupt driven readings
- Support for oversampling ratio
- Support for set scale values (Gain)

Among the bug fixes, there are changes in the formula for calculate raw
value and a fix for mismatch in the devicetree property.

Another general improvement for the driver is to use managed resources
for all allocated resources.

General changes for the series:

v3:
- Drop Phase patch
- Add Fixes tags for those patches that are fixes
- Move Fixes patches to the beginning of the patchset

v4:
- Split up devm-cleanup functions 
- Cosmetic cleanups
- Add
    select IIO_BUFFER
    select IIO_TRIGGERED_BUFFER
    To Kconfig
- Add .endianness = IIO_BE


Best regards,
Marcus Folkesson

[PATCH v4 1/9] iio: adc: mcp3911: make use of the sign bit

[Marcus Folkesson]

The device supports negative values as well.

Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 1cb4590fe412..f581cefb6719 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -113,6 +113,8 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			goto out;
 
+		*val = sign_extend32(*val, 23);
+
 		ret = IIO_VAL_INT;
 		break;
 
-- 
2.36.1

[PATCH v4 2/9] iio: adc: mcp3911: correct "microchip,device-addr" property

[Marcus Folkesson]

Go for the right property name that is documented in the bindings.

Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index f581cefb6719..f8875076ae80 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -210,7 +210,14 @@ static int mcp3911_config(struct mcp3911 *adc)
 	u32 configreg;
 	int ret;
 
-	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
+	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
+
+	/*
+	 * Fallback to "device-addr" due to historical mismatch between
+	 * dt-bindings and implementation
+	 */
+	if (ret)
+		device_property_read_u32(dev, "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",
-- 
2.36.1

[PATCH v4 3/9] iio: adc: mcp3911: use correct formula for AD conversion

[Marcus Folkesson]

The ADC conversion is actually not rail-to-rail but with a factor 1.5.
Make use of this factor when calculating actual voltage.

Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index f8875076ae80..890af7dca62d 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -40,8 +40,8 @@
 #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
+/* Internal voltage reference in mV */
+#define MCP3911_INT_VREF_MV		1200
 
 #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)
@@ -139,11 +139,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 			*val = ret / 1000;
 		} else {
-			*val = MCP3911_INT_VREF_UV;
+			*val = MCP3911_INT_VREF_MV;
 		}
 
-		*val2 = 24;
-		ret = IIO_VAL_FRACTIONAL_LOG2;
+		/*
+		 * For 24bit Conversion
+		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
+		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
+		 */
+
+		/* val2 = (2^23 * 1.5) */
+		*val2 = 12582912;
+		ret = IIO_VAL_FRACTIONAL;
 		break;
 	}
 
-- 
2.36.1

[PATCH v4 4/9] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Marcus Folkesson]

Keep using managed resources as much as possible.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/Kconfig   |  2 ++
 drivers/iio/adc/mcp3911.c | 51 ++++++++++++++++++++-------------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7fe5930891e0..3b5db59f3931 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -718,6 +718,8 @@ config MCP3422
 config MCP3911
 	tristate "Microchip Technology MCP3911 driver"
 	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Microchip Technology's MCP3911
 	  analog to digital converter.
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 890af7dca62d..00dadb1761dc 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -258,6 +258,18 @@ static int mcp3911_config(struct mcp3911 *adc)
 	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
 }
 
+static void mcp3911_cleanup_clock(void *_adc)
+{
+	struct mcp3911 *adc = _adc;
+	clk_disable_unprepare(adc->clki);
+}
+
+static void mcp3911_cleanup_regulator(void *_adc)
+{
+	struct mcp3911 *adc = _adc;
+	regulator_disable(adc->vref);
+}
+
 static int mcp3911_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -286,6 +298,11 @@ static int mcp3911_probe(struct spi_device *spi)
 		ret = regulator_enable(adc->vref);
 		if (ret)
 			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev,
+				mcp3911_cleanup_regulator, adc);
+		if (ret)
+			return ret;
 	}
 
 	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
@@ -296,21 +313,25 @@ static int mcp3911_probe(struct spi_device *spi)
 			dev_err(&adc->spi->dev,
 				"failed to get adc clk (%ld)\n",
 				PTR_ERR(adc->clki));
-			ret = PTR_ERR(adc->clki);
-			goto reg_disable;
+			return PTR_ERR(adc->clki);
 		}
 	} 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;
+			return ret;
 		}
+
+		ret = devm_add_action_or_reset(&spi->dev,
+				mcp3911_cleanup_clock, adc);
+		if (ret)
+			return ret;
 	}
 
 	ret = mcp3911_config(adc);
 	if (ret)
-		goto clk_disable;
+		return ret;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -322,31 +343,11 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto clk_disable;
-
-	return ret;
-
-clk_disable:
-	clk_disable_unprepare(adc->clki);
-reg_disable:
-	if (adc->vref)
-		regulator_disable(adc->vref);
-
-	return ret;
+	return devm_iio_device_register(&adc->spi->dev, indio_dev);
 }
 
 static void 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);
-
-	clk_disable_unprepare(adc->clki);
-	if (adc->vref)
-		regulator_disable(adc->vref);
 }
 
 static const struct of_device_id mcp3911_dt_ids[] = {
-- 
2.36.1

[PATCH v4 5/9] iio: adc: mcp3911: add support for buffers

[Marcus Folkesson]

Add support for buffers to make the driver fit for more usecases.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 96 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 00dadb1761dc..96c0a2a50c7c 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -5,15 +5,22 @@
  * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
  * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
  */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/trigger.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <asm/unaligned.h>
 
 #define MCP3911_REG_CHANNEL0		0x00
 #define MCP3911_REG_CHANNEL1		0x03
@@ -22,6 +29,7 @@
 #define MCP3911_REG_GAIN		0x09
 
 #define MCP3911_REG_STATUSCOM		0x0a
+#define MCP3911_STATUSCOM_READ		GENMASK(7, 6)
 #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
 #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
 #define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
@@ -54,6 +62,13 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct {
+		u32 channels[MCP3911_NUM_CHANNELS];
+		s64 ts __aligned(8);
+	} scan;
+
+	u8 tx_buf[MCP3911_NUM_CHANNELS * 3] __aligned(IIO_DMA_MINALIGN);
+	u8 rx_buf[MCP3911_NUM_CHANNELS * 3];
 };
 
 static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
@@ -196,16 +211,63 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
 		.channel = idx,					\
+		.scan_index = idx,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 24,				\
+			.storagebits = 32,			\
+			.endianness = IIO_BE,			\
+		},						\
 }
 
 static const struct iio_chan_spec mcp3911_channels[] = {
 	MCP3911_CHAN(0),
 	MCP3911_CHAN(1),
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
+static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct mcp3911 *adc = iio_priv(indio_dev);
+	struct spi_transfer xfer = {
+		.tx_buf = adc->tx_buf,
+		.rx_buf = adc->rx_buf,
+		.len = sizeof(adc->rx_buf),
+	};
+	int scan_index;
+	int i = 0;
+	int ret;
+
+	mutex_lock(&adc->lock);
+	adc->tx_buf[0] = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
+	ret = spi_sync_transfer(adc->spi, &xfer, 1);
+	if (ret < 0) {
+		dev_warn(&adc->spi->dev,
+				"failed to get conversion data\n");
+		goto out;
+	}
+
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			indio_dev->masklength) {
+		const struct iio_chan_spec *scan_chan = &indio_dev->channels[scan_index];
+
+		adc->scan.channels[i] = get_unaligned_be24(&adc->rx_buf[scan_chan->channel * 3]);
+		i++;
+	}
+	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
+			iio_get_time_ns(indio_dev));
+out:
+	mutex_unlock(&adc->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info mcp3911_info = {
 	.read_raw = mcp3911_read_raw,
 	.write_raw = mcp3911_write_raw,
@@ -214,7 +276,7 @@ static const struct iio_info mcp3911_info = {
 static int mcp3911_config(struct mcp3911 *adc)
 {
 	struct device *dev = &adc->spi->dev;
-	u32 configreg;
+	u32 regval;
 	int ret;
 
 	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
@@ -233,29 +295,43 @@ static int mcp3911_config(struct mcp3911 *adc)
 	}
 	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
 
-	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
+	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &regval, 2);
 	if (ret)
 		return ret;
 
+	regval &= ~MCP3911_CONFIG_VREFEXT;
 	if (adc->vref) {
 		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
-		configreg |= MCP3911_CONFIG_VREFEXT;
+		regval |= FIELD_PREP(MCP3911_CONFIG_VREFEXT, 1);
 	} else {
 		dev_dbg(&adc->spi->dev,
 			"use internal voltage reference (1.2V)\n");
-		configreg &= ~MCP3911_CONFIG_VREFEXT;
+		regval |= FIELD_PREP(MCP3911_CONFIG_VREFEXT, 0);
 	}
 
+	regval &= ~MCP3911_CONFIG_CLKEXT;
 	if (adc->clki) {
 		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
-		configreg |= MCP3911_CONFIG_CLKEXT;
+		regval |= FIELD_PREP(MCP3911_CONFIG_CLKEXT, 1);
 	} else {
 		dev_dbg(&adc->spi->dev,
 			"use crystal oscillator as clocksource\n");
-		configreg &= ~MCP3911_CONFIG_CLKEXT;
+		regval |= FIELD_PREP(MCP3911_CONFIG_CLKEXT, 0);
 	}
 
-	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
+	ret = mcp3911_write(adc, MCP3911_REG_CONFIG, regval, 2);
+	if (ret)
+		return ret;
+
+	ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &regval, 2);
+	if (ret)
+		return ret;
+
+	/* Address counter incremented, cycle through register types */
+	regval &= ~MCP3911_STATUSCOM_READ;
+	regval |= FIELD_PREP(MCP3911_STATUSCOM_READ, 0x02);
+
+	return  mcp3911_write(adc, MCP3911_REG_STATUSCOM, regval, 2);
 }
 
 static void mcp3911_cleanup_clock(void *_adc)
@@ -343,6 +419,12 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+			NULL,
+			mcp3911_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(&adc->spi->dev, indio_dev);
 }
 
-- 
2.36.1

[PATCH v4 6/9] iio: adc: mcp3911: add support for interrupts

[Marcus Folkesson]

Make it possible to read values upon interrupts.
Configure Data Ready Signal Output Pin to either HiZ or push-pull and
use it as interrupt source.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 96c0a2a50c7c..7db2c75da4ac 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -29,6 +29,7 @@
 #define MCP3911_REG_GAIN		0x09
 
 #define MCP3911_REG_STATUSCOM		0x0a
+#define MCP3911_STATUSCOM_DRHIZ         BIT(12)
 #define MCP3911_STATUSCOM_READ		GENMASK(7, 6)
 #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
 #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
@@ -62,6 +63,7 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct iio_trigger *trig;
 	struct {
 		u32 channels[MCP3911_NUM_CHANNELS];
 		s64 ts __aligned(8);
@@ -346,6 +348,23 @@ static void mcp3911_cleanup_regulator(void *_adc)
 	regulator_disable(adc->vref);
 }
 
+static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
+{
+	struct mcp3911 *adc = iio_trigger_get_drvdata(trig);
+
+	if (enable)
+		enable_irq(adc->spi->irq);
+	else
+		disable_irq(adc->spi->irq);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops mcp3911_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+	.set_trigger_state = mcp3911_set_trigger_state,
+};
+
 static int mcp3911_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -409,6 +428,15 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
+		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+				0, 2);
+	else
+		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+				MCP3911_STATUSCOM_DRHIZ, 2);
+	if (ret)
+		return ret;
+
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mcp3911_info;
@@ -419,6 +447,31 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
+	if (spi->irq > 0) {
+		adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
+				indio_dev->name,
+				iio_device_id(indio_dev));
+		if (!adc->trig)
+			return PTR_ERR(adc->trig);
+
+		adc->trig->ops = &mcp3911_trigger_ops;
+		iio_trigger_set_drvdata(adc->trig, adc);
+		ret = devm_iio_trigger_register(&spi->dev, adc->trig);
+		if (ret)
+			return ret;
+
+		/*
+		 * The device generates interrupts as long as it is powered up.
+		 * Some platforms might not allow the option to power it down so
+		 * don't enable the interrupt to avoid extra load on the system.
+		 */
+		ret = devm_request_irq(&spi->dev, spi->irq,
+				&iio_trigger_generic_data_rdy_poll, IRQF_NO_AUTOEN | IRQF_ONESHOT,
+				indio_dev->name, adc->trig);
+		if (ret)
+			return ret;
+	}
+
 	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 			NULL,
 			mcp3911_trigger_handler, NULL);
-- 
2.36.1

[PATCH v4 7/9] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

[Marcus Folkesson]

The Data Ready Output Pin is either hard wired to work as high
impedance or push-pull. Make it configurable.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml     | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index 95ab285f4eba..57a16380f864 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -36,6 +36,13 @@ properties:
     description: IRQ line of the ADC
     maxItems: 1
 
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control
+      true = The DR pin state is high-impedance when data are NOT ready
+      false = The DR pin state is a logic high when data are NOT ready
+    type: boolean
+
   microchip,device-addr:
     description: Device address when multiple MCP3911 chips are present on the same SPI bus.
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.36.1

[PATCH v4 8/9] iio: adc: mcp3911: add support for oversampling ratio

[Marcus Folkesson]

The chip supports oversampling ratio, so expose it to userspace.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 58 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 7db2c75da4ac..30c91ccc5fb6 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -39,6 +39,7 @@
 #define MCP3911_REG_CONFIG		0x0c
 #define MCP3911_CONFIG_CLKEXT		BIT(1)
 #define MCP3911_CONFIG_VREFEXT		BIT(2)
+#define MCP3911_CONFIG_OSR		GENMASK(13, 11)
 
 #define MCP3911_REG_OFFCAL_CH0		0x0e
 #define MCP3911_REG_GAINCAL_CH0		0x11
@@ -57,6 +58,8 @@
 
 #define MCP3911_NUM_CHANNELS		2
 
+static const int mcp3911_osr_table[] = { 32, 64, 128, 256, 512, 1024, 2048, 4096 };
+
 struct mcp3911 {
 	struct spi_device *spi;
 	struct mutex lock;
@@ -115,6 +118,36 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
 	return mcp3911_write(adc, reg, val, len);
 }
 
+static int mcp3911_write_raw_get_fmt(struct iio_dev *indio_dev,
+					struct iio_chan_spec const *chan,
+					long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return IIO_VAL_INT;
+	default:
+		return IIO_VAL_INT_PLUS_NANO;
+	}
+}
+
+static int mcp3911_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*type = IIO_VAL_INT;
+		*vals = mcp3911_osr_table;
+		*length = ARRAY_SIZE(mcp3911_osr_table);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int mcp3911_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
@@ -143,6 +176,15 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 		ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = mcp3911_read(adc, MCP3911_REG_CONFIG, val, 2);
+		if (ret)
+			goto out;
+
+		*val = FIELD_GET(MCP3911_CONFIG_OSR, *val);
+		*val = 32 << *val;
+		ret = IIO_VAL_INT;
+		break;
 
 	case IIO_CHAN_INFO_SCALE:
 		if (adc->vref) {
@@ -202,6 +244,17 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 				MCP3911_STATUSCOM_EN_OFFCAL,
 				MCP3911_STATUSCOM_EN_OFFCAL, 2);
 		break;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
+			if (val == mcp3911_osr_table[i]) {
+				val = FIELD_PREP(MCP3911_CONFIG_OSR, i);
+				ret = mcp3911_update(adc, MCP3911_REG_CONFIG, MCP3911_CONFIG_OSR,
+						val, 2);
+				break;
+			}
+		}
+		break;
 	}
 
 out:
@@ -214,9 +267,12 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.indexed = 1,					\
 		.channel = idx,					\
 		.scan_index = idx,				\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
+		.info_mask_shared_by_type_available =		\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = 24,				\
@@ -273,6 +329,8 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
 static const struct iio_info mcp3911_info = {
 	.read_raw = mcp3911_read_raw,
 	.write_raw = mcp3911_write_raw,
+	.read_avail = mcp3911_read_avail,
+	.write_raw_get_fmt = mcp3911_write_raw_get_fmt,
 };
 
 static int mcp3911_config(struct mcp3911 *adc)
-- 
2.36.1

[PATCH v4 9/9] iio: adc: mcp3911: add support to set PGA

[Marcus Folkesson]

Add support for setting the Programmable Gain Amplifiers by adjust the
scale value.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 107 +++++++++++++++++++++++++++++---------
 1 file changed, 83 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 30c91ccc5fb6..22a43d3fe402 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -27,6 +27,8 @@
 #define MCP3911_REG_MOD			0x06
 #define MCP3911_REG_PHASE		0x07
 #define MCP3911_REG_GAIN		0x09
+#define MCP3911_GAIN_MASK(ch)		(GENMASK(2, 0) << 3 * ch)
+#define MCP3911_GAIN_VAL(ch, val)      ((val << 3 * ch) & MCP3911_GAIN_MASK(ch))
 
 #define MCP3911_REG_STATUSCOM		0x0a
 #define MCP3911_STATUSCOM_DRHIZ         BIT(12)
@@ -57,8 +59,10 @@
 #define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
 
 #define MCP3911_NUM_CHANNELS		2
+#define MCP3911_NUM_SCALES		6
 
 static const int mcp3911_osr_table[] = { 32, 64, 128, 256, 512, 1024, 2048, 4096 };
+static u32 mcp3911_scale_table[MCP3911_NUM_SCALES][2];
 
 struct mcp3911 {
 	struct spi_device *spi;
@@ -67,6 +71,7 @@ struct mcp3911 {
 	struct clk *clki;
 	u32 dev_addr;
 	struct iio_trigger *trig;
+	u32 gain[MCP3911_NUM_CHANNELS];
 	struct {
 		u32 channels[MCP3911_NUM_CHANNELS];
 		s64 ts __aligned(8);
@@ -143,6 +148,11 @@ static int mcp3911_read_avail(struct iio_dev *indio_dev,
 		*vals = mcp3911_osr_table;
 		*length = ARRAY_SIZE(mcp3911_osr_table);
 		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*vals = (int *)mcp3911_scale_table;
+		*length = ARRAY_SIZE(mcp3911_scale_table) * 2;
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
@@ -187,29 +197,9 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 		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_MV;
-		}
-
-		/*
-		 * For 24bit Conversion
-		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
-		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
-		 */
-
-		/* val2 = (2^23 * 1.5) */
-		*val2 = 12582912;
-		ret = IIO_VAL_FRACTIONAL;
+		*val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0];
+		*val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1];
+		ret = IIO_VAL_INT_PLUS_NANO;
 		break;
 	}
 
@@ -227,6 +217,18 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&adc->lock);
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
+			if (val == mcp3911_scale_table[i][0] &&
+				val2 == mcp3911_scale_table[i][1]) {
+
+				adc->gain[channel->channel] = BIT(i);
+				ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+						MCP3911_GAIN_MASK(channel->channel),
+						MCP3911_GAIN_VAL(channel->channel, i), 1);
+			}
+		}
+		break;
 	case IIO_CHAN_INFO_OFFSET:
 		if (val2 != 0) {
 			ret = -EINVAL;
@@ -262,6 +264,47 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int mcp3911_calc_scale_table(struct mcp3911 *adc)
+{
+	u32 ref = MCP3911_INT_VREF_MV;
+	u32 div;
+	int ret;
+	int tmp0, tmp1;
+	s64 tmp2;
+
+	if (adc->vref) {
+		ret = regulator_get_voltage(adc->vref);
+		if (ret < 0) {
+			dev_err(&adc->spi->dev,
+				"failed to get vref voltage: %d\n",
+			       ret);
+			return ret;
+		}
+
+		ref = ret / 1000;
+	}
+
+	/*
+	 * For 24-bit Conversion
+	 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
+	 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
+	 *
+	 * ref = Reference voltage
+	 * div = (2^23 * 1.5 * gain) = 12582912 * gain
+	 */
+	for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
+		div = 12582912 * BIT(i);
+		tmp2 = div_s64((s64)ref * 1000000000LL, div);
+		tmp1 = div;
+		tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
+
+		mcp3911_scale_table[i][0] = 0;
+		mcp3911_scale_table[i][1] = tmp1;
+	}
+
+	return 0;
+}
+
 #define MCP3911_CHAN(idx) {					\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
@@ -271,8 +314,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
-		.info_mask_shared_by_type_available =		\
+		.info_mask_shared_by_type_available =           \
 			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.info_mask_separate_available =			\
+			BIT(IIO_CHAN_INFO_SCALE),		\
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = 24,				\
@@ -495,6 +540,20 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	ret = mcp3911_calc_scale_table(adc);
+	if (ret)
+		return ret;
+
+       /* Set gain to 1 for all channels */
+	for (int i = 0; i < MCP3911_NUM_CHANNELS; i++) {
+		adc->gain[i] = 1;
+		ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+				MCP3911_GAIN_MASK(i),
+				MCP3911_GAIN_VAL(i, 0), 1);
+		if (ret)
+			return ret;
+	}
+
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mcp3911_info;
-- 
2.36.1

Re: [PATCH v4 4/9] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:21 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Keep using managed resources as much as possible.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Hi Marcus,

Not sure how I missed the below on previous versions :(

Jonathan

> ---
>  drivers/iio/adc/Kconfig   |  2 ++
>  drivers/iio/adc/mcp3911.c | 51 ++++++++++++++++++++-------------------
>  2 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7fe5930891e0..3b5db59f3931 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -718,6 +718,8 @@ config MCP3422
>  config MCP3911
>  	tristate "Microchip Technology MCP3911 driver"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Microchip Technology's MCP3911
>  	  analog to digital converter.
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 890af7dca62d..00dadb1761dc 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -258,6 +258,18 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
>  }
>  
> +static void mcp3911_cleanup_clock(void *_adc)
> +{
> +	struct mcp3911 *adc = _adc;
> +	clk_disable_unprepare(adc->clki);
> +}
> +
> +static void mcp3911_cleanup_regulator(void *_adc)
> +{
> +	struct mcp3911 *adc = _adc;
> +	regulator_disable(adc->vref);
> +}
> +
>  static int mcp3911_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -286,6 +298,11 @@ static int mcp3911_probe(struct spi_device *spi)
>  		ret = regulator_enable(adc->vref);
>  		if (ret)
>  			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +				mcp3911_cleanup_regulator, adc);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
> @@ -296,21 +313,25 @@ static int mcp3911_probe(struct spi_device *spi)
>  			dev_err(&adc->spi->dev,
>  				"failed to get adc clk (%ld)\n",
>  				PTR_ERR(adc->clki));
> -			ret = PTR_ERR(adc->clki);
> -			goto reg_disable;
> +			return PTR_ERR(adc->clki);
>  		}
>  	} 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;
> +			return ret;
>  		}
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +				mcp3911_cleanup_clock, adc);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	ret = mcp3911_config(adc);
>  	if (ret)
> -		goto clk_disable;
> +		return ret;
>  
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -322,31 +343,11 @@ static int mcp3911_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		goto clk_disable;
> -
> -	return ret;
> -
> -clk_disable:
> -	clk_disable_unprepare(adc->clki);
> -reg_disable:
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
> -
> -	return ret;
> +	return devm_iio_device_register(&adc->spi->dev, indio_dev);
>  }
>  
>  static void mcp3911_remove(struct spi_device *spi)

There is no requirement to provide a remove function.  So not it
is empty get rid of it completely.

>  {
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct mcp3911 *adc = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	clk_disable_unprepare(adc->clki);
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
>  }
>  
>  static const struct of_device_id mcp3911_dt_ids[] = {

Re: [PATCH v4 4/9] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:21 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Keep using managed resources as much as possible.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/adc/Kconfig   |  2 ++
>  drivers/iio/adc/mcp3911.c | 51 ++++++++++++++++++++-------------------
>  2 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7fe5930891e0..3b5db59f3931 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -718,6 +718,8 @@ config MCP3422
>  config MCP3911
>  	tristate "Microchip Technology MCP3911 driver"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER

This is unrelated to the devm cleanups, so should be in the patch that
introduces the code that needs them (next patch I think).

Thanks,

Jonathan



>  	help
>  	  Say yes here to build support for Microchip Technology's MCP3911
>  	  analog to digital converter.
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 890af7dca62d..00dadb1761dc 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -258,6 +258,18 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
>  }
>  
> +static void mcp3911_cleanup_clock(void *_adc)
> +{
> +	struct mcp3911 *adc = _adc;
> +	clk_disable_unprepare(adc->clki);
> +}
> +
> +static void mcp3911_cleanup_regulator(void *_adc)
> +{
> +	struct mcp3911 *adc = _adc;
> +	regulator_disable(adc->vref);
> +}
> +
>  static int mcp3911_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -286,6 +298,11 @@ static int mcp3911_probe(struct spi_device *spi)
>  		ret = regulator_enable(adc->vref);
>  		if (ret)
>  			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +				mcp3911_cleanup_regulator, adc);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
> @@ -296,21 +313,25 @@ static int mcp3911_probe(struct spi_device *spi)
>  			dev_err(&adc->spi->dev,
>  				"failed to get adc clk (%ld)\n",
>  				PTR_ERR(adc->clki));
> -			ret = PTR_ERR(adc->clki);
> -			goto reg_disable;
> +			return PTR_ERR(adc->clki);
>  		}
>  	} 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;
> +			return ret;
>  		}
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +				mcp3911_cleanup_clock, adc);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	ret = mcp3911_config(adc);
>  	if (ret)
> -		goto clk_disable;
> +		return ret;
>  
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -322,31 +343,11 @@ static int mcp3911_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		goto clk_disable;
> -
> -	return ret;
> -
> -clk_disable:
> -	clk_disable_unprepare(adc->clki);
> -reg_disable:
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
> -
> -	return ret;
> +	return devm_iio_device_register(&adc->spi->dev, indio_dev);
>  }
>  
>  static void 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);
> -
> -	clk_disable_unprepare(adc->clki);
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
>  }
>  
>  static const struct of_device_id mcp3911_dt_ids[] = {

Re: [PATCH v4 0/9] Improve MCP3911 driver

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:17 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Hi,
> 
> This patch series intend to fix bugs and improve functionality of the
> MCP3911 driver.
> The main features added are
> - Support for buffers
> - Interrupt driven readings
> - Support for oversampling ratio
> - Support for set scale values (Gain)
> 
> Among the bug fixes, there are changes in the formula for calculate raw
> value and a fix for mismatch in the devicetree property.
> 
> Another general improvement for the driver is to use managed resources
> for all allocated resources.
> 
Hi Marcus,

The first 3 fixes look good to me.  Do you want me to pick those up to
go in after rc1 via my togreg-fixes branch?  The side effect of doing
that is it'll be a little while before they are upstream in the branch
I'll want to pick the rest of the series on top of.

So it's a trade off between getting fixes in as soon as possible and
slowing down other improvements a little.

Jonathan

> General changes for the series:
> 
> v3:
> - Drop Phase patch
> - Add Fixes tags for those patches that are fixes
> - Move Fixes patches to the beginning of the patchset
> 
> v4:
> - Split up devm-cleanup functions 
> - Cosmetic cleanups
> - Add
>     select IIO_BUFFER
>     select IIO_TRIGGERED_BUFFER
>     To Kconfig
> - Add .endianness = IIO_BE
> 
> 
> Best regards,
> Marcus Folkesson
> 
> 
> 

Re: [PATCH v4 5/9] iio: adc: mcp3911: add support for buffers

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:22 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Add support for buffers to make the driver fit for more usecases.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Assuming the Kconfig change from previous patch is pulled into this one...

A few questions / comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mcp3911.c | 96 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 00dadb1761dc..96c0a2a50c7c 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -5,15 +5,22 @@
>   * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
>   * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
>   */
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/trigger.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>

Line break here to separate the 'chunks' of includes.

> +#include <asm/unaligned.h>
>  
>  #define MCP3911_REG_CHANNEL0		0x00
>  #define MCP3911_REG_CHANNEL1		0x03
> @@ -22,6 +29,7 @@
>  #define MCP3911_REG_GAIN		0x09
>  
>  #define MCP3911_REG_STATUSCOM		0x0a
> +#define MCP3911_STATUSCOM_READ		GENMASK(7, 6)
>  #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
>  #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
>  #define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
> @@ -54,6 +62,13 @@ struct mcp3911 {
>  	struct regulator *vref;
>  	struct clk *clki;
>  	u32 dev_addr;
> +	struct {
> +		u32 channels[MCP3911_NUM_CHANNELS];
> +		s64 ts __aligned(8);
> +	} scan;
> +
> +	u8 tx_buf[MCP3911_NUM_CHANNELS * 3] __aligned(IIO_DMA_MINALIGN);
> +	u8 rx_buf[MCP3911_NUM_CHANNELS * 3];
>  };
>  
>  static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> @@ -196,16 +211,63 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  		.type = IIO_VOLTAGE,				\
>  		.indexed = 1,					\
>  		.channel = idx,					\
> +		.scan_index = idx,				\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>  			BIT(IIO_CHAN_INFO_OFFSET) |		\
>  			BIT(IIO_CHAN_INFO_SCALE),		\
> +		.scan_type = {					\
> +			.sign = 's',				\
> +			.realbits = 24,				\
> +			.storagebits = 32,			\
> +			.endianness = IIO_BE,			\
> +		},						\
>  }
>  
>  static const struct iio_chan_spec mcp3911_channels[] = {
>  	MCP3911_CHAN(0),
>  	MCP3911_CHAN(1),
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };
>  
> +static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct mcp3911 *adc = iio_priv(indio_dev);
> +	struct spi_transfer xfer = {
> +		.tx_buf = adc->tx_buf,
> +		.rx_buf = adc->rx_buf,
> +		.len = sizeof(adc->rx_buf),
> +	};
> +	int scan_index;
> +	int i = 0;
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +	adc->tx_buf[0] = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
> +	ret = spi_sync_transfer(adc->spi, &xfer, 1);
> +	if (ret < 0) {
> +		dev_warn(&adc->spi->dev,
> +				"failed to get conversion data\n");
> +		goto out;
> +	}
> +
> +	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> +			indio_dev->masklength) {
> +		const struct iio_chan_spec *scan_chan = &indio_dev->channels[scan_index];
> +
> +		adc->scan.channels[i] = get_unaligned_be24(&adc->rx_buf[scan_chan->channel * 3]);

This has me a little curious.  It seems to be potentially reading from byte 0 which in the spi
transfer is at the same time as the tx that tells the device what the command is.  I'd expect
it to be one byte later.  Easiest way to do that being to have two transfers (though you could
just add to the offset). I might be misremembering how the spi_transfer stuff works though.
Been a while since I hacked anything SPI based...

> +		i++;
> +	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
> +			iio_get_time_ns(indio_dev));
> +out:
> +	mutex_unlock(&adc->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info mcp3911_info = {
>  	.read_raw = mcp3911_read_raw,
>  	.write_raw = mcp3911_write_raw,
> @@ -214,7 +276,7 @@ static const struct iio_info mcp3911_info = {
>  static int mcp3911_config(struct mcp3911 *adc)
>  {
>  	struct device *dev = &adc->spi->dev;
> -	u32 configreg;
> +	u32 regval;
>  	int ret;
>  
>  	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
> @@ -233,29 +295,43 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	}
>  	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
>  
> -	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
> +	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &regval, 2);

If I was being fussy I'd ask you to pull the refactoring in here out as a precusor
patch to simplify reviewing the new stuff a little.  However it's fairly minor
so I'll let that go this time.

>  	if (ret)
>  		return ret;
>  
> +	regval &= ~MCP3911_CONFIG_VREFEXT;
>  	if (adc->vref) {
>  		dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> -		configreg |= MCP3911_CONFIG_VREFEXT;
> +		regval |= FIELD_PREP(MCP3911_CONFIG_VREFEXT, 1);
>  	} else {
>  		dev_dbg(&adc->spi->dev,
>  			"use internal voltage reference (1.2V)\n");
> -		configreg &= ~MCP3911_CONFIG_VREFEXT;
> +		regval |= FIELD_PREP(MCP3911_CONFIG_VREFEXT, 0);
>  	}
>  
> +	regval &= ~MCP3911_CONFIG_CLKEXT;
>  	if (adc->clki) {
>  		dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> -		configreg |= MCP3911_CONFIG_CLKEXT;
> +		regval |= FIELD_PREP(MCP3911_CONFIG_CLKEXT, 1);
>  	} else {
>  		dev_dbg(&adc->spi->dev,
>  			"use crystal oscillator as clocksource\n");
> -		configreg &= ~MCP3911_CONFIG_CLKEXT;
> +		regval |= FIELD_PREP(MCP3911_CONFIG_CLKEXT, 0);
>  	}
>  
> -	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> +	ret = mcp3911_write(adc, MCP3911_REG_CONFIG, regval, 2);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &regval, 2);
> +	if (ret)
> +		return ret;
> +
> +	/* Address counter incremented, cycle through register types */
> +	regval &= ~MCP3911_STATUSCOM_READ;
> +	regval |= FIELD_PREP(MCP3911_STATUSCOM_READ, 0x02);
> +
> +	return  mcp3911_write(adc, MCP3911_REG_STATUSCOM, regval, 2);
>  }
>  

Re: [PATCH v4 6/9] iio: adc: mcp3911: add support for interrupts

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:23 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Make it possible to read values upon interrupts.
> Configure Data Ready Signal Output Pin to either HiZ or push-pull and
> use it as interrupt source.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Looks good to me.

J

> ---
>  drivers/iio/adc/mcp3911.c | 53 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 96c0a2a50c7c..7db2c75da4ac 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -29,6 +29,7 @@
>  #define MCP3911_REG_GAIN		0x09
>  
>  #define MCP3911_REG_STATUSCOM		0x0a
> +#define MCP3911_STATUSCOM_DRHIZ         BIT(12)
>  #define MCP3911_STATUSCOM_READ		GENMASK(7, 6)
>  #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
>  #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
> @@ -62,6 +63,7 @@ struct mcp3911 {
>  	struct regulator *vref;
>  	struct clk *clki;
>  	u32 dev_addr;
> +	struct iio_trigger *trig;
>  	struct {
>  		u32 channels[MCP3911_NUM_CHANNELS];
>  		s64 ts __aligned(8);
> @@ -346,6 +348,23 @@ static void mcp3911_cleanup_regulator(void *_adc)
>  	regulator_disable(adc->vref);
>  }
>  
> +static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{
> +	struct mcp3911 *adc = iio_trigger_get_drvdata(trig);
> +
> +	if (enable)
> +		enable_irq(adc->spi->irq);
> +	else
> +		disable_irq(adc->spi->irq);
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops mcp3911_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +	.set_trigger_state = mcp3911_set_trigger_state,
> +};
> +
>  static int mcp3911_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -409,6 +428,15 @@ static int mcp3911_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
> +		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
> +				0, 2);
> +	else
> +		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
> +				MCP3911_STATUSCOM_DRHIZ, 2);
> +	if (ret)
> +		return ret;
> +
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &mcp3911_info;
> @@ -419,6 +447,31 @@ static int mcp3911_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> +	if (spi->irq > 0) {
> +		adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> +				indio_dev->name,
> +				iio_device_id(indio_dev));
> +		if (!adc->trig)
> +			return PTR_ERR(adc->trig);
> +
> +		adc->trig->ops = &mcp3911_trigger_ops;
> +		iio_trigger_set_drvdata(adc->trig, adc);
> +		ret = devm_iio_trigger_register(&spi->dev, adc->trig);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * The device generates interrupts as long as it is powered up.
> +		 * Some platforms might not allow the option to power it down so
> +		 * don't enable the interrupt to avoid extra load on the system.
> +		 */
> +		ret = devm_request_irq(&spi->dev, spi->irq,
> +				&iio_trigger_generic_data_rdy_poll, IRQF_NO_AUTOEN | IRQF_ONESHOT,
> +				indio_dev->name, adc->trig);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
>  			NULL,
>  			mcp3911_trigger_handler, NULL);

Re: [PATCH v4 9/9] iio: adc: mcp3911: add support to set PGA

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:26 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Add support for setting the Programmable Gain Amplifiers by adjust the
> scale value.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

This and other patches I haven't commented on all look good to me.

> ---
>  drivers/iio/adc/mcp3911.c | 107 +++++++++++++++++++++++++++++---------
>  1 file changed, 83 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 30c91ccc5fb6..22a43d3fe402 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -27,6 +27,8 @@
>  #define MCP3911_REG_MOD			0x06
>  #define MCP3911_REG_PHASE		0x07
>  #define MCP3911_REG_GAIN		0x09
> +#define MCP3911_GAIN_MASK(ch)		(GENMASK(2, 0) << 3 * ch)
> +#define MCP3911_GAIN_VAL(ch, val)      ((val << 3 * ch) & MCP3911_GAIN_MASK(ch))
>  
>  #define MCP3911_REG_STATUSCOM		0x0a
>  #define MCP3911_STATUSCOM_DRHIZ         BIT(12)
> @@ -57,8 +59,10 @@
>  #define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
>  
>  #define MCP3911_NUM_CHANNELS		2
> +#define MCP3911_NUM_SCALES		6
>  
>  static const int mcp3911_osr_table[] = { 32, 64, 128, 256, 512, 1024, 2048, 4096 };
> +static u32 mcp3911_scale_table[MCP3911_NUM_SCALES][2];
>  
>  struct mcp3911 {
>  	struct spi_device *spi;
> @@ -67,6 +71,7 @@ struct mcp3911 {
>  	struct clk *clki;
>  	u32 dev_addr;
>  	struct iio_trigger *trig;
> +	u32 gain[MCP3911_NUM_CHANNELS];
>  	struct {
>  		u32 channels[MCP3911_NUM_CHANNELS];
>  		s64 ts __aligned(8);
> @@ -143,6 +148,11 @@ static int mcp3911_read_avail(struct iio_dev *indio_dev,
>  		*vals = mcp3911_osr_table;
>  		*length = ARRAY_SIZE(mcp3911_osr_table);
>  		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SCALE:
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*vals = (int *)mcp3911_scale_table;
> +		*length = ARRAY_SIZE(mcp3911_scale_table) * 2;
> +		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -187,29 +197,9 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
>  		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_MV;
> -		}
> -
> -		/*
> -		 * For 24bit Conversion
> -		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
> -		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
> -		 */
> -
> -		/* val2 = (2^23 * 1.5) */
> -		*val2 = 12582912;
> -		ret = IIO_VAL_FRACTIONAL;
> +		*val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0];
> +		*val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1];
> +		ret = IIO_VAL_INT_PLUS_NANO;
>  		break;
>  	}
>  
> @@ -227,6 +217,18 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&adc->lock);
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
> +			if (val == mcp3911_scale_table[i][0] &&
> +				val2 == mcp3911_scale_table[i][1]) {
> +
> +				adc->gain[channel->channel] = BIT(i);
> +				ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> +						MCP3911_GAIN_MASK(channel->channel),
> +						MCP3911_GAIN_VAL(channel->channel, i), 1);
> +			}
> +		}
> +		break;
>  	case IIO_CHAN_INFO_OFFSET:
>  		if (val2 != 0) {
>  			ret = -EINVAL;
> @@ -262,6 +264,47 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int mcp3911_calc_scale_table(struct mcp3911 *adc)
> +{
> +	u32 ref = MCP3911_INT_VREF_MV;
> +	u32 div;
> +	int ret;
> +	int tmp0, tmp1;
> +	s64 tmp2;
> +
> +	if (adc->vref) {
> +		ret = regulator_get_voltage(adc->vref);
> +		if (ret < 0) {
> +			dev_err(&adc->spi->dev,
> +				"failed to get vref voltage: %d\n",
> +			       ret);
> +			return ret;
> +		}
> +
> +		ref = ret / 1000;
> +	}
> +
> +	/*
> +	 * For 24-bit Conversion
> +	 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
> +	 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
> +	 *
> +	 * ref = Reference voltage
> +	 * div = (2^23 * 1.5 * gain) = 12582912 * gain
> +	 */
> +	for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
> +		div = 12582912 * BIT(i);
> +		tmp2 = div_s64((s64)ref * 1000000000LL, div);
> +		tmp1 = div;
> +		tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
> +
> +		mcp3911_scale_table[i][0] = 0;
> +		mcp3911_scale_table[i][1] = tmp1;
> +	}
> +
> +	return 0;
> +}
> +
>  #define MCP3911_CHAN(idx) {					\
>  		.type = IIO_VOLTAGE,				\
>  		.indexed = 1,					\
> @@ -271,8 +314,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>  			BIT(IIO_CHAN_INFO_OFFSET) |		\
>  			BIT(IIO_CHAN_INFO_SCALE),		\
> -		.info_mask_shared_by_type_available =		\
> +		.info_mask_shared_by_type_available =           \
>  			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +		.info_mask_separate_available =			\
> +			BIT(IIO_CHAN_INFO_SCALE),		\
>  		.scan_type = {					\
>  			.sign = 's',				\
>  			.realbits = 24,				\
> @@ -495,6 +540,20 @@ static int mcp3911_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	ret = mcp3911_calc_scale_table(adc);
> +	if (ret)
> +		return ret;
> +
> +       /* Set gain to 1 for all channels */
> +	for (int i = 0; i < MCP3911_NUM_CHANNELS; i++) {
> +		adc->gain[i] = 1;
> +		ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> +				MCP3911_GAIN_MASK(i),
> +				MCP3911_GAIN_VAL(i, 0), 1);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &mcp3911_info;

Re: [PATCH v4 5/9] iio: adc: mcp3911: add support for buffers

[Marcus Folkesson]

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

Hi Jonathan,

On Sun, Jul 31, 2022 at 05:51:21PM +0100, Jonathan Cameron wrote:
> On Fri, 22 Jul 2022 15:07:22 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > Add support for buffers to make the driver fit for more usecases.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Assuming the Kconfig change from previous patch is pulled into this one...

Yep

> 
> A few questions / comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/mcp3911.c | 96 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 89 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > index 00dadb1761dc..96c0a2a50c7c 100644
> > --- a/drivers/iio/adc/mcp3911.c
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -5,15 +5,22 @@
> >   * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> >   * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
> >   */
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/trigger.h>
> >  #include <linux/module.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/property.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> 
> Line break here to separate the 'chunks' of includes.
> 

OK

> > +#include <asm/unaligned.h>
> >  
> >  #define MCP3911_REG_CHANNEL0		0x00
> >  #define MCP3911_REG_CHANNEL1		0x03
> > @@ -22,6 +29,7 @@
> >  #define MCP3911_REG_GAIN		0x09
> >  
> >  #define MCP3911_REG_STATUSCOM		0x0a
> > +#define MCP3911_STATUSCOM_READ		GENMASK(7, 6)
> >  #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
> >  #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
> >  #define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
> > @@ -54,6 +62,13 @@ struct mcp3911 {
> >  	struct regulator *vref;
> >  	struct clk *clki;
> >  	u32 dev_addr;
> > +	struct {
> > +		u32 channels[MCP3911_NUM_CHANNELS];
> > +		s64 ts __aligned(8);
> > +	} scan;
> > +
> > +	u8 tx_buf[MCP3911_NUM_CHANNELS * 3] __aligned(IIO_DMA_MINALIGN);
> > +	u8 rx_buf[MCP3911_NUM_CHANNELS * 3];
> >  };
> >  
> >  static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > @@ -196,16 +211,63 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
> >  		.type = IIO_VOLTAGE,				\
> >  		.indexed = 1,					\
> >  		.channel = idx,					\
> > +		.scan_index = idx,				\
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> >  			BIT(IIO_CHAN_INFO_OFFSET) |		\
> >  			BIT(IIO_CHAN_INFO_SCALE),		\
> > +		.scan_type = {					\
> > +			.sign = 's',				\
> > +			.realbits = 24,				\
> > +			.storagebits = 32,			\
> > +			.endianness = IIO_BE,			\
> > +		},						\
> >  }
> >  
> >  static const struct iio_chan_spec mcp3911_channels[] = {
> >  	MCP3911_CHAN(0),
> >  	MCP3911_CHAN(1),
> > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> >  };
> >  
> > +static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct mcp3911 *adc = iio_priv(indio_dev);
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = adc->tx_buf,
> > +		.rx_buf = adc->rx_buf,
> > +		.len = sizeof(adc->rx_buf),
> > +	};
> > +	int scan_index;
> > +	int i = 0;
> > +	int ret;
> > +
> > +	mutex_lock(&adc->lock);
> > +	adc->tx_buf[0] = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
> > +	ret = spi_sync_transfer(adc->spi, &xfer, 1);
> > +	if (ret < 0) {
> > +		dev_warn(&adc->spi->dev,
> > +				"failed to get conversion data\n");
> > +		goto out;
> > +	}
> > +
> > +	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> > +			indio_dev->masklength) {
> > +		const struct iio_chan_spec *scan_chan = &indio_dev->channels[scan_index];
> > +
> > +		adc->scan.channels[i] = get_unaligned_be24(&adc->rx_buf[scan_chan->channel * 3]);
> 
> This has me a little curious.  It seems to be potentially reading from byte 0 which in the spi
> transfer is at the same time as the tx that tells the device what the command is.  I'd expect
> it to be one byte later.  Easiest way to do that being to have two transfers (though you could
> just add to the offset). I might be misremembering how the spi_transfer stuff works though.
> Been a while since I hacked anything SPI based...
> 

Good catch.
I did not even notice that the resolution of my plot-script was wrong...


> > +		i++;
> > +	}
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
> > +			iio_get_time_ns(indio_dev));
> > +out:
> > +	mutex_unlock(&adc->lock);
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const struct iio_info mcp3911_info = {
> >  	.read_raw = mcp3911_read_raw,
> >  	.write_raw = mcp3911_write_raw,
> > @@ -214,7 +276,7 @@ static const struct iio_info mcp3911_info = {
> >  static int mcp3911_config(struct mcp3911 *adc)
> >  {
> >  	struct device *dev = &adc->spi->dev;
> > -	u32 configreg;
> > +	u32 regval;
> >  	int ret;
> >  
> >  	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
> > @@ -233,29 +295,43 @@ static int mcp3911_config(struct mcp3911 *adc)
> >  	}
> >  	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
> >  
> > -	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
> > +	ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &regval, 2);
> 
> If I was being fussy I'd ask you to pull the refactoring in here out as a precusor
> patch to simplify reviewing the new stuff a little.  However it's fairly minor
> so I'll let that go this time.

Ok, I got you wrong. Thanks.


Best regards
Marcus Folkesson


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

Re: [PATCH v4 0/9] Improve MCP3911 driver

[Marcus Folkesson]

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

On Sun, Jul 31, 2022 at 05:41:12PM +0100, Jonathan Cameron wrote:
> On Fri, 22 Jul 2022 15:07:17 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > Hi,
> > 
> > This patch series intend to fix bugs and improve functionality of the
> > MCP3911 driver.
> > The main features added are
> > - Support for buffers
> > - Interrupt driven readings
> > - Support for oversampling ratio
> > - Support for set scale values (Gain)
> > 
> > Among the bug fixes, there are changes in the formula for calculate raw
> > value and a fix for mismatch in the devicetree property.
> > 
> > Another general improvement for the driver is to use managed resources
> > for all allocated resources.
> > 
> Hi Marcus,
> 
> The first 3 fixes look good to me.  Do you want me to pick those up to
> go in after rc1 via my togreg-fixes branch?  The side effect of doing
> that is it'll be a little while before they are upstream in the branch
> I'll want to pick the rest of the series on top of.
> 
> So it's a trade off between getting fixes in as soon as possible and
> slowing down other improvements a little.

Both ways works for me.
I guess it is preferable to get the fixes in as soon as possible?

If so, do you want me to rebase the series on your togreg-fixes branch
or wait to send v5 until the patches are upstream?

Or simply keep sending the whole series?

Thanks,
Marcus Folkesson

> 
> Jonathan
> 

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

Re: [PATCH v4 0/9] Improve MCP3911 driver

[Jonathan Cameron]

On Mon, 1 Aug 2022 09:45:07 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> On Sun, Jul 31, 2022 at 05:41:12PM +0100, Jonathan Cameron wrote:
> > On Fri, 22 Jul 2022 15:07:17 +0200
> > Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> >   
> > > Hi,
> > > 
> > > This patch series intend to fix bugs and improve functionality of the
> > > MCP3911 driver.
> > > The main features added are
> > > - Support for buffers
> > > - Interrupt driven readings
> > > - Support for oversampling ratio
> > > - Support for set scale values (Gain)
> > > 
> > > Among the bug fixes, there are changes in the formula for calculate raw
> > > value and a fix for mismatch in the devicetree property.
> > > 
> > > Another general improvement for the driver is to use managed resources
> > > for all allocated resources.
> > >   
> > Hi Marcus,
> > 
> > The first 3 fixes look good to me.  Do you want me to pick those up to
> > go in after rc1 via my togreg-fixes branch?  The side effect of doing
> > that is it'll be a little while before they are upstream in the branch
> > I'll want to pick the rest of the series on top of.
> > 
> > So it's a trade off between getting fixes in as soon as possible and
> > slowing down other improvements a little.  
> 
> Both ways works for me.
> I guess it is preferable to get the fixes in as soon as possible?
> 
> If so, do you want me to rebase the series on your togreg-fixes branch
> or wait to send v5 until the patches are upstream?
> 
> Or simply keep sending the whole series?

For now resend the whole series. I don't want to pick anything up onto the fixes-togreg
branch until rc1 (as good time to get it to a more recent base).  So I'll sit on these
for another week anyway.

Thanks,


Jonathan

> 
> Thanks,
> Marcus Folkesson
> 
> > 
> > Jonathan
> >   

Re: [PATCH v4 3/9] iio: adc: mcp3911: use correct formula for AD conversion

[Jonathan Cameron]

On Fri, 22 Jul 2022 15:07:20 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> The ADC conversion is actually not rail-to-rail but with a factor 1.5.
> Make use of this factor when calculating actual voltage.
> 
> Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Patches 1-3 now applied to the fixes-togreg branch of iio.git
which is currently based on char-misc-linus which is at
Linus' tree just after he took the char-misc pull requests for
the merge window.  I might rebase this on rc1 once available
or I might just use the base as is.  Either way, it should be
fine in linux-next

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index f8875076ae80..890af7dca62d 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -40,8 +40,8 @@
>  #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
> +/* Internal voltage reference in mV */
> +#define MCP3911_INT_VREF_MV		1200
>  
>  #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)
> @@ -139,11 +139,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
>  
>  			*val = ret / 1000;
>  		} else {
> -			*val = MCP3911_INT_VREF_UV;
> +			*val = MCP3911_INT_VREF_MV;
>  		}
>  
> -		*val2 = 24;
> -		ret = IIO_VAL_FRACTIONAL_LOG2;
> +		/*
> +		 * For 24bit Conversion
> +		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
> +		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
> +		 */
> +
> +		/* val2 = (2^23 * 1.5) */
> +		*val2 = 12582912;
> +		ret = IIO_VAL_FRACTIONAL;
>  		break;
>  	}
>