2023-08-08 18:46:29

by Marcus Folkesson

[permalink] [raw]
Subject: [PATCH v4 1/4] dt-bindings: iio: adc: mcp3911: add support for the whole MCP39xx family

Microchip does have many similar chips, add those to the compatible
string as the driver support is extended.

The new supported chips are:
- microchip,mcp3910
- microchip,mcp3912
- microchip,mcp3913
- microchip,mcp3914
- microchip,mcp3918
- microchip,mcp3919

Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---

Notes:
v2:
- No changes
v3:
- No changes
v4:
- No changes

.../devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index f7b3fde4115a..06951ec5f5da 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -18,7 +18,13 @@ description: |
properties:
compatible:
enum:
+ - microchip,mcp3910
- microchip,mcp3911
+ - microchip,mcp3912
+ - microchip,mcp3913
+ - microchip,mcp3914
+ - microchip,mcp3918
+ - microchip,mcp3919

reg:
maxItems: 1
--
2.40.1



2023-08-08 18:47:53

by Marcus Folkesson

[permalink] [raw]
Subject: [PATCH v4 2/4] iio: adc: mcp3911: simplify usage of spi->dev in probe function

Replace the usage of adc->spi->dev with spi->dev to make the code prettier.

Signed-off-by: Marcus Folkesson <[email protected]>
---

Notes:
v4:
- New patch in this series

drivers/iio/adc/mcp3911.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 974c5bd923a6..8bbf2f7c839e 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -477,12 +477,12 @@ static int mcp3911_probe(struct spi_device *spi)
adc = iio_priv(indio_dev);
adc->spi = spi;

- adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
+ adc->vref = devm_regulator_get_optional(&spi->dev, "vref");
if (IS_ERR(adc->vref)) {
if (PTR_ERR(adc->vref) == -ENODEV) {
adc->vref = NULL;
} else {
- dev_err(&adc->spi->dev,
+ dev_err(&spi->dev,
"failed to get regulator (%ld)\n",
PTR_ERR(adc->vref));
return PTR_ERR(adc->vref);
@@ -499,12 +499,12 @@ static int mcp3911_probe(struct spi_device *spi)
return ret;
}

- adc->clki = devm_clk_get_enabled(&adc->spi->dev, NULL);
+ adc->clki = devm_clk_get_enabled(&spi->dev, NULL);
if (IS_ERR(adc->clki)) {
if (PTR_ERR(adc->clki) == -ENOENT) {
adc->clki = NULL;
} else {
- dev_err(&adc->spi->dev,
+ dev_err(&spi->dev,
"failed to get adc clk (%ld)\n",
PTR_ERR(adc->clki));
return PTR_ERR(adc->clki);
@@ -515,7 +515,7 @@ static int mcp3911_probe(struct spi_device *spi)
if (ret)
return ret;

- if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
+ if (device_property_read_bool(&spi->dev, "microchip,data-ready-hiz"))
ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
0, 2);
else
@@ -579,7 +579,7 @@ static int mcp3911_probe(struct spi_device *spi)
if (ret)
return ret;

- return devm_iio_device_register(&adc->spi->dev, indio_dev);
+ return devm_iio_device_register(&spi->dev, indio_dev);
}

static const struct of_device_id mcp3911_dt_ids[] = {
--
2.40.1


2023-08-08 19:05:17

by Marcus Folkesson

[permalink] [raw]
Subject: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

Microchip does have many similar chips, add support for those.

The new supported chips are:
- microchip,mcp3910
- microchip,mcp3912
- microchip,mcp3913
- microchip,mcp3914
- microchip,mcp3918
- microchip,mcp3919

Signed-off-by: Marcus Folkesson <[email protected]>
---

Notes:
v2:
- Use callbacks rather than matching against enum for determine chip variants
v3:
- Fix cosmetics
v4:
- Do not pollute output variable upon error in *_get_osr() functions.
- Fix cosmetics

drivers/iio/adc/Kconfig | 5 +-
drivers/iio/adc/mcp3911.c | 459 +++++++++++++++++++++++++++++++++-----
2 files changed, 405 insertions(+), 59 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb2b09ef5d5b..9a9838430530 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -774,8 +774,9 @@ config MCP3911
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
help
- Say yes here to build support for Microchip Technology's MCP3911
- analog to digital converter.
+ Say yes here to build support for Microchip Technology's MCP3910,
+ MCP3911, MCP3912, MCP3913, MCP3914, MCP3918 and MCP3919
+ analog to digital converters.

This driver can also be built as a module. If so, the module will be
called mcp3911.
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 8b465d2aad1a..c033e6d29650 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -61,12 +61,55 @@
#define MCP3911_REG_WRITE(reg, id) ((((reg) << 1) | ((id) << 6) | (0 << 0)) & 0xff)
#define MCP3911_REG_MASK GENMASK(4, 1)

-#define MCP3911_NUM_CHANNELS 2
#define MCP3911_NUM_SCALES 6

+/* Registers compatible with MCP3910 */
+#define MCP3910_REG_STATUSCOM 0x0c
+#define MCP3910_STATUSCOM_READ GENMASK(23, 22)
+#define MCP3910_STATUSCOM_DRHIZ BIT(20)
+
+#define MCP3910_REG_GAIN 0x0b
+
+#define MCP3910_REG_CONFIG0 0x0d
+#define MCP3910_CONFIG0_EN_OFFCAL BIT(23)
+#define MCP3910_CONFIG0_OSR GENMASK(15, 13)
+
+#define MCP3910_REG_CONFIG1 0x0e
+#define MCP3910_CONFIG1_CLKEXT BIT(6)
+#define MCP3910_CONFIG1_VREFEXT BIT(7)
+
+#define MCP3910_REG_OFFCAL_CH0 0x0f
+#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6)
+
+/* Maximal number of channels used by the MCP39XX family */
+#define MCP39XX_MAX_NUM_CHANNELS 8
+
static const int mcp3911_osr_table[] = { 32, 64, 128, 256, 512, 1024, 2048, 4096 };
static u32 mcp3911_scale_table[MCP3911_NUM_SCALES][2];

+enum mcp3911_id {
+ MCP3910,
+ MCP3911,
+ MCP3912,
+ MCP3913,
+ MCP3914,
+ MCP3918,
+ MCP3919,
+};
+
+struct mcp3911;
+struct mcp3911_chip_info {
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+
+ int (*config)(struct mcp3911 *adc);
+ int (*get_osr)(struct mcp3911 *adc, int *val);
+ int (*set_osr)(struct mcp3911 *adc, int val);
+ int (*get_offset)(struct mcp3911 *adc, int channel, int *val);
+ int (*set_offset)(struct mcp3911 *adc, int channel, int val);
+ int (*set_scale)(struct mcp3911 *adc, int channel, int val);
+};
+
struct mcp3911 {
struct spi_device *spi;
struct mutex lock;
@@ -74,14 +117,15 @@ struct mcp3911 {
struct clk *clki;
u32 dev_addr;
struct iio_trigger *trig;
- u32 gain[MCP3911_NUM_CHANNELS];
+ u32 gain[MCP39XX_MAX_NUM_CHANNELS];
+ const struct mcp3911_chip_info *chip;
struct {
- u32 channels[MCP3911_NUM_CHANNELS];
+ u32 channels[MCP39XX_MAX_NUM_CHANNELS];
s64 ts __aligned(8);
} scan;

u8 tx_buf __aligned(IIO_DMA_MINALIGN);
- u8 rx_buf[MCP3911_NUM_CHANNELS * 3];
+ u8 rx_buf[MCP39XX_MAX_NUM_CHANNELS * 3];
};

static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
@@ -126,6 +170,104 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
return mcp3911_write(adc, reg, val, len);
}

+static int mcp3910_get_offset(struct mcp3911 *adc, int channel, int *val)
+{
+ return mcp3911_read(adc, MCP3910_OFFCAL(channel), val, 3);
+}
+
+static int mcp3910_set_offset(struct mcp3911 *adc, int channel, int val)
+{
+ int ret;
+
+ /* Write offset */
+ ret = mcp3911_write(adc, MCP3910_OFFCAL(channel), val, 3);
+ if (ret)
+ return ret;
+
+ /* Enable offset*/
+ return mcp3911_update(adc, MCP3910_REG_CONFIG0,
+ MCP3910_CONFIG0_EN_OFFCAL,
+ MCP3910_CONFIG0_EN_OFFCAL, 3);
+}
+
+static int mcp3911_get_offset(struct mcp3911 *adc, int channel, int *val)
+{
+ return mcp3911_read(adc, MCP3911_OFFCAL(channel), val, 3);
+}
+
+static int mcp3911_set_offset(struct mcp3911 *adc, int channel, int val)
+{
+ int ret;
+
+ /* Write offset */
+ ret = mcp3911_write(adc, MCP3911_OFFCAL(channel), val, 3);
+ if (ret)
+ return ret;
+
+ /* Enable offset */
+ return mcp3911_update(adc, MCP3911_REG_STATUSCOM,
+ MCP3911_STATUSCOM_EN_OFFCAL,
+ MCP3911_STATUSCOM_EN_OFFCAL, 2);
+}
+
+static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
+{
+ int ret, osr;
+
+ ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3);
+ if (ret)
+ return ret;
+
+ osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
+ *val = 32 << osr;
+ return ret;
+}
+
+static int mcp3910_set_osr(struct mcp3911 *adc, int val)
+{
+ int osr;
+
+ osr = FIELD_PREP(MCP3910_CONFIG0_OSR, val);
+ return mcp3911_update(adc, MCP3910_REG_CONFIG0,
+ MCP3910_CONFIG0_OSR, osr, 3);
+}
+
+static int mcp3911_set_osr(struct mcp3911 *adc, int val)
+{
+ int osr;
+
+ osr = FIELD_PREP(MCP3911_CONFIG_OSR, val);
+ return mcp3911_update(adc, MCP3911_REG_CONFIG,
+ MCP3911_CONFIG_OSR, osr, 2);
+}
+
+static int mcp3911_get_osr(struct mcp3911 *adc, int *val)
+{
+ int ret, osr;
+
+ ret = mcp3911_read(adc, MCP3911_REG_CONFIG, val, 2);
+ if (ret)
+ return ret;
+
+ osr = FIELD_GET(MCP3911_CONFIG_OSR, *val);
+ *val = 32 << osr;
+ return ret;
+}
+
+static int mcp3910_set_scale(struct mcp3911 *adc, int channel, int val)
+{
+ return mcp3911_update(adc, MCP3910_REG_GAIN,
+ MCP3911_GAIN_MASK(channel),
+ MCP3911_GAIN_VAL(channel, val), 3);
+}
+
+static int mcp3911_set_scale(struct mcp3911 *adc, int channel, int val)
+{
+ return mcp3911_update(adc, MCP3911_REG_GAIN,
+ MCP3911_GAIN_MASK(channel),
+ MCP3911_GAIN_VAL(channel, val), 1);
+}
+
static int mcp3911_write_raw_get_fmt(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
long mask)
@@ -182,20 +324,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
break;

case IIO_CHAN_INFO_OFFSET:
- ret = mcp3911_read(adc,
- MCP3911_OFFCAL(channel->channel), val, 3);
+
+ ret = adc->chip->get_offset(adc, channel->channel, val);
if (ret)
goto out;

ret = IIO_VAL_INT;
break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- ret = mcp3911_read(adc, MCP3911_REG_CONFIG, val, 2);
+ ret = adc->chip->get_osr(adc, val);
if (ret)
goto out;

- *val = FIELD_GET(MCP3911_CONFIG_OSR, *val);
- *val = 32 << *val;
ret = IIO_VAL_INT;
break;

@@ -226,9 +366,7 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
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);
+ ret = adc->chip->set_scale(adc, channel->channel, i);
}
}
break;
@@ -238,24 +376,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
goto out;
}

