[PATCH 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[PATCH 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 1cb4590fe412..4338552cd0ca 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -208,7 +208,7 @@ static int mcp3911_config(struct mcp3911 *adc)
 	u32 configreg;
 	int ret;
 
-	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
+	device_property_read_u32(dev, "microchip,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 02/10] 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>
---
 drivers/iio/adc/mcp3911.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 4338552cd0ca..3d9e8ed10874 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -306,7 +306,7 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
 	if (ret)
 		goto clk_disable;
 
@@ -326,8 +326,6 @@ 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);
-- 
2.36.1

[PATCH 03/10] 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>
---
 drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 3d9e8ed10874..768cb0203f52 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -9,6 +9,10 @@
 #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>
@@ -54,6 +58,10 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct {
+		u32 channels[2];
+		s64 ts __aligned(8);
+	} scan;
 };
 
 static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
@@ -187,16 +195,58 @@ 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,			\
+		},						\
 }
 
 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);
+	int scan_index;
+	int i = 0;
+	u32 val;
+
+	mutex_lock(&adc->lock);
+	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];
+		int ret = mcp3911_read(adc,
+				MCP3911_CHANNEL(scan_chan->channel), &val, 3);
+
+		if (ret < 0) {
+			dev_warn(&adc->spi->dev,
+					"failed to get conversion data\n");
+			goto out;
+		}
+
+		adc->scan.channels[i] = val;
+		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,
@@ -297,7 +347,7 @@ static int mcp3911_probe(struct spi_device *spi)
 		goto clk_disable;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &mcp3911_info;
 	spi_set_drvdata(spi, indio_dev);
 
@@ -306,6 +356,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)
+		goto clk_disable;
+
 	ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
 	if (ret)
 		goto clk_disable;
-- 
2.36.1

[PATCH 04/10] 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>
---
 drivers/iio/adc/mcp3911.c | 69 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 768cb0203f52..e761feed5303 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -26,6 +26,7 @@
 #define MCP3911_REG_GAIN		0x09
 
 #define MCP3911_REG_STATUSCOM		0x0a
+#define MCP3911_STATUSCOM_DRHIZ         BIT(12)
 #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
 #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
 #define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
@@ -58,6 +59,7 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct iio_trigger *trig;
 	struct {
 		u32 channels[2];
 		s64 ts __aligned(8);
@@ -252,6 +254,17 @@ static const struct iio_info mcp3911_info = {
 	.write_raw = mcp3911_write_raw,
 };
 
+static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct mcp3911 *adc = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(adc->trig);
+
+	return IRQ_HANDLED;
+};
+
 static int mcp3911_config(struct mcp3911 *adc)
 {
 	struct device *dev = &adc->spi->dev;
@@ -292,11 +305,30 @@ static int mcp3911_config(struct mcp3911 *adc)
 	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
 }
 
+
+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;
 	struct mcp3911 *adc;
 	int ret;
+	bool dr_hiz;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
 	if (!indio_dev)
@@ -346,6 +378,17 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		goto clk_disable;
 
