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
v5:
- Drop remove function
- Split tx&rx transfers in mcp3911_trigger_handler()
- Moved Kconfig changes to right patch
Best regards,
Marcus Folkesson
The device supports negative values as well.
Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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.37.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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.37.1
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 <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
.../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.37.1
Add support for setting the Programmable Gain Amplifiers by adjust the
scale value.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 61b516573207..ed8d0bb11fd6 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -28,6 +28,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)
@@ -58,8 +60,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;
@@ -68,6 +72,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);
@@ -144,6 +149,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;
}
@@ -188,29 +198,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;
}
@@ -228,6 +218,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;
@@ -263,6 +265,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, \
@@ -272,8 +315,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, \
@@ -501,6 +546,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.37.1
The chip supports oversampling ratio, so expose it to userspace.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 ac614030e400..61b516573207 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -40,6 +40,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
@@ -58,6 +59,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;
@@ -116,6 +119,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)
@@ -144,6 +177,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) {
@@ -203,6 +245,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:
@@ -215,9 +268,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, \
@@ -279,6 +335,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.37.1
Add support for buffers to make the driver fit for more usecases.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/iio/adc/Kconfig | 2 +
drivers/iio/adc/mcp3911.c | 102 +++++++++++++++++++++++++++++++++++---
2 files changed, 97 insertions(+), 7 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 29d73b847f5f..87ab7e24ab08 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -5,16 +5,24 @@
* Copyright (C) 2018 Marcus Folkesson <[email protected]>
* Copyright (C) 2018 Kent Gustavsson <[email protected]>
*/
+#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
#define MCP3911_REG_MOD 0x06
@@ -22,6 +30,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 +63,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 __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 +212,68 @@ 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,
+ .len = 1,
+ }, {
+ .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 = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
+ ret = spi_sync_transfer(adc->spi, xfer, ARRAY_SIZE(xfer));
+ 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 +282,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 +301,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, ®val, 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, ®val, 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 +425,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.37.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 87ab7e24ab08..ac614030e400 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -30,6 +30,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)
@@ -63,6 +64,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);
@@ -352,6 +354,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;
@@ -415,6 +434,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;
@@ -425,6 +453,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.37.1
Keep using managed resources as much as possible.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/iio/adc/mcp3911.c | 56 ++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 890af7dca62d..29d73b847f5f 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,7 @@ 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;
-}
-
-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);
+ return devm_iio_device_register(&adc->spi->dev, indio_dev);
}
static const struct of_device_id mcp3911_dt_ids[] = {
@@ -367,7 +364,6 @@ static struct spi_driver mcp3911_driver = {
.of_match_table = mcp3911_dt_ids,
},
.probe = mcp3911_probe,
- .remove = mcp3911_remove,
.id_table = mcp3911_id,
};
module_spi_driver(mcp3911_driver);
--
2.37.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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.37.1
On Tue, Aug 9, 2022 at 11:39 AM Andy Shevchenko
<[email protected]> wrote:
...
> You may rather switch to devm_clk_get_enabled() and drop this.
While doing this I would recommend to split the regulator case and clock case.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 9:32 AM Marcus Folkesson
<[email protected]> wrote:
>
> Keep using managed resources as much as possible.
...
> +static void mcp3911_cleanup_clock(void *_adc)
> +{
> + struct mcp3911 *adc = _adc;
> + clk_disable_unprepare(adc->clki);
> +}
You may rather switch to devm_clk_get_enabled() and drop this.
...
> 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;
> }
As per above.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 9:32 AM Marcus Folkesson
<[email protected]> wrote:
>
> The Data Ready Output Pin is either hard wired to work as high
> impedance or push-pull. Make it configurable.
...
> + 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
Wouldn't be better to move " when data are NOT ready" to the main
description text:
Data Ready Pin Inactive State Control when data are NOT ready
In this case it unloads the list and moves focus from the part of "NOT
ready" to the real part of what you pointed out here --
"high-impedance" or "high".
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 9:32 AM Marcus Folkesson
<[email protected]> wrote:
Below comments are minor and may be addressed in case you have to
issue another version of the series (depending on what Jonathan asks).
...
> Add support for buffers to make the driver fit for more usecases.
use cases
...
> #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>
I would split this group...
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
> #include <linux/spi/spi.h>
>
...and put it here to explicitly show that the driver belongs to the
IIO subsystem.
> +#include <asm/unaligned.h>
...
> + struct spi_transfer xfer[] = {
> + {
> + .tx_buf = &adc->tx_buf,
> + .len = 1,
> + }, {
> + .rx_buf = adc->rx_buf,
> + .len = sizeof(adc->rx_buf),
> + },
> + };
> +
Redundant blank line.
> + int scan_index;
...
> + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
One line?
> + 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++;
> + }
--
With Best Regards,
Andy Shevchenko
On Tue, 9 Aug 2022 09:36:44 +0200
Marcus Folkesson <[email protected]> wrote:
> Add support for buffers to make the driver fit for more usecases.
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
One really trivial comment from me below if you are respinning anyway.
> +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,
> + .len = 1,
> + }, {
> + .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 = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
> + ret = spi_sync_transfer(adc->spi, xfer, ARRAY_SIZE(xfer));
> + 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));
iio_get_time_ns(indio_dev));
trivial: align with opening bracket unless the resulting line length is going to be very high as
a result. I could be wrong but I thought checkpatch warned on this...
> +out:
> + mutex_unlock(&adc->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
On Tue, 9 Aug 2022 09:36:39 +0200
Marcus Folkesson <[email protected]> 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.
>
Given requested changes from Andy and I were all small, I would normally
have just made them whilst applying. However, patches 1-3 still need
to work their way through as fixes to be upstream so we can't merge the
rest yet anyway.
Looks good to me in general, just those little things to tidy up in a v6.
Thanks,
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
>
> v5:
> - Drop remove function
> - Split tx&rx transfers in mcp3911_trigger_handler()
> - Moved Kconfig changes to right patch
>
>
> Best regards,
> Marcus Folkesson
>
>
On Tue, 9 Aug 2022 11:40:55 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Aug 9, 2022 at 11:39 AM Andy Shevchenko
> <[email protected]> wrote:
>
> ...
>
> > You may rather switch to devm_clk_get_enabled() and drop this.
>
> While doing this I would recommend to split the regulator case and clock case.
>
Hi Andy,
I read this and wasn't quite sure what you meant. Do you mean splitting
it into multiple patches?
If so I think that is probably taking "one thing one patch" a step too far.
I wouldn't object it if were presented as separate patches, but I'm not
sure I'd bother splitting them.
Definitely good to switch to devm_clk_get_enabled() now that's available.
There is another go at adding devm_regulator_enable() under review at the
moment, but we can tidy that up as a follow on patch (along with the 100s of
other cases in IIO!)
Jonathan
On Tue, Aug 09, 2022 at 11:57:19AM +0200, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 9:32 AM Marcus Folkesson
> <[email protected]> wrote:
> >
> > The Data Ready Output Pin is either hard wired to work as high
> > impedance or push-pull. Make it configurable.
>
> ...
>
> > + 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
>
> Wouldn't be better to move " when data are NOT ready" to the main
> description text:
>
> Data Ready Pin Inactive State Control when data are NOT ready
>
> In this case it unloads the list and moves focus from the part of "NOT
> ready" to the real part of what you pointed out here --
> "high-impedance" or "high".
I think I will put it as
Data Ready Pin Inactive State Control
true = The DR pin state is high-impedance
false = The DR pin state is logic high
As "NOT ready" is redundant is it is a "Inactive state control".
>
>
> --
> With Best Regards,
> Andy Shevchenko