- /* Write offset */
- ret = mcp3911_write(adc, MCP3911_OFFCAL(channel->channel), val,
- 3);
- if (ret)
- goto out;
-
- /* Enable offset*/
- ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM,
- MCP3911_STATUSCOM_EN_OFFCAL,
- MCP3911_STATUSCOM_EN_OFFCAL, 2);
+ ret = adc->chip->set_offset(adc, channel->channel, val);
break;

case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
for (int i = 0; i < ARRAY_SIZE(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);
+ ret = adc->chip->set_osr(adc, i);
break;
}
}
@@ -326,12 +453,60 @@ static int mcp3911_calc_scale_table(struct mcp3911 *adc)
}, \
}

+static const struct iio_chan_spec mcp3910_channels[] = {
+ MCP3911_CHAN(0),
+ MCP3911_CHAN(1),
+ IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
static const struct iio_chan_spec mcp3911_channels[] = {
MCP3911_CHAN(0),
MCP3911_CHAN(1),
IIO_CHAN_SOFT_TIMESTAMP(2),
};

+static const struct iio_chan_spec mcp3912_channels[] = {
+ MCP3911_CHAN(0),
+ MCP3911_CHAN(1),
+ MCP3911_CHAN(2),
+ MCP3911_CHAN(3),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const struct iio_chan_spec mcp3913_channels[] = {
+ MCP3911_CHAN(0),
+ MCP3911_CHAN(1),
+ MCP3911_CHAN(2),
+ MCP3911_CHAN(3),
+ MCP3911_CHAN(4),
+ MCP3911_CHAN(5),
+ IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
+static const struct iio_chan_spec mcp3914_channels[] = {
+ MCP3911_CHAN(0),
+ MCP3911_CHAN(1),
+ MCP3911_CHAN(2),
+ MCP3911_CHAN(3),
+ MCP3911_CHAN(4),
+ MCP3911_CHAN(5),
+ MCP3911_CHAN(6),
+ MCP3911_CHAN(7),
+ IIO_CHAN_SOFT_TIMESTAMP(8),
+};
+
+static const struct iio_chan_spec mcp3918_channels[] = {
+ MCP3911_CHAN(0),
+ IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct iio_chan_spec mcp3919_channels[] = {
+ MCP3911_CHAN(0),
+ MCP3911_CHAN(1),
+ MCP3911_CHAN(2),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
@@ -343,7 +518,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
.len = 1,
}, {
.rx_buf = adc->rx_buf,
- .len = sizeof(adc->rx_buf),
+ .len = (adc->chip->num_channels - 1) * 3,
},
};
int scan_index;
@@ -383,26 +558,9 @@ static const struct iio_info mcp3911_info = {

static int mcp3911_config(struct mcp3911 *adc)
{
- struct device *dev = &adc->spi->dev;
u32 regval;
int ret;

- 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",
- adc->dev_addr);
- return -EINVAL;
- }
- dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
-
ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &regval, 2);
if (ret)
return ret;
@@ -439,7 +597,101 @@ static int mcp3911_config(struct mcp3911 *adc)
regval &= ~MCP3911_STATUSCOM_READ;
regval |= FIELD_PREP(MCP3911_STATUSCOM_READ, 0x02);

- return mcp3911_write(adc, MCP3911_REG_STATUSCOM, regval, 2);
+ regval &= ~MCP3911_STATUSCOM_DRHIZ;
+ if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
+ regval |= FIELD_PREP(MCP3911_STATUSCOM_DRHIZ, 0);
+ else
+ regval |= FIELD_PREP(MCP3911_STATUSCOM_DRHIZ, 1);
+
+ /* Disable offset to ignore any old values in offset register*/
+ regval &= ~MCP3911_STATUSCOM_EN_OFFCAL;
+
+ ret = mcp3911_write(adc, MCP3911_REG_STATUSCOM, regval, 2);
+ if (ret)
+ return ret;
+
+ /* Set gain to 1 for all channels */
+ ret = mcp3911_read(adc, MCP3911_REG_GAIN, &regval, 1);
+ if (ret)
+ return ret;
+
+ for (int i = 0; i < adc->chip->num_channels - 1; i++) {
+ adc->gain[i] = 1;
+ regval &= ~MCP3911_GAIN_MASK(i);
+ }
+
+ return mcp3911_write(adc, MCP3911_REG_GAIN, regval, 1);
+}
+
+static int mcp3910_config(struct mcp3911 *adc)
+{
+ u32 regval;
+ int ret;
+
+ ret = mcp3911_read(adc, MCP3910_REG_CONFIG1, &regval, 3);
+ if (ret)
+ return ret;
+
+ regval &= ~MCP3910_CONFIG1_VREFEXT;
+ if (adc->vref) {
+ dev_dbg(&adc->spi->dev, "use external voltage reference\n");
+ regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 1);
+ } else {
+ dev_dbg(&adc->spi->dev,
+ "use internal voltage reference (1.2V)\n");
+ regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 0);
+ }
+
+ regval &= ~MCP3910_CONFIG1_CLKEXT;
+ if (adc->clki) {
+ dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
+ regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 1);
+ } else {
+ dev_dbg(&adc->spi->dev,
+ "use crystal oscillator as clocksource\n");
+ regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 0);
+ }
+
+ ret = mcp3911_write(adc, MCP3910_REG_CONFIG1, regval, 3);
+ if (ret)
+ return ret;
+
+ ret = mcp3911_read(adc, MCP3910_REG_STATUSCOM, &regval, 3);
+ if (ret)
+ return ret;
+
+ /* Address counter incremented, cycle through register types */
+ regval &= ~MCP3910_STATUSCOM_READ;
+ regval |= FIELD_PREP(MCP3910_STATUSCOM_READ, 0x02);
+
+
+ regval &= ~MCP3910_STATUSCOM_DRHIZ;
+ if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
+ regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 0);
+ else
+ regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 1);
+
+ ret = mcp3911_write(adc, MCP3910_REG_STATUSCOM, regval, 3);
+ if (ret)
+ return ret;
+
+ /* Set gain to 1 for all channels */
+ ret = mcp3911_read(adc, MCP3910_REG_GAIN, &regval, 3);
+ if (ret)
+ return ret;
+
+ for (int i = 0; i < adc->chip->num_channels - 1; i++) {
+ adc->gain[i] = 1;
+ regval &= ~MCP3911_GAIN_MASK(i);
+ }
+ ret = mcp3911_write(adc, MCP3910_REG_GAIN, regval, 3);
+ if (ret)
+ return ret;
+
+ /* Disable offset to ignore any old values in offset register */
+ return mcp3911_update(adc, MCP3910_REG_CONFIG0,
+ MCP3910_CONFIG0_EN_OFFCAL,
+ MCP3910_CONFIG0_EN_OFFCAL, 3);
}