+	dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
+	if (dr_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 < 0)
+		goto clk_disable;
+
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &mcp3911_info;
@@ -356,6 +399,32 @@ 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)
+			goto clk_disable;
+
+		adc->trig->ops = &mcp3911_trigger_ops;
+		iio_trigger_set_drvdata(adc->trig, adc);
+		ret = devm_iio_trigger_register(&spi->dev, adc->trig);
+		if (ret)
+			goto clk_disable;
+
+		/*
+		 * 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,
+				&mcp3911_interrupt,
+				IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN | IRQF_ONESHOT,
+				indio_dev->name, indio_dev);
+		if (ret)
+			goto clk_disable;
+	}
+
 	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 			NULL,
 			mcp3911_trigger_handler, NULL);
-- 
2.36.1

[PATCH 05/10] 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>
---
 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index 95ab285f4eba..74b333e44bfd 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -36,6 +36,14 @@ properties:
     description: IRQ line of the ADC
     maxItems: 1
 
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control bit
+      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
+    default: false
+
   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 06/10] iio: adc: mcp3911: add support for oversampling ratio

[Marcus Folkesson]

The chip support oversampling ratio so expose it to userspace.

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

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index e761feed5303..65831bef12f6 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -5,6 +5,8 @@
  * 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>
@@ -35,6 +37,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
@@ -53,6 +56,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;
@@ -108,6 +113,22 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
 	return mcp3911_write(adc, reg, val, len);
 }
 
+
+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)
@@ -134,6 +155,16 @@ 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) {
@@ -186,6 +217,42 @@ 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:
+		switch (val) {
+		case 4096:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
+			break;
+		case 2048:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
+			break;
+		case 1024:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
+			break;
+		case 512:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
+			break;
+		case 256:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
+			break;
+		case 128:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
+			break;
+		case 64:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
+			break;
+		case 32:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
+			break;
+		default:
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
+				MCP3911_CONFIG_OSR,
+				val, 2);
+		break;
 	}
 
 out:
@@ -198,9 +265,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.indexed = 1,					\
 		.channel = idx,					\
 		.scan_index = 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,				\
@@ -252,6 +323,7 @@ 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,
 };
 
 static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
-- 
2.36.1

[PATCH 07/10] 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.

Signed-off-by: Marcus Folkesson <marcus.folkesson@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 65831bef12f6..9e365947d285 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -48,8 +48,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)
@@ -178,11 +178,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 08/10] iio: adc: mcp3911: add support for phase

[Marcus Folkesson]

The MCP3911 incorporates a phase delay generator,
which ensures that the two ADCs are converting the
inputs with a fixed delay between them.
Expose it to userspace.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 9e365947d285..aefc5eac874c 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -155,6 +155,17 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 		ret = IIO_VAL_INT;
 		break;
+
+	case IIO_CHAN_INFO_PHASE:
+		ret = mcp3911_read(adc,
+				   MCP3911_REG_PHASE, val, 2);
+		if (ret)
+			goto out;
+
+		*val = sign_extend32(*val, 12);
+		ret = IIO_VAL_INT;
+		break;
+
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		ret = mcp3911_read(adc,
 				MCP3911_REG_CONFIG, val, 2);
@@ -225,6 +236,16 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 				MCP3911_STATUSCOM_EN_OFFCAL, 2);
 		break;
 
+	case IIO_CHAN_INFO_PHASE:
+		if (val2 != 0 || val > 0xfff) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* Write phase */
+		ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
+				    2);
+		break;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (val) {
 		case 4096:
@@ -273,7 +294,9 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.channel = idx,					\
 		.scan_index = idx,				\
 		.scan_index = idx,				\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+		.info_mask_shared_by_type =			\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)|	\
+			BIT(IIO_CHAN_INFO_PHASE),		\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
-- 
2.36.1

[PATCH 09/10] iio: adc: mcp3911: make use of the sign bit

[Marcus Folkesson]

The device supports negative values as well.

Signed-off-by: Marcus Folkesson <marcus.folkesson@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 aefc5eac874c..084f6d1aa6d1 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -144,6 +144,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 10/10] 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>
---
 drivers/iio/adc/mcp3911.c | 135 +++++++++++++++++++++++++++++++-------
 1 file changed, 110 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 084f6d1aa6d1..631015ead8e7 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -26,6 +26,8 @@
 #define MCP3911_REG_MOD			0x06
 #define MCP3911_REG_PHASE		0x07
 #define MCP3911_REG_GAIN		0x09