static void mcp3911_cleanup_regulator(void *vref)
@@ -476,6 +728,7 @@ static int mcp3911_probe(struct spi_device *spi)

adc = iio_priv(indio_dev);
adc->spi = spi;
+ adc->chip = spi_get_device_match_data(spi);

adc->vref = devm_regulator_get_optional(&spi->dev, "vref");
if (IS_ERR(adc->vref)) {
@@ -511,16 +764,24 @@ static int mcp3911_probe(struct spi_device *spi)
}
}

- ret = mcp3911_config(adc);
+ ret = device_property_read_u32(&spi->dev, "microchip,device-addr", &adc->dev_addr);
+
+ /*
+ * Fallback to "device-addr" due to historical mismatch between
+ * dt-bindings and implementation.
+ */
if (ret)
- return ret;
+ device_property_read_u32(&spi->dev, "device-addr", &adc->dev_addr);
+ if (adc->dev_addr > 3) {
+ dev_err(&spi->dev,
+ "invalid device address (%i). Must be in range 0-3.\n",
+ adc->dev_addr);
+ return -EINVAL;
+ }
+ dev_dbg(&spi->dev, "use device address %i\n", adc->dev_addr);

- if (device_property_read_bool(&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);
+
+ ret = adc->chip->config(adc);
if (ret)
return ret;

@@ -529,7 +790,7 @@ static int mcp3911_probe(struct spi_device *spi)
return ret;

/* Set gain to 1 for all channels */
- for (int i = 0; i < MCP3911_NUM_CHANNELS; i++) {
+ for (int i = 0; i < adc->chip->num_channels - 1; i++) {
adc->gain[i] = 1;
ret = mcp3911_update(adc, MCP3911_REG_GAIN,
MCP3911_GAIN_MASK(i),
@@ -543,8 +804,8 @@ static int mcp3911_probe(struct spi_device *spi)
indio_dev->info = &mcp3911_info;
spi_set_drvdata(spi, indio_dev);

- indio_dev->channels = mcp3911_channels;
- indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
+ indio_dev->channels = adc->chip->channels;
+ indio_dev->num_channels = adc->chip->num_channels;

mutex_init(&adc->lock);

@@ -583,14 +844,98 @@ static int mcp3911_probe(struct spi_device *spi)
return devm_iio_device_register(&spi->dev, indio_dev);
}

+static const struct mcp3911_chip_info mcp3911_chip_info[] = {
+ [MCP3910] = {
+ .channels = mcp3910_channels,
+ .num_channels = ARRAY_SIZE(mcp3910_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+ [MCP3911] = {
+ .channels = mcp3911_channels,
+ .num_channels = ARRAY_SIZE(mcp3911_channels),
+ .config = mcp3911_config,
+ .get_osr = mcp3911_get_osr,
+ .set_osr = mcp3911_set_osr,
+ .get_offset = mcp3911_get_offset,
+ .set_offset = mcp3911_set_offset,
+ .set_scale = mcp3911_set_scale,
+ },
+ [MCP3912] = {
+ .channels = mcp3912_channels,
+ .num_channels = ARRAY_SIZE(mcp3912_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+ [MCP3913] = {
+ .channels = mcp3913_channels,
+ .num_channels = ARRAY_SIZE(mcp3913_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+ [MCP3914] = {
+ .channels = mcp3914_channels,
+ .num_channels = ARRAY_SIZE(mcp3914_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+ [MCP3918] = {
+ .channels = mcp3918_channels,
+ .num_channels = ARRAY_SIZE(mcp3918_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+ [MCP3919] = {
+ .channels = mcp3919_channels,
+ .num_channels = ARRAY_SIZE(mcp3919_channels),
+ .config = mcp3910_config,
+ .get_osr = mcp3910_get_osr,
+ .set_osr = mcp3910_set_osr,
+ .get_offset = mcp3910_get_offset,
+ .set_offset = mcp3910_set_offset,
+ .set_scale = mcp3910_set_scale,
+ },
+};
static const struct of_device_id mcp3911_dt_ids[] = {
- { .compatible = "microchip,mcp3911" },
+ { .compatible = "microchip,mcp3910", .data = &mcp3911_chip_info[MCP3910] },
+ { .compatible = "microchip,mcp3911", .data = &mcp3911_chip_info[MCP3911] },
+ { .compatible = "microchip,mcp3912", .data = &mcp3911_chip_info[MCP3912] },
+ { .compatible = "microchip,mcp3913", .data = &mcp3911_chip_info[MCP3913] },
+ { .compatible = "microchip,mcp3914", .data = &mcp3911_chip_info[MCP3914] },
+ { .compatible = "microchip,mcp3918", .data = &mcp3911_chip_info[MCP3918] },
+ { .compatible = "microchip,mcp3919", .data = &mcp3911_chip_info[MCP3919] },
{ }
};
MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);

static const struct spi_device_id mcp3911_id[] = {
- { "mcp3911", 0 },
+ { "mcp3910", (kernel_ulong_t)&mcp3911_chip_info[MCP3910] },
+ { "mcp3911", (kernel_ulong_t)&mcp3911_chip_info[MCP3911] },
+ { "mcp3912", (kernel_ulong_t)&mcp3911_chip_info[MCP3912] },
+ { "mcp3913", (kernel_ulong_t)&mcp3911_chip_info[MCP3913] },
+ { "mcp3914", (kernel_ulong_t)&mcp3911_chip_info[MCP3914] },
+ { "mcp3918", (kernel_ulong_t)&mcp3911_chip_info[MCP3918] },
+ { "mcp3919", (kernel_ulong_t)&mcp3911_chip_info[MCP3919] },
{ }
};
MODULE_DEVICE_TABLE(spi, mcp3911_id);
--
2.40.1


2023-08-08 19:05:39

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

On Tue, Aug 08, 2023 at 01:04:32PM +0200, Marcus Folkesson wrote:
> Microchip does have many similar chips, add support for those.

...

> help
> - Say yes here to build support for Microchip Technology's MCP3911
> - analog to digital converter.
> + Say yes here to build support for Microchip Technology's MCP3910,
> + MCP3911, MCP3912, MCP3913, MCP3914, MCP3918 and MCP3919
> + analog to digital converters.

For maintenance this can be written as

Say yes here to build support for one of the following
Microchip Technology's analog to digital converters:
MCP3910, MCP3911, MCP3912, MCP3913, MCP3914,
MCP3918, MCP3919

> This driver can also be built as a module. If so, the module will be
> called mcp3911.

...

> +#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6)

Inconsistent macro implementation, i.e. you need to use (x).

...

> +static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
> +{
> + int ret, osr;

Strictly speaking osr can't be negative, otherwise it's a UB below.

u32 osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
int ret;

and why val is int?

> + ret = mcp3911_read(adc, MCP3910_REG_CONFIG0, val, 3);
> + if (ret)
> + return ret;
> +
> + osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
> + *val = 32 << osr;
> + return ret;
> +}

...

> +static int mcp3910_set_osr(struct mcp3911 *adc, int val)
> +{

> + int osr;
> +
> + osr = FIELD_PREP(MCP3910_CONFIG0_OSR, val);

Can be on one line.

> + return mcp3911_update(adc, MCP3910_REG_CONFIG0,
> + MCP3910_CONFIG0_OSR, osr, 3);
> +}

...

> +static int mcp3911_set_osr(struct mcp3911 *adc, int val)
> +static int mcp3911_get_osr(struct mcp3911 *adc, int *val)

As per above comments.

...

> + if (adc->vref) {
> + dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> + regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 1);
> + } else {

> + dev_dbg(&adc->spi->dev,
> + "use internal voltage reference (1.2V)\n");


As per previous patch comments

dev_dbg(dev, "use internal voltage reference (1.2V)\n");

> + regval |= FIELD_PREP(MCP3910_CONFIG1_VREFEXT, 0);
> + }

...

> + if (adc->clki) {
> + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> + regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 1);
> + } else {
> + dev_dbg(&adc->spi->dev,
> + "use crystal oscillator as clocksource\n");

Ditto.

> + regval |= FIELD_PREP(MCP3910_CONFIG1_CLKEXT, 0);
> + }

...

> + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))

This also becomes shorter.

One trick to make it even shorter:

if (device_property_present(dev, "microchip,data-ready-hiz"))

> + regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 0);
> + else
> + regval |= FIELD_PREP(MCP3910_STATUSCOM_DRHIZ, 1);

...

> + ret = device_property_read_u32(&spi->dev, "microchip,device-addr", &adc->dev_addr);

I would move it after the comment. It will be more visible what we are
expecting and what the legacy is.


> + /*
> + * Fallback to "device-addr" due to historical mismatch between
> + * dt-bindings and implementation.
> + */

ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);

> if (ret)
> - return ret;
> + device_property_read_u32(&spi->dev, "device-addr", &adc->dev_addr);

> + if (adc->dev_addr > 3) {
> + dev_err(&spi->dev,
> + "invalid device address (%i). Must be in range 0-3.\n",
> + adc->dev_addr);
> + return -EINVAL;

return dev_err_probe(...);

> + }

...

> + dev_dbg(&spi->dev, "use device address %i\n", adc->dev_addr);

Is it useful?

--
With Best Regards,
Andy Shevchenko



2023-08-08 19:51:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] iio: adc: mcp3911: simplify usage of spi->dev in probe function

On Tue, Aug 08, 2023 at 01:04:30PM +0200, Marcus Folkesson wrote:
> Replace the usage of adc->spi->dev with spi->dev to make the code prettier.

Suggested-by: ?

> Signed-off-by: Marcus Folkesson <[email protected]>

...

> - adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
> + adc->vref = devm_regulator_get_optional(&spi->dev, "vref");

Why not

struct device *dev = &spi->dev;

and all the rest accordingly?

> if (IS_ERR(adc->vref)) {
> if (PTR_ERR(adc->vref) == -ENODEV) {
> adc->vref = NULL;
> } else {
> - dev_err(&adc->spi->dev,
> + dev_err(&spi->dev,
> "failed to get regulator (%ld)\n",
> PTR_ERR(adc->vref));
> return PTR_ERR(adc->vref);

Actually, you may first to switch to dev_err_probe() with the above introduced

struct device *dev = &spi->dev;
...
return dev_err_probe(dev, PTR_ERR(adc->vref),
"failed to get regulator\n",

and in the second patch do what you are doing here.

Will be much less changes and neater code at the end.

--
With Best Regards,
Andy Shevchenko



2023-08-08 22:51:16

by Marcus Folkesson

[permalink] [raw]
Subject: [PATCH v4 3/4] iio: adc: mcp3911: fix indentation

The file does not make use of indentation properly.
Fix that.

Signed-off-by: Marcus Folkesson <[email protected]>
---

Notes:
v4:
- New patch in this series

drivers/iio/adc/mcp3911.c | 97 ++++++++++++++++++++-------------------
1 file changed, 49 insertions(+), 48 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 8bbf2f7c839e..8b465d2aad1a 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -33,7 +33,7 @@
#define MCP3911_GAIN_VAL(ch, val) ((val << 3 * ch) & MCP3911_GAIN_MASK(ch))

#define MCP3911_REG_STATUSCOM 0x0a
-#define MCP3911_STATUSCOM_DRHIZ BIT(12)
+#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)
@@ -112,7 +112,7 @@ static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
}

static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
- u32 val, u8 len)
+ u32 val, u8 len)
{
u32 tmp;
int ret;
@@ -127,8 +127,8 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
}

static int mcp3911_write_raw_get_fmt(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- long mask)
+ struct iio_chan_spec const *chan,
+ long mask)
{
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -141,9 +141,9 @@ static int mcp3911_write_raw_get_fmt(struct iio_dev *indio_dev,
}

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)
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
{
switch (info) {
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -212,8 +212,8 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
}

static int mcp3911_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *channel, int val,
- int val2, long mask)
+ struct iio_chan_spec const *channel, int val,
+ int val2, long mask)
{
struct mcp3911 *adc = iio_priv(indio_dev);
int ret = -EINVAL;
@@ -223,12 +223,12 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
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]) {
+ 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);
+ MCP3911_GAIN_MASK(channel->channel),
+ MCP3911_GAIN_VAL(channel->channel, i), 1);
}
}
break;
@@ -246,8 +246,8 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,

/* Enable offset*/
ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM,
- MCP3911_STATUSCOM_EN_OFFCAL,
- MCP3911_STATUSCOM_EN_OFFCAL, 2);
+ MCP3911_STATUSCOM_EN_OFFCAL,
+ MCP3911_STATUSCOM_EN_OFFCAL, 2);
break;

case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -255,7 +255,7 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
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);
+ val, 2);
break;
}
}
@@ -279,7 +279,7 @@ static int mcp3911_calc_scale_table(struct mcp3911 *adc)
if (ret < 0) {
dev_err(&adc->spi->dev,
"failed to get vref voltage: %d\n",
- ret);
+ ret);
return ret;
}

@@ -305,25 +305,25 @@ static int mcp3911_calc_scale_table(struct mcp3911 *adc)
return 0;
}

-#define MCP3911_CHAN(idx) { \
- .type = IIO_VOLTAGE, \
- .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), \
- .info_mask_separate_available = \
- BIT(IIO_CHAN_INFO_SCALE), \
- .scan_type = { \
- .sign = 's', \
- .realbits = 24, \
- .storagebits = 32, \
- .endianness = IIO_BE, \
- }, \
+#define MCP3911_CHAN(idx) { \
+ .type = IIO_VOLTAGE, \
+ .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), \
+ .info_mask_separate_available = \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 24, \
+ .storagebits = 32, \
+ .endianness = IIO_BE, \
+ }, \
}