+#define MCP3911_GAIN_MASK(ch)          (0x7 << 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)
@@ -55,8 +57,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;
@@ -65,6 +69,7 @@ struct mcp3911 {
 	struct clk *clki;
 	u32 dev_addr;
 	struct iio_trigger *trig;
+	u32 gain[MCP3911_NUM_CHANNELS];
 	struct {
 		u32 channels[2];
 		s64 ts __aligned(8);
@@ -113,7 +118,35 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
 	return mcp3911_write(adc, reg, val, len);
 }
 
+static int mcp3911_get_gain(struct mcp3911 *adc, u8 channel, u32 *val)
+{
+	int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
+
+	if (ret)
+		return ret;
 
+	*val >>= channel * 3;
+	*val &= 0x07;
+	*val = (1 << *val);
+
+	return 0;
+}
+
+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;
+	case IIO_CHAN_INFO_PHASE:
+		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,
@@ -125,6 +158,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;
 	}
@@ -180,29 +218,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;
 	}
 
@@ -220,6 +238,19 @@ 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;
@@ -290,6 +321,49 @@ 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 = 0;
+	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);
+			goto out;
+		}
+
+		ref = ret / 1000;
+	}
+
+	/*
+	 * For 24bit 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;
+	}
+
+out:
+	return ret;
+}
+
 #define MCP3911_CHAN(idx) {					\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
@@ -302,8 +376,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 =		\
-			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.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,				\
@@ -356,6 +432,7 @@ 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 irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
@@ -482,6 +559,14 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		goto clk_disable;
 
+	ret = mcp3911_calc_scale_table(adc);
+	if (ret)
+		goto clk_disable;
+
+       /* Store gain values to better calculate scale values */
+	mcp3911_get_gain(adc, 0, &adc->gain[0]);
+	mcp3911_get_gain(adc, 1, &adc->gain[1]);
+
 	dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
 	if (dr_hiz)
 		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
-- 
2.36.1

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

[Marcus Folkesson]

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

On Thu, Jun 23, 2022 at 07:08:44PM +0200, Marcus Folkesson wrote:
> -		.info_mask_shared_by_type_available =		\
> -			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +		.info_mask_shared_by_type_available =           \
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)	\

This change was not supposed to sneak in. Removed in v2.

/Marcus



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

Re: [PATCH 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Go for the right property name that is documented in the bindings.

If the driver is already for a while in the kernel, I'm afraid we may
not do this, since it's part of ABI (firmware <--> OS). You can add a
new property and try it first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Keep using managed resources as much as possible.

You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 7:40 PM 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.

...

>  }
>
> +

Unwanted blank line.

> +static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{

> +       bool dr_hiz;

As far as I can see you don't need this variable, just call if
(device_property_...(...)).

> +       dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
> +       if (dr_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);

> +

Unneeded blank line.

> +       if (ret < 0)
> +               goto clk_disable;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 7:42 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> The MCP3911 incorporates a phase delay generator,
> which ensures that the two ADCs are converting the
> inputs with a fixed delay between them.
> Expose it to userspace.

...

> +       case IIO_CHAN_INFO_PHASE:
> +               if (val2 != 0 || val > 0xfff) {

>= BIT(12) will show better the intention

> +                       ret = -EINVAL;
> +                       goto out;
> +               }

...

> +               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
> +                                   2);

One line.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 7:41 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Add support for setting the Programmable Gain Amplifiers by adjust the
> scale value.

...

> +       int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
> +
> +       if (ret)
> +               return ret;

Please split the assignment.

  int ret;

  ret = ...
  if (ret)


...

> +       *val >>= channel * 3;
> +       *val &= 0x07;

GENMASK() ?

> +       *val = (1 << *val);

Unneeded parentheses, perhaps BIT()?

...

> +                               ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> +                                               MCP3911_GAIN_MASK(channel->channel),
> +                                               MCP3911_GAIN_VAL(channel->channel,
> +                                                       i), 1);

This is not good indentation, at least i), should be on the previous line.

...