static const struct iio_chan_spec mcp3911_channels[] = {
@@ -355,7 +355,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
ret = spi_sync_transfer(adc->spi, xfer, ARRAY_SIZE(xfer));
if (ret < 0) {
dev_warn(&adc->spi->dev,
- "failed to get conversion data\n");
+ "failed to get conversion data\n");
goto out;
}

@@ -494,7 +494,7 @@ static int mcp3911_probe(struct spi_device *spi)
return ret;

ret = devm_add_action_or_reset(&spi->dev,
- mcp3911_cleanup_regulator, adc->vref);
+ mcp3911_cleanup_regulator, adc->vref);
if (ret)
return ret;
}
@@ -517,10 +517,10 @@ static int mcp3911_probe(struct spi_device *spi)

if (device_property_read_bool(&spi->dev, "microchip,data-ready-hiz"))
ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
- 0, 2);
+ 0, 2);
else
ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
- MCP3911_STATUSCOM_DRHIZ, 2);
+ MCP3911_STATUSCOM_DRHIZ, 2);
if (ret)
return ret;

@@ -528,12 +528,12 @@ static int mcp3911_probe(struct spi_device *spi)
if (ret)
return ret;

- /* Set gain to 1 for all channels */
+ /* 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);
+ MCP3911_GAIN_MASK(i),
+ MCP3911_GAIN_VAL(i, 0), 1);
if (ret)
return ret;
}
@@ -550,8 +550,8 @@ static int mcp3911_probe(struct spi_device *spi)

if (spi->irq > 0) {
adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
- indio_dev->name,
- iio_device_id(indio_dev));
+ indio_dev->name,
+ iio_device_id(indio_dev));
if (!adc->trig)
return -ENOMEM;

@@ -567,15 +567,16 @@ static int mcp3911_probe(struct spi_device *spi)
* 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);
+ &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);
+ NULL,
+ mcp3911_trigger_handler, NULL);
if (ret)
return ret;

--
2.40.1


2023-08-09 07:34:02

by Marcus Folkesson

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

> ...
>
> > +#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6)
>
> Inconsistent macro implementation, i.e. you need to use (x).

Sorry, I do not get you


[...]

> > +static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
> > +{
> > + int ret, osr;
>
> Strictly speaking osr can't be negative, otherwise it's a UB below.
>
> u32 osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
> int ret;
>
> and why val is int?

I will change val to u32 for *_get_osr(), *_set_osr() and *_set_scale().

[...]

> > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
>
> This also becomes shorter.
>
> One trick to make it even shorter:
>
> if (device_property_present(dev, "microchip,data-ready-hiz"))

Thank you, I wasn't aware of device_property_present().

[...]

>
> > + dev_dbg(&spi->dev, "use device address %i\n", adc->dev_addr);
>
> Is it useful?

Yes, I think so.

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

Best regards,
Marcus Folkesson


Attachments:
(No filename) (1.02 kB)
signature.asc (849.00 B)
Download all attachments

2023-08-09 18:38:53

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

On 09/08/2023 20:02, Jonathan Cameron wrote:
> On Wed, 9 Aug 2023 08:41:05 +0200
> Marcus Folkesson <[email protected]> wrote:
>
>>> ...
>>>
>>>> +#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6)
>>>
>>> Inconsistent macro implementation, i.e. you need to use (x).
>>
>> Sorry, I do not get you
>>
>>
>> [...]
>>
>>>> +static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
>>>> +{
>>>> + int ret, osr;
>>>
>>> Strictly speaking osr can't be negative, otherwise it's a UB below.
>>>
>>> u32 osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
>>> int ret;
>>>
>>> and why val is int?
>>
>> I will change val to u32 for *_get_osr(), *_set_osr() and *_set_scale().
>>
>> [...]
>>
>>>> + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
>>>
>>> This also becomes shorter.
>>>
>>> One trick to make it even shorter:
>>>
>>> if (device_property_present(dev, "microchip,data-ready-hiz"))
>>
>> Thank you, I wasn't aware of device_property_present().
>
> I know the read_bool function is direct equivalent of this but where a property
> is a flag, it feels more natural to me to check it with that one.
> read_present() feels more appropriate for where you want to know a more
> complex property is present.
>
> Doesn't matter that much either way however so up to you.

For the OF, of_property_read_bool() is indeed preferred. Is there
device-xxx() equivalent?

Best regards,
Krzysztof


2023-08-09 19:13:09

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

On Wed, 9 Aug 2023 08:41:05 +0200
Marcus Folkesson <[email protected]> wrote:

> > ...
> >
> > > +#define MCP3910_OFFCAL(x) (MCP3910_REG_OFFCAL_CH0 + x * 6)
> >
> > Inconsistent macro implementation, i.e. you need to use (x).
>
> Sorry, I do not get you
>
>
> [...]
>
> > > +static int mcp3910_get_osr(struct mcp3911 *adc, int *val)
> > > +{
> > > + int ret, osr;
> >
> > Strictly speaking osr can't be negative, otherwise it's a UB below.
> >
> > u32 osr = FIELD_GET(MCP3910_CONFIG0_OSR, *val);
> > int ret;
> >
> > and why val is int?
>
> I will change val to u32 for *_get_osr(), *_set_osr() and *_set_scale().
>
> [...]
>
> > > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
> >
> > This also becomes shorter.
> >
> > One trick to make it even shorter:
> >
> > if (device_property_present(dev, "microchip,data-ready-hiz"))
>
> Thank you, I wasn't aware of device_property_present().

I know the read_bool function is direct equivalent of this but where a property
is a flag, it feels more natural to me to check it with that one.
read_present() feels more appropriate for where you want to know a more
complex property is present.

Doesn't matter that much either way however so up to you.

>
> [...]
>
> >
> > > + dev_dbg(&spi->dev, "use device address %i\n", adc->dev_addr);
> >
> > Is it useful?
>
> Yes, I think so.
>
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> Best regards,
> Marcus Folkesson


2023-08-10 14:57:00

by Marcus Folkesson

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

On Thu, Aug 10, 2023 at 04:44:20PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 09, 2023 at 08:14:54PM +0200, Krzysztof Kozlowski wrote:
> > On 09/08/2023 20:02, Jonathan Cameron wrote:
> > > On Wed, 9 Aug 2023 08:41:05 +0200
> > > Marcus Folkesson <[email protected]> wrote:
>
> ...
>
> > >>> Inconsistent macro implementation, i.e. you need to use (x).
> > >>
> > >> Sorry, I do not get you
>
> In other macros you avoid ambiguity of the parameter, so they can be evaluated
> properly, and not here.


Hrmf, I missed that I had asked about this before I sent out v5. Such a shame.

I got you now, thanks.

I agree that maybe it should be 'ch' instead.
Already existing macros for channel and offcal use 'x' though, so in
that case should be changed as well.

#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)

[...]

>
> See (1) above ?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Best regards,
Marcus Folkesson


Attachments:
(No filename) (1.02 kB)
signature.asc (849.00 B)
Download all attachments

2023-08-10 15:13:17

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] iio: adc: mcp3911: add support for the whole MCP39xx family

On Wed, Aug 09, 2023 at 08:14:54PM +0200, Krzysztof Kozlowski wrote:
> On 09/08/2023 20:02, Jonathan Cameron wrote:
> > On Wed, 9 Aug 2023 08:41:05 +0200
> > Marcus Folkesson <[email protected]> wrote:

...

> >>> Inconsistent macro implementation, i.e. you need to use (x).
> >>
> >> Sorry, I do not get you

In other macros you avoid ambiguity of the parameter, so they can be evaluated
properly, and not here.


...

> >>>> + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))

(1)

> >>> This also becomes shorter.
> >>>
> >>> One trick to make it even shorter:
> >>>
> >>> if (device_property_present(dev, "microchip,data-ready-hiz"))
> >>
> >> Thank you, I wasn't aware of device_property_present().
> >
> > I know the read_bool function is direct equivalent of this but where a property
> > is a flag, it feels more natural to me to check it with that one.
> > read_present() feels more appropriate for where you want to know a more
> > complex property is present.
> >
> > Doesn't matter that much either way however so up to you.
>
> For the OF, of_property_read_bool() is indeed preferred. Is there
> device-xxx() equivalent?

See (1) above ?

--
With Best Regards,
Andy Shevchenko