> +static int mcp3911_calc_scale_table(struct mcp3911 *adc)
> +{
> +       u32 ref = MCP3911_INT_VREF_MV;
> +       u32 div;

> +       int ret = 0;

Useless assignment.

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

> +                       goto out;

Return directly.

> +               }
> +
> +               ref = ret / 1000;
> +       }
> +
> +       /*
> +        * For 24bit 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;
> +       }

> +out:

The useless label.

> +       return ret;

return 0;

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 9:08 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> The chip support oversampling ratio so expose it to userspace.

supports

ratio, so

...

> +static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};

Spaces inside {}.

...

>  }
>
> +

Unwanted blank line.

...

> +               switch (val) {
> +               case 4096:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
> +                       break;
> +               case 2048:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
> +                       break;
> +               case 1024:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
> +                       break;
> +               case 512:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
> +                       break;
> +               case 256:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
> +                       break;
> +               case 128:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
> +                       break;
> +               case 64:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
> +                       break;
> +               case 32:
> +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
> +                       break;
> +               default:
> +                       ret = -EINVAL;
> +                       goto out;
> +               }

I understood why the table above, but this is a waste of resources.
Use that table

...

> +               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
> +                               MCP3911_CONFIG_OSR,
> +                               val, 2);

sizeof() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Marcus Folkesson]

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

Thank you for your comments (all of them) Andy!

On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Keep using managed resources as much as possible.
> 
> You may not mix devm_ and non-devm_ API calls like this.
> So, you rule of thumb that goto is most of the time wrong after devm_ call.

Can you please confirm that clocks and regulators are disabled when the
resources are handed back?
I cannot see where when I'm trying to follow the code.

Best regards
Marcus Folkesson

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

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

[Krzysztof Kozlowski]

On 23/06/2022 19:08, Marcus Folkesson wrote:
> 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>
> ---
>  .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> index 95ab285f4eba..74b333e44bfd 100644
> --- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> @@ -36,6 +36,14 @@ properties:
>      description: IRQ line of the ADC
>      maxItems: 1
>  
> +  microchip,data-ready-hiz:
> +    description:
> +      Data Ready Pin Inactive State Control bit

"Bit" of what? Do not describe the programming model but the actual feature.

> +      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
> +    default: false

You do not need default for bools.

> +
>    microchip,device-addr:
>      description: Device address when multiple MCP3911 chips are present on the same SPI bus.
>      $ref: /schemas/types.yaml#/definitions/uint32


Best regards,
Krzysztof

Re: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts

[Marcus Folkesson]

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


On Thu, Jun 23, 2022 at 09:04:51PM +0200, Andy Shevchenko wrote:
> Unwanted blank line.
> 

Removed

> > +static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
> > +{
> 
> > +       bool dr_hiz;
> 
> As far as I can see you don't need this variable, just call if
> (device_property_...(...)).

Fixed

> 
> Unneeded blank line.

Removed

Thanks,
Marcus Folkesson

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

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

[Marcus Folkesson]

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

On Thu, Jun 23, 2022 at 09:07:21PM +0200, Andy Shevchenko wrote:
> > +               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
> > +                                   2);
> 
> One line.

Fixed

> 
> -- 
> With Best Regards,
> Andy Shevchenko


Thanks,
Marcus Folkesson

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

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

[Marcus Folkesson]

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

On Thu, Jun 23, 2022 at 09:15:51PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 7:41 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Add support for setting the Programmable Gain Amplifiers by adjust the
> > scale value.
> 
> ...
> 
> > +       int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
> > +
> > +       if (ret)
> > +               return ret;
> 
> Please split the assignment.
> 
>   int ret;
> 
>   ret = ...
>   if (ret)

OK

> 
> 
> ...
> 
> > +       *val >>= channel * 3;
> > +       *val &= 0x07;
> 
> GENMASK() ?

I will use GENMASK
> 
> > +       *val = (1 << *val);
> 
> Unneeded parentheses, perhaps BIT()?

I will switch to BIT()
> 
> ...
> 
> > +                               ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> > +                                               MCP3911_GAIN_MASK(channel->channel),
> > +                                               MCP3911_GAIN_VAL(channel->channel,
> > +                                                       i), 1);
> 
> This is not good indentation, at least i), should be on the previous line.

Agree

> 
> ...
> 
> > +static int mcp3911_calc_scale_table(struct mcp3911 *adc)
> > +{
> > +       u32 ref = MCP3911_INT_VREF_MV;
> > +       u32 div;
> 
> > +       int ret = 0;
> 
> Useless assignment.

Agree
> 
> 
> Return directly.

OK
> 
> 
> The useless label.

Agree
> 
> > +       return ret;
> 


Thanks,
Marcus Folkesson

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

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

[Marcus Folkesson]

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

On Thu, Jun 23, 2022 at 09:20:01PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 9:08 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > The chip support oversampling ratio so expose it to userspace.
> 
> supports
> 
> ratio, so
> 
> ...

Thanks
> 
> > +static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
> 
> Spaces inside {}.

Not sure what you mean?
> 
> ...
> 
> >  }
> >
> > +
> 
> Unwanted blank line.
> 
> ...

Removed
> 
> > +               switch (val) {
> > +               case 4096:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
> > +                       break;
> > +               case 2048:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
> > +                       break;
> > +               case 1024:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
> > +                       break;
> > +               case 512:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
> > +                       break;
> > +               case 256:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
> > +                       break;
> > +               case 128:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
> > +                       break;
> > +               case 64:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
> > +                       break;
> > +               case 32:
> > +                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
> > +                       break;
> > +               default:
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }
> 
> I understood why the table above, but this is a waste of resources.
> Use that table

Yep, I will use the table instead
> 
> ...
> 
> > +               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
> > +                               MCP3911_CONFIG_OSR,
> > +                               val, 2);
> 
> sizeof() ?

sizeof() what?


Thanks,
Marcus Folkesson

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

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

[Marcus Folkesson]

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

On Fri, Jun 24, 2022 at 11:54:58AM +0200, Krzysztof Kozlowski wrote:
> > +  microchip,data-ready-hiz:
> > +    description:
> > +      Data Ready Pin Inactive State Control bit
> 
> "Bit" of what? Do not describe the programming model but the actual feature.
> 

You are right. I will remove "bit".

> > +      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
> > +    default: false
> 
> You do not need default for bools.

OK
> 
> > +
> >    microchip,device-addr:
> >      description: Device address when multiple MCP3911 chips are present on the same SPI bus.
> >      $ref: /schemas/types.yaml#/definitions/uint32
> 
> 
> Best regards,
> Krzysztof

Thanks,
Marcus Folkesson



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

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

[Rob Herring]

On Thu, 23 Jun 2022 19:08:39 +0200, Marcus Folkesson wrote:
> 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>
> ---
>  .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
	Additional properties are not allowed ('default' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('default', 'type' were unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: ignoring, error in schema: properties: microchip,data-ready-hiz
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.example.dtb:0:0: /example-0/spi/adc@0: failed to match any schema with compatible: ['microchip,mcp3911']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Jonathan Cameron]

On Fri, 24 Jun 2022 08:29:20 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Thank you for your comments (all of them) Andy!
> 
> On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:  
> > >
> > > Keep using managed resources as much as possible.  
> > 
> > You may not mix devm_ and non-devm_ API calls like this.
> > So, you rule of thumb that goto is most of the time wrong after devm_ call.  
> 
> Can you please confirm that clocks and regulators are disabled when the
> resources are handed back?
> I cannot see where when I'm trying to follow the code.
Andy isn't arguing that the goto is wrong but rather that you cannot
in general safely use devm_* calls if their failure leads to having to
do any cleanup.  The reason is the ordering is hard to reason about. Sometimes
it's safe, but often enough causes problems that we basically refuse to think
hard enough to figure out if it is.  Hence basic rule is don't do it.

The issue is this.
probe() {

	non_devm_call_1();
	ret = devm_call_2()
	if (ret)
		goto err;

	return 0;
err:
	unwind_non_devm_call_1()
}

remove() {
	unwind_non_devm_call_1()
}

remove or error path should unwind in opposite order of what happens in probe.
On the rare occasion where that isn't the right choice, there should be very
clear comments to say why.

Order is

remove() -> unwind_non_devm_call_1()
devm_managed_cleanup() -> unwind_devm_call_2()

Whereas should be

remove()-> unwind_call_2() then unwind_call_1()


There are two ways to solve this.  Either only use devm for those
elements in probe() that happen before the first thing you need to
unwind manually or make everything devm managed (it unwinds in reverse
order of setup) devm_add_action_or_reset() allows you to use your
own devm_ managed callbacks if there isn't a standard one available.

Jonathan




> 
> Best regards
> Marcus Folkesson

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

[Jonathan Cameron]

On Fri, 24 Jun 2022 14:02:22 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> On Thu, Jun 23, 2022 at 09:07:21PM +0200, Andy Shevchenko wrote:
> > > +               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
> > > +                                   2);  
> > 
> > One line.  
> 
> Fixed
Hi Marcus,

Grumpy maintainer moment: If you've accepted feedback don't send yet
more email.  Just have it incorporated in next version + in changes
logs.  It's on a few seconds for people to open each email, but it
adds up over time!

Jonathan

> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko  
> 
> 
> Thanks,
> Marcus Folkesson

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 2:07 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
> On Thu, Jun 23, 2022 at 09:20:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 9:08 PM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:

...

> > > +static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
> >
> > Spaces inside {}.
>
> Not sure what you mean?

   int foo = { 1, 2, 3 };

...

> > > +               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
> > > +                               MCP3911_CONFIG_OSR,
> > > +                               val, 2);
> >
> > sizeof() ?
>
> sizeof() what?

sizeof(val) ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Andy Shevchenko]

On Sat, Jun 25, 2022 at 1:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 24 Jun 2022 08:29:20 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
>
> > Thank you for your comments (all of them) Andy!
> >
> > On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:
> > > >
> > > > Keep using managed resources as much as possible.
> > >
> > > You may not mix devm_ and non-devm_ API calls like this.
> > > So, you rule of thumb that goto is most of the time wrong after devm_ call.
> >
> > Can you please confirm that clocks and regulators are disabled when the
> > resources are handed back?
> > I cannot see where when I'm trying to follow the code.
> Andy isn't arguing that the goto is wrong but rather that you cannot
> in general safely use devm_* calls if their failure leads to having to
> do any cleanup.  The reason is the ordering is hard to reason about. Sometimes
> it's safe, but often enough causes problems that we basically refuse to think
> hard enough to figure out if it is.  Hence basic rule is don't do it.
>
> The issue is this.
> probe() {
>
>         non_devm_call_1();
>         ret = devm_call_2()
>         if (ret)
>                 goto err;
>
>         return 0;
> err:
>         unwind_non_devm_call_1()
> }
>
> remove() {
>         unwind_non_devm_call_1()
> }
>
> remove or error path should unwind in opposite order of what happens in probe.
> On the rare occasion where that isn't the right choice, there should be very
> clear comments to say why.
>
> Order is
>
> remove() -> unwind_non_devm_call_1()
> devm_managed_cleanup() -> unwind_devm_call_2()
>
> Whereas should be
>
> remove()-> unwind_call_2() then unwind_call_1()
>
>
> There are two ways to solve this.  Either only use devm for those
> elements in probe() that happen before the first thing you need to
> unwind manually or make everything devm managed (it unwinds in reverse
> order of setup) devm_add_action_or_reset() allows you to use your
> own devm_ managed callbacks if there isn't a standard one available.

Thanks, Jonathan, that's exactly what I meant!

-- 
With Best Regards,
Andy Shevchenko