MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
Signed-off-by: Marcus Folkesson <[email protected]>
Signed-off-by: Kent Gustavsson <[email protected]>
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp3911.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 455 insertions(+)
create mode 100644 drivers/iio/adc/mcp3911.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 15606f237480..f9a41fa96fcc 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -501,6 +501,16 @@ config MCP3422
This driver can also be built as a module. If so, the module will be
called mcp3422.
+config MCP3911
+ tristate "Microchip Technology MCP3911 driver"
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3911
+ analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp3911.
+
config MEDIATEK_MT6577_AUXADC
tristate "MediaTek AUXADC driver"
depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 28a9423997f3..3cfebfff7d26 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_MAX9611) += max9611.o
obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
+obj-$(CONFIG_MCP3911) += mcp3911.o
obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
new file mode 100644
index 000000000000..be74cb15827b
--- /dev/null
+++ b/drivers/iio/adc/mcp3911.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Microchip MCP3911, Two-channel Analog Front End
+ *
+ * Copyright (C) 2018 Marcus Folkesson <[email protected]>
+ * Copyright (C) 2018 Kent Gustavsson <[email protected]>
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define MCP3911_REG_CHANNEL0 0x00
+#define MCP3911_REG_CHANNEL1 0x03
+#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_CH1_24WIDTH BIT(4)
+#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
+#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
+#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1)
+
+#define MCP3911_REG_CONFIG 0x0c
+#define MCP3911_CONFIG_CLKEXT BIT(1)
+#define MCP3911_CONFIG_VREFEXT BIT(2)
+
+#define MCP3911_REG_OFFCAL_CH0 0x0e
+#define MCP3911_REG_GAINCAL_CH0 0x11
+#define MCP3911_REG_OFFCAL_CH1 0x14
+#define MCP3911_REG_GAINCAL_CH1 0x17
+#define MCP3911_REG_VREFCAL 0x1a
+
+#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
+#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
+#define MCP3911_GAINCAL(x) (MCP3911_REG_GAINCAL_CH0 + x * 6)
+
+
+/* Internal voltage reference in uV */
+#define MCP3911_INT_VREF_UV 1200000
+
+#define REG_READ(reg, id) (((reg << 1) | (id << 5) | (1 << 0)) & 0xff)
+#define REG_WRITE(reg, id) (((reg << 1) | (id << 5) | (0 << 0)) & 0xff)
+
+#define MCP3911_NUM_CHANNELS 2
+
+
+struct mcp3911 {
+ struct spi_device *spi;
+ struct device_node *np;
+ struct mutex lock;
+
+ u32 gain[MCP3911_NUM_CHANNELS];
+ u32 width[MCP3911_NUM_CHANNELS];
+
+ u32 dev_addr;
+ bool vrefext;
+ struct regulator *vref;
+};
+
+static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
+{
+ int ret;
+
+ reg = REG_READ(reg, adc->dev_addr);
+ ret = spi_write_then_read(adc->spi, ®, 1, val, len);
+ if (ret < 0)
+ return ret;
+
+ *val <<= ((4-len)*8);
+ be32_to_cpus(val);
+ dev_dbg(&adc->spi->dev, "Reading 0x%x from register 0x%x\n", *val,
+ reg>>1);
+ return ret;
+}
+
+static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
+{
+ dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
+
+ cpu_to_be32s(&val);
+ val >>= (3-len)*8;
+ val |= REG_WRITE(reg, adc->dev_addr);
+
+ return spi_write(adc->spi, &val, len+1);
+}
+
+static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
+ u32 val, u8 len)
+{
+ u32 tmp;
+ int ret;
+
+ ret = mcp3911_read(adc, reg, &tmp, len);
+ if (ret)
+ return ret;
+
+ val &= mask;
+ val |= tmp & ~mask;
+ return mcp3911_write(adc, reg, val, len);
+}
+
+static int mcp3911_get_hwgain(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_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp3911 *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(&adc->lock);
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = mcp3911_read(adc,
+ MCP3911_CHANNEL(channel->channel), val, 3);
+ if (ret)
+ goto out;
+
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_OFFSET:
+ ret = mcp3911_read(adc,
+ MCP3911_OFFCAL(channel->channel), val, 3);
+ if (ret)
+ goto out;
+
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ ret = mcp3911_get_hwgain(adc, channel->channel, val);
+ if (ret)
+ goto out;
+
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ if (adc->vrefext) {
+ 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_UV;
+ }
+
+ /* apply with gain value */
+ *val /= adc->gain[channel->channel];
+ *val2 = adc->width[channel->channel];
+
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+ }
+
+out:
+ mutex_unlock(&adc->lock);
+
+ return ret;
+}
+
+static int mcp3911_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int val,
+ int val2, long mask)
+{
+ struct mcp3911 *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(&adc->lock);
+ switch (mask) {
+ case IIO_CHAN_INFO_OFFSET:
+
+ /* 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);
+ if (ret)
+ goto out;
+
+ break;
+
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ if (!is_power_of_2(val) && val <= 32) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ adc->gain[channel->channel] = val;
+
+ val = ilog2(val);
+ ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+ MCP3911_GAIN_MASK(channel->channel),
+ MCP3911_GAIN_VAL(channel->channel,
+ val), 1);
+ break;
+ }
+
+out:
+ mutex_unlock(&adc->lock);
+
+ return ret;
+}
+
+static const struct iio_chan_spec mcp3911_channels[] = {
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 0,
+ .address = MCP3911_REG_CHANNEL0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_OFFSET) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+ },
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 1,
+ .address = MCP3911_REG_CHANNEL1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_OFFSET) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+ },
+};
+
+static const struct iio_info mcp3911_info = {
+ .read_raw = mcp3911_read_raw,
+ .write_raw = mcp3911_write_raw,
+};
+
+static int mcp3911_config_of(struct mcp3911 *adc)
+{
+ u32 configreg;
+ u32 statuscomreg;
+ int ret;
+
+ of_property_read_u32(adc->np, "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, &configreg, 2);
+ if (ret)
+ return ret;
+
+ adc->vrefext = of_property_read_bool(adc->np, "external-vref");
+ if (adc->vrefext) {
+ dev_dbg(&adc->spi->dev, "use external voltage reference\n");
+ configreg |= MCP3911_CONFIG_VREFEXT;
+
+ } else {
+ dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
+ configreg &= ~MCP3911_CONFIG_VREFEXT;
+ }
+
+ if (of_property_read_bool(adc->np, "external-clock")) {
+ dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
+ configreg |= MCP3911_CONFIG_CLKEXT;
+ } else {
+ dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
+ configreg &= ~MCP3911_CONFIG_CLKEXT;
+ }
+
+ ret = mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
+ if (ret)
+ return ret;
+
+
+ ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &statuscomreg, 2);
+ if (ret)
+ return ret;
+
+
+ of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
+ switch (adc->width[0]) {
+ case 24:
+ statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
+ dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
+ break;
+ case 16:
+ statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
+ dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
+ break;
+ default:
+ adc->width[0] = 24;
+ dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
+ break;
+ }
+
+ of_property_read_u32(adc->np, "ch1-width", &adc->width[1]);
+ switch (adc->width[1]) {
+ case 24:
+ statuscomreg &= ~MCP3911_STATUSCOM_CH1_24WIDTH;
+ dev_dbg(&adc->spi->dev, "set channel 1 into 24bit mode\n");
+ break;
+ case 16:
+ statuscomreg |= MCP3911_STATUSCOM_CH1_24WIDTH;
+ dev_dbg(&adc->spi->dev, "set channel 1 into 16bit mode\n");
+ break;
+ default:
+ adc->width[1] = 24;
+ dev_info(&adc->spi->dev, "invalid width for channel 1. Use 24bit.\n");
+ break;
+ }
+
+ return mcp3911_write(adc, MCP3911_REG_STATUSCOM, statuscomreg, 2);
+}
+
+static int mcp3911_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct mcp3911 *adc;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ adc = iio_priv(indio_dev);
+ adc->spi = spi;
+ adc->np = spi->dev.of_node;
+
+ ret = mcp3911_config_of(adc);
+ if (ret)
+ return ret;
+
+ if (adc->vrefext) {
+ adc->vref = devm_regulator_get(&adc->spi->dev, "vref");
+ if (IS_ERR(adc->vref))
+ return PTR_ERR(adc->vref);
+
+ ret = regulator_enable(adc->vref);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Store gain values to better calculate scale values */
+ mcp3911_get_hwgain(adc, 0, &adc->gain[0]);
+ mcp3911_get_hwgain(adc, 1, &adc->gain[1]);
+
+ indio_dev->dev.parent = &spi->dev;
+ indio_dev->dev.of_node = spi->dev.of_node;
+ indio_dev->name = spi_get_device_id(spi)->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &mcp3911_info;
+ spi_set_drvdata(spi, indio_dev);
+
+ indio_dev->channels = mcp3911_channels;
+ indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
+
+ mutex_init(&adc->lock);
+
+ ret = iio_device_register(indio_dev);
+ if (ret)
+ goto reg_disable;
+
+ return ret;
+
+reg_disable:
+ if (adc->vref)
+ regulator_disable(adc->vref);
+
+ return ret;
+}
+
+static int 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);
+
+ if (adc->vref)
+ regulator_disable(adc->vref);
+
+ return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id mcp3911_dt_ids[] = {
+ { .compatible = "microchip,mcp3911" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
+#endif
+
+static const struct spi_device_id mcp3911_id[] = {
+ { "mcp3911", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, mcp3911_id);
+
+static struct spi_driver mcp3911_driver = {
+ .driver = {
+ .name = "mcp3911",
+ .of_match_table = of_match_ptr(mcp3911_dt_ids),
+ },
+ .probe = mcp3911_probe,
+ .remove = mcp3911_remove,
+ .id_table = mcp3911_id,
+};
+module_spi_driver(mcp3911_driver);
+
+MODULE_AUTHOR("Marcus Folkesson <[email protected]>");
+MODULE_AUTHOR("Kent Gustavsson <[email protected]>");
+MODULE_DESCRIPTION("Microchip Technology MCP3911");
+MODULE_LICENSE("GPL v2");
--
2.11.0.rc2
Add an entry for mcp3911 ADC driver and add myself and
Kent Gustavsson as maintainers of this driver.
Signed-off-by: Marcus Folkesson <[email protected]>
Signed-off-by: Kent Gustavsson <[email protected]>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 79bb02ff812f..9276da915d9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9271,6 +9271,14 @@ L: [email protected]
S: Maintained
F: drivers/net/ethernet/microchip/lan743x_*
+MICROCHIP / ATMEL MCP3911 ADC DRIVER
+M: Marcus Folkesson <[email protected]>
+M: Kent Gustavsson <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/iio/adc/mcp3911.c
+F: Documentation/devicetree/bindings/iio/adc/mcp3911.txt
+
MICROCHIP USB251XB DRIVER
M: Richard Leitner <[email protected]>
L: [email protected]
--
2.11.0.rc2
MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
Signed-off-by: Marcus Folkesson <[email protected]>
Signed-off-by: Kent Gustavsson <[email protected]>
---
.../devicetree/bindings/iio/adc/mcp3911.txt | 33 ++++++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
new file mode 100644
index 000000000000..e233ee94ad96
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
@@ -0,0 +1,33 @@
+* Microchip MCP3911 Dual channel analog front end (ADC)
+
+Required properties:
+ - compatible: Should be "microchip,mcp3911"
+ - reg: SPI chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+ Documentation/devicetree/bindings/spi/spi-bus.txt.
+ Max frequency for this chip is 20MHz.
+
+Optional properties:
+ - device-addr: Device address when multiple MCP3911 chips are present on the
+ same SPI bus. Valid values are 0-3. Defaults to 0.
+ - external-clock: Use external clock instead of crystal oscillator.
+ - external-vref: Use external voltage reference
+ - vref-supply: Phandle to the external reference voltage supply. (only valid in combination with `external-vref`)
+ - ch0-width: width for channel0. Valid widths are 16 and 24bits.
+ - ch1-width: width for channel1. Valid widths are 16 and 24bits.
+
+
+Example:
+adc@0 {
+ compatible = "microchip,mcp3911";
+ reg = <0>;
+ spi-max-frequency = <20000000>;
+ device-addr = <0>;
+ ch0-width = <16>;
+ ch1-width = <24>;
+ external-clock;
+ external-vref;
+ vref-supply = <&vref_reg>;
+};
--
2.11.0.rc2
Hello,
> MCP3911 is a dual channel Analog Front End (AFE) containing two
> synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
some comments below...
> Signed-off-by: Marcus Folkesson <[email protected]>
> Signed-off-by: Kent Gustavsson <[email protected]>
> ---
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/mcp3911.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 455 insertions(+)
> create mode 100644 drivers/iio/adc/mcp3911.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 15606f237480..f9a41fa96fcc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -501,6 +501,16 @@ config MCP3422
> This driver can also be built as a module. If so, the module will be
> called mcp3422.
>
> +config MCP3911
> + tristate "Microchip Technology MCP3911 driver"
> + depends on SPI
> + help
> + Say yes here to build support for Microchip Technology's MCP3911
> + analog to digital converter.
> +
> + This driver can also be built as a module. If so, the module will be
> + called mcp3911.
> +
> config MEDIATEK_MT6577_AUXADC
> tristate "MediaTek AUXADC driver"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 28a9423997f3..3cfebfff7d26 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX9611) += max9611.o
> obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> +obj-$(CONFIG_MCP3911) += mcp3911.o
> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> new file mode 100644
> index 000000000000..be74cb15827b
> --- /dev/null
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Microchip MCP3911, Two-channel Analog Front End
> + *
> + * Copyright (C) 2018 Marcus Folkesson <[email protected]>
> + * Copyright (C) 2018 Kent Gustavsson <[email protected]>
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#define MCP3911_REG_CHANNEL0 0x00
> +#define MCP3911_REG_CHANNEL1 0x03
> +#define MCP3911_REG_MOD 0x06
> +#define MCP3911_REG_PHASE 0x07
> +
> +#define MCP3911_REG_GAIN 0x09
> +#define MCP3911_GAIN_MASK(ch) (0x7 << 3*ch)
space around * operator, maybe parenthesis around variable, i.e
(0x07 << (3 * (ch)))
> +#define MCP3911_GAIN_VAL(ch, val) ((val << 3*ch) & MCP3911_GAIN_MASK(ch))
> +
> +#define MCP3911_REG_STATUSCOM 0x0a
> +#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)
> +#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
> +#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
> +#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1)
> +
> +#define MCP3911_REG_CONFIG 0x0c
> +#define MCP3911_CONFIG_CLKEXT BIT(1)
> +#define MCP3911_CONFIG_VREFEXT BIT(2)
> +
> +#define MCP3911_REG_OFFCAL_CH0 0x0e
> +#define MCP3911_REG_GAINCAL_CH0 0x11
> +#define MCP3911_REG_OFFCAL_CH1 0x14
> +#define MCP3911_REG_GAINCAL_CH1 0x17
> +#define MCP3911_REG_VREFCAL 0x1a
> +
> +#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
> +#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
> +#define MCP3911_GAINCAL(x) (MCP3911_REG_GAINCAL_CH0 + x * 6)
> +
delete newline
> +
> +/* Internal voltage reference in uV */
> +#define MCP3911_INT_VREF_UV 1200000
> +
> +#define REG_READ(reg, id) (((reg << 1) | (id << 5) | (1 << 0)) & 0xff)
> +#define REG_WRITE(reg, id) (((reg << 1) | (id << 5) | (0 << 0)) & 0xff)
MCP3911_ prefix please
parenthesis around variables
> +
> +#define MCP3911_NUM_CHANNELS 2
> +
> +
> +struct mcp3911 {
> + struct spi_device *spi;
> + struct device_node *np;
> + struct mutex lock;
> +
> + u32 gain[MCP3911_NUM_CHANNELS];
> + u32 width[MCP3911_NUM_CHANNELS];
> +
> + u32 dev_addr;
> + bool vrefext;
> + struct regulator *vref;
> +};
> +
> +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> +{
> + int ret;
> +
> + reg = REG_READ(reg, adc->dev_addr);
> + ret = spi_write_then_read(adc->spi, ®, 1, val, len);
> + if (ret < 0)
> + return ret;
> +
> + *val <<= ((4-len)*8);
space around - and * operator, here and elsewhere
shouldn't the endiness conversion happen before the value is shifted?
(here and below)?
> + be32_to_cpus(val);
> + dev_dbg(&adc->spi->dev, "Reading 0x%x from register 0x%x\n", *val,
> + reg>>1);
> + return ret;
> +}
> +
> +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> +{
> + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> +
> + cpu_to_be32s(&val);
> + val >>= (3-len)*8;
> + val |= REG_WRITE(reg, adc->dev_addr);
> +
> + return spi_write(adc->spi, &val, len+1);
> +}
> +
> +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> + u32 val, u8 len)
> +{
> + u32 tmp;
> + int ret;
> +
> + ret = mcp3911_read(adc, reg, &tmp, len);
> + if (ret)
> + return ret;
> +
> + val &= mask;
> + val |= tmp & ~mask;
> + return mcp3911_write(adc, reg, val, len);
> +}
> +
> +static int mcp3911_get_hwgain(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_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct mcp3911 *adc = iio_priv(indio_dev);
> + int ret = -EINVAL;
> +
> + mutex_lock(&adc->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = mcp3911_read(adc,
> + MCP3911_CHANNEL(channel->channel), val, 3);
> + if (ret)
> + goto out;
> +
> + ret = IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_OFFSET:
> + ret = mcp3911_read(adc,
> + MCP3911_OFFCAL(channel->channel), val, 3);
> + if (ret)
> + goto out;
> +
> + ret = IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + ret = mcp3911_get_hwgain(adc, channel->channel, val);
> + if (ret)
> + goto out;
> +
> + ret = IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_SCALE:
> + if (adc->vrefext) {
> + ret = regulator_get_voltage(adc->vref);
> + if (ret < 0) {
> + dev_err(indio_dev->dev.parent,
> + "failed to get vref voltage:%d\n", ret);
start message consistently with upper/lowercase
maybe space before :
> + goto out;
> + }
> +
> + *val = ret / 1000;
> + } else {
> + *val = MCP3911_INT_VREF_UV;
> + }
> +
> + /* apply with gain value */
> + *val /= adc->gain[channel->channel];
> + *val2 = adc->width[channel->channel];
> +
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + break;
> + }
> +
> +out:
> + mutex_unlock(&adc->lock);
> +
> + return ret;
> +}
> +
> +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int val,
> + int val2, long mask)
> +{
> + struct mcp3911 *adc = iio_priv(indio_dev);
> + int ret = -EINVAL;
> +
> + mutex_lock(&adc->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_OFFSET:
> +
val2 should probably be zero and checked?
> + /* 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);
> + if (ret)
> + goto out;
> +
> + break;
> +
> + case IIO_CHAN_INFO_HARDWAREGAIN:
val2?
> + if (!is_power_of_2(val) && val <= 32) {
the check looks suspicious, maybe || val > 32
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + adc->gain[channel->channel] = val;
> +
> + val = ilog2(val);
> + ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> + MCP3911_GAIN_MASK(channel->channel),
> + MCP3911_GAIN_VAL(channel->channel,
> + val), 1);
> + break;
> + }
> +
> +out:
> + mutex_unlock(&adc->lock);
> +
> + return ret;
> +}
> +
> +static const struct iio_chan_spec mcp3911_channels[] = {
> + {
maybe use a MACRO(), e.g. MCP3911_CHANNEL(idx) ...
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .channel = 0,
> + .address = MCP3911_REG_CHANNEL0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .channel = 1,
> + .address = MCP3911_REG_CHANNEL1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + },
> +};
> +
> +static const struct iio_info mcp3911_info = {
> + .read_raw = mcp3911_read_raw,
> + .write_raw = mcp3911_write_raw,
> +};
> +
> +static int mcp3911_config_of(struct mcp3911 *adc)
> +{
> + u32 configreg;
> + u32 statuscomreg;
> + int ret;
> +
> + of_property_read_u32(adc->np, "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, &configreg, 2);
> + if (ret)
> + return ret;
> +
> + adc->vrefext = of_property_read_bool(adc->np, "external-vref");
> + if (adc->vrefext) {
> + dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> + configreg |= MCP3911_CONFIG_VREFEXT;
> +
> + } else {
> + dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> + configreg &= ~MCP3911_CONFIG_VREFEXT;
> + }
> +
> + if (of_property_read_bool(adc->np, "external-clock")) {
> + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> + configreg |= MCP3911_CONFIG_CLKEXT;
> + } else {
> + dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> + configreg &= ~MCP3911_CONFIG_CLKEXT;
> + }
> +
> + ret = mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> + if (ret)
> + return ret;
> +
> +
> + ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &statuscomreg, 2);
> + if (ret)
> + return ret;
> +
no duplicate newlines (here and elsewhere)
> +
> + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> + switch (adc->width[0]) {
> + case 24:
> + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> + break;
> + case 16:
> + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> + break;
> + default:
> + adc->width[0] = 24;
> + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> + break;
> + }
> +
> + of_property_read_u32(adc->np, "ch1-width", &adc->width[1]);
> + switch (adc->width[1]) {
> + case 24:
> + statuscomreg &= ~MCP3911_STATUSCOM_CH1_24WIDTH;
> + dev_dbg(&adc->spi->dev, "set channel 1 into 24bit mode\n");
> + break;
> + case 16:
> + statuscomreg |= MCP3911_STATUSCOM_CH1_24WIDTH;
> + dev_dbg(&adc->spi->dev, "set channel 1 into 16bit mode\n");
> + break;
> + default:
> + adc->width[1] = 24;
> + dev_info(&adc->spi->dev, "invalid width for channel 1. Use 24bit.\n");
> + break;
> + }
> +
> + return mcp3911_write(adc, MCP3911_REG_STATUSCOM, statuscomreg, 2);
> +}
> +
> +static int mcp3911_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct mcp3911 *adc;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc = iio_priv(indio_dev);
> + adc->spi = spi;
> + adc->np = spi->dev.of_node;
> +
> + ret = mcp3911_config_of(adc);
> + if (ret)
> + return ret;
> +
> + if (adc->vrefext) {
> + adc->vref = devm_regulator_get(&adc->spi->dev, "vref");
> + if (IS_ERR(adc->vref))
> + return PTR_ERR(adc->vref);
> +
> + ret = regulator_enable(adc->vref);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* Store gain values to better calculate scale values */
> + mcp3911_get_hwgain(adc, 0, &adc->gain[0]);
> + mcp3911_get_hwgain(adc, 1, &adc->gain[1]);
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->dev.of_node = spi->dev.of_node;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &mcp3911_info;
> + spi_set_drvdata(spi, indio_dev);
> +
> + indio_dev->channels = mcp3911_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> +
> + mutex_init(&adc->lock);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto reg_disable;
> +
> + return ret;
> +
> +reg_disable:
> + if (adc->vref)
> + regulator_disable(adc->vref);
> +
> + return ret;
> +}
> +
> +static int 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);
> +
> + if (adc->vref)
> + regulator_disable(adc->vref);
> +
> + return 0;
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mcp3911_dt_ids[] = {
> + { .compatible = "microchip,mcp3911" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> +#endif
> +
> +static const struct spi_device_id mcp3911_id[] = {
> + { "mcp3911", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> +
> +static struct spi_driver mcp3911_driver = {
> + .driver = {
> + .name = "mcp3911",
> + .of_match_table = of_match_ptr(mcp3911_dt_ids),
> + },
> + .probe = mcp3911_probe,
> + .remove = mcp3911_remove,
> + .id_table = mcp3911_id,
> +};
> +module_spi_driver(mcp3911_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <[email protected]>");
> +MODULE_AUTHOR("Kent Gustavsson <[email protected]>");
> +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> +MODULE_LICENSE("GPL v2");
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
On Sat, 21 Jul 2018 23:19:48 +0200 (CEST)
Peter Meerwald-Stadler <[email protected]> wrote:
> Hello,
>
> > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
>
> some comments below...
+CC Mark for the unusual SPI addressing stuff. I'm mostly interested in what
precedent there is for bindings etc.
>
> > Signed-off-by: Marcus Folkesson <[email protected]>
> > Signed-off-by: Kent Gustavsson <[email protected]>
> > ---
> > drivers/iio/adc/Kconfig | 10 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/mcp3911.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 455 insertions(+)
> > create mode 100644 drivers/iio/adc/mcp3911.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 15606f237480..f9a41fa96fcc 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -501,6 +501,16 @@ config MCP3422
> > This driver can also be built as a module. If so, the module will be
> > called mcp3422.
> >
> > +config MCP3911
> > + tristate "Microchip Technology MCP3911 driver"
> > + depends on SPI
> > + help
> > + Say yes here to build support for Microchip Technology's MCP3911
> > + analog to digital converter.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called mcp3911.
> > +
> > config MEDIATEK_MT6577_AUXADC
> > tristate "MediaTek AUXADC driver"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 28a9423997f3..3cfebfff7d26 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> > obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > +obj-$(CONFIG_MCP3911) += mcp3911.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > new file mode 100644
> > index 000000000000..be74cb15827b
> > --- /dev/null
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -0,0 +1,444 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Microchip MCP3911, Two-channel Analog Front End
> > + *
> > + * Copyright (C) 2018 Marcus Folkesson <[email protected]>
> > + * Copyright (C) 2018 Kent Gustavsson <[email protected]>
> > + *
No need for blank line here.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define MCP3911_REG_CHANNEL0 0x00
> > +#define MCP3911_REG_CHANNEL1 0x03
> > +#define MCP3911_REG_MOD 0x06
> > +#define MCP3911_REG_PHASE 0x07
> > +
> > +#define MCP3911_REG_GAIN 0x09
> > +#define MCP3911_GAIN_MASK(ch) (0x7 << 3*ch)
>
> space around * operator, maybe parenthesis around variable, i.e
> (0x07 << (3 * (ch)))
>
> > +#define MCP3911_GAIN_VAL(ch, val) ((val << 3*ch) & MCP3911_GAIN_MASK(ch))
> > +
> > +#define MCP3911_REG_STATUSCOM 0x0a
> > +#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)
> > +#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
> > +#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
> > +#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1)
> > +
> > +#define MCP3911_REG_CONFIG 0x0c
> > +#define MCP3911_CONFIG_CLKEXT BIT(1)
> > +#define MCP3911_CONFIG_VREFEXT BIT(2)
> > +
> > +#define MCP3911_REG_OFFCAL_CH0 0x0e
> > +#define MCP3911_REG_GAINCAL_CH0 0x11
> > +#define MCP3911_REG_OFFCAL_CH1 0x14
> > +#define MCP3911_REG_GAINCAL_CH1 0x17
> > +#define MCP3911_REG_VREFCAL 0x1a
> > +
> > +#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
> > +#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
> > +#define MCP3911_GAINCAL(x) (MCP3911_REG_GAINCAL_CH0 + x * 6)
> > +
>
> delete newline
>
> > +
> > +/* Internal voltage reference in uV */
> > +#define MCP3911_INT_VREF_UV 1200000
> > +
> > +#define REG_READ(reg, id) (((reg << 1) | (id << 5) | (1 << 0)) & 0xff)
> > +#define REG_WRITE(reg, id) (((reg << 1) | (id << 5) | (0 << 0)) & 0xff)
>
> MCP3911_ prefix please
> parenthesis around variables
>
> > +
> > +#define MCP3911_NUM_CHANNELS 2
> > +
> > +
> > +struct mcp3911 {
> > + struct spi_device *spi;
> > + struct device_node *np;
> > + struct mutex lock;
> > +
> > + u32 gain[MCP3911_NUM_CHANNELS];
> > + u32 width[MCP3911_NUM_CHANNELS];
> > +
> > + u32 dev_addr;
> > + bool vrefext;
> > + struct regulator *vref;
> > +};
> > +
> > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > +{
> > + int ret;
> > +
> > + reg = REG_READ(reg, adc->dev_addr);
> > + ret = spi_write_then_read(adc->spi, ®, 1, val, len);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val <<= ((4-len)*8);
>
> space around - and * operator, here and elsewhere
>
> shouldn't the endiness conversion happen before the value is shifted?
> (here and below)?
>
> > + be32_to_cpus(val);
> > + dev_dbg(&adc->spi->dev, "Reading 0x%x from register 0x%x\n", *val,
> > + reg>>1);
> > + return ret;
> > +}
> > +
> > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > +{
> > + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> > +
> > + cpu_to_be32s(&val);
> > + val >>= (3-len)*8;
Hmm. It might be worth considering regmap here to handle all this stuff for
you rather than re rolling the same stuff.
> > + val |= REG_WRITE(reg, adc->dev_addr);
> > +
> > + return spi_write(adc->spi, &val, len+1);
> > +}
> > +
> > +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> > + u32 val, u8 len)
> > +{
> > + u32 tmp;
> > + int ret;
> > +
> > + ret = mcp3911_read(adc, reg, &tmp, len);
> > + if (ret)
> > + return ret;
> > +
> > + val &= mask;
> > + val |= tmp & ~mask;
> > + return mcp3911_write(adc, reg, val, len);
> > +}
> > +
> > +static int mcp3911_get_hwgain(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_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct mcp3911 *adc = iio_priv(indio_dev);
> > + int ret = -EINVAL;
> > +
> > + mutex_lock(&adc->lock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = mcp3911_read(adc,
> > + MCP3911_CHANNEL(channel->channel), val, 3);
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_OFFSET:
> > + ret = mcp3911_read(adc,
> > + MCP3911_OFFCAL(channel->channel), val, 3);
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > + ret = mcp3911_get_hwgain(adc, channel->channel, val);
I'm not convinced it's useful to expose this as it right control for this
is scale.
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + if (adc->vrefext) {
> > + ret = regulator_get_voltage(adc->vref);
> > + if (ret < 0) {
> > + dev_err(indio_dev->dev.parent,
> > + "failed to get vref voltage:%d\n", ret);
>
> start message consistently with upper/lowercase
> maybe space before :
>
> > + goto out;
> > + }
> > +
> > + *val = ret / 1000;
> > + } else {
> > + *val = MCP3911_INT_VREF_UV;
> > + }
> > +
> > + /* apply with gain value */
> > + *val /= adc->gain[channel->channel];
> > + *val2 = adc->width[channel->channel];
> > +
> > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > + break;
> > + }
> > +
> > +out:
> > + mutex_unlock(&adc->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int val,
> > + int val2, long mask)
> > +{
> > + struct mcp3911 *adc = iio_priv(indio_dev);
> > + int ret = -EINVAL;
> > +
> > + mutex_lock(&adc->lock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_OFFSET:
> > +
>
> val2 should probably be zero and checked?
>
> > + /* 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);
> > + if (ret)
> > + goto out;
We go there anyway so why bother with the goto?
> > +
> > + break;
> > +
> > + case IIO_CHAN_INFO_HARDWAREGAIN:
Default choice (by precedent) is to control variable gain
front ends via the scale parameter. Hardware gain
is not meant to have any 'visible' impact on the output
value - most commonly used when the thing we are measuring
is not amplitude of anything.
>
> val2?
>
> > + if (!is_power_of_2(val) && val <= 32) {
>
> the check looks suspicious, maybe || val > 32
>
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + adc->gain[channel->channel] = val;
> > +
> > + val = ilog2(val);
> > + ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> > + MCP3911_GAIN_MASK(channel->channel),
> > + MCP3911_GAIN_VAL(channel->channel,
> > + val), 1);
> > + break;
> > + }
> > +
> > +out:
> > + mutex_unlock(&adc->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_chan_spec mcp3911_channels[] = {
> > + {
>
> maybe use a MACRO(), e.g. MCP3911_CHANNEL(idx) ...
>
> > + .type = IIO_VOLTAGE,
> > + .indexed = 1,
> > + .channel = 0,
> > + .address = MCP3911_REG_CHANNEL0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_OFFSET) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .indexed = 1,
> > + .channel = 1,
> > + .address = MCP3911_REG_CHANNEL1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_OFFSET) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > + },
> > +};
> > +
> > +static const struct iio_info mcp3911_info = {
> > + .read_raw = mcp3911_read_raw,
> > + .write_raw = mcp3911_write_raw,
> > +};
> > +
> > +static int mcp3911_config_of(struct mcp3911 *adc)
> > +{
> > + u32 configreg;
> > + u32 statuscomreg;
> > + int ret;
> > +
> > + of_property_read_u32(adc->np, "device-addr", &adc->dev_addr);
This is 'interesting' - I wonder if there is any precedence for it.
Mark,
> > + 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, &configreg, 2);
> > + if (ret)
> > + return ret;
> > +
> > + adc->vrefext = of_property_read_bool(adc->np, "external-vref");
Why not just use the presence or lack of a regulator being supplied to indicate
this? Use the regulator_get_optional functionality to avoid getting a stub
regulator if you need to know there isn't one there to connect.
Here you need the _optional form.
> > + if (adc->vrefext) {
> > + dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> > + configreg |= MCP3911_CONFIG_VREFEXT;
> > +
> > + } else {
> > + dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> > + configreg &= ~MCP3911_CONFIG_VREFEXT;
> > + }
> > +
> > + if (of_property_read_bool(adc->np, "external-clock")) {
> > + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> > + configreg |= MCP3911_CONFIG_CLKEXT;
> > + } else {
> > + dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> > + configreg &= ~MCP3911_CONFIG_CLKEXT;
> > + }
Sort of feels like this should be handled a bit like the regulator.
The kernel has bindings and software support for clocks. Would be nice
to use them.
> > +
> > + ret = mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > + if (ret)
> > + return ret;
> > +
> > +
> > + ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &statuscomreg, 2);
> > + if (ret)
> > + return ret;
> > +
>
> no duplicate newlines (here and elsewhere)
>
> > +
> > + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> > + switch (adc->width[0]) {
> > + case 24:
> > + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> > + break;
> > + case 16:
> > + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> > + break;
> > + default:
> > + adc->width[0] = 24;
> > + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> > + break;
> > + }
This feels like something that isn't really a dt choice, as it's not down to
wiring but rather down to precision desired.
Now variable resolution isn't something IIO has traditionally dealt
with very well - particularly as it causes problems when we add in
buffered interfaces (as it changes the data layout etc).
> > +
> > + of_property_read_u32(adc->np, "ch1-width", &adc->width[1]);
> > + switch (adc->width[1]) {
> > + case 24:
> > + statuscomreg &= ~MCP3911_STATUSCOM_CH1_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 1 into 24bit mode\n");
> > + break;
> > + case 16:
> > + statuscomreg |= MCP3911_STATUSCOM_CH1_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 1 into 16bit mode\n");
> > + break;
> > + default:
> > + adc->width[1] = 24;
> > + dev_info(&adc->spi->dev, "invalid width for channel 1. Use 24bit.\n");
> > + break;
> > + }
> > +
> > + return mcp3911_write(adc, MCP3911_REG_STATUSCOM, statuscomreg, 2);
> > +}
> > +
> > +static int mcp3911_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct mcp3911 *adc;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + adc = iio_priv(indio_dev);
> > + adc->spi = spi;
> > + adc->np = spi->dev.of_node;
np is is rather unusual naming. Why not of_node?
Also, why do we need to keep a copy of this?
> > +
> > + ret = mcp3911_config_of(adc);
> > + if (ret)
> > + return ret;
> > +
> > + if (adc->vrefext) {
> > + adc->vref = devm_regulator_get(&adc->spi->dev, "vref");
As I mention above, use devm_regulator_get_optional if you need
to be able to do something different dependent on whether the regulator
is actually there, if not just use devm_regulator_get and without
any if (adc->vrefext) and you'll get a 'fake' regulator which you can
enable and disable without it doing anything.
> > + if (IS_ERR(adc->vref))
> > + return PTR_ERR(adc->vref);
> > +
> > + ret = regulator_enable(adc->vref);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + /* Store gain values to better calculate scale values */
> > + mcp3911_get_hwgain(adc, 0, &adc->gain[0]);
> > + mcp3911_get_hwgain(adc, 1, &adc->gain[1]);
> > +
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->dev.of_node = spi->dev.of_node;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &mcp3911_info;
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + indio_dev->channels = mcp3911_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> > +
> > + mutex_init(&adc->lock);
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret)
> > + goto reg_disable;
> > +
> > + return ret;
> > +
> > +reg_disable:
> > + if (adc->vref)
> > + regulator_disable(adc->vref);
> > +
> > + return ret;
> > +}
> > +
> > +static int 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);
> > +
> > + if (adc->vref)
> > + regulator_disable(adc->vref);
> > +
> > + return 0;
> > +}
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mcp3911_dt_ids[] = {
> > + { .compatible = "microchip,mcp3911" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> > +#endif
> > +
> > +static const struct spi_device_id mcp3911_id[] = {
> > + { "mcp3911", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> > +
> > +static struct spi_driver mcp3911_driver = {
> > + .driver = {
> > + .name = "mcp3911",
> > + .of_match_table = of_match_ptr(mcp3911_dt_ids),
> > + },
> > + .probe = mcp3911_probe,
> > + .remove = mcp3911_remove,
> > + .id_table = mcp3911_id,
> > +};
> > +module_spi_driver(mcp3911_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <[email protected]>");
> > +MODULE_AUTHOR("Kent Gustavsson <[email protected]>");
> > +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> > +MODULE_LICENSE("GPL v2");
> >
>
On Sat, 21 Jul 2018 21:59:22 +0200
Marcus Folkesson <[email protected]> wrote:
> MCP3911 is a dual channel Analog Front End (AFE) containing two
> synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> Signed-off-by: Kent Gustavsson <[email protected]>
> ---
> .../devicetree/bindings/iio/adc/mcp3911.txt | 33 ++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> new file mode 100644
> index 000000000000..e233ee94ad96
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> @@ -0,0 +1,33 @@
> +* Microchip MCP3911 Dual channel analog front end (ADC)
> +
> +Required properties:
> + - compatible: Should be "microchip,mcp3911"
> + - reg: SPI chip select number for the device
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> + Documentation/devicetree/bindings/spi/spi-bus.txt.
> + Max frequency for this chip is 20MHz.
> +
> +Optional properties:
> + - device-addr: Device address when multiple MCP3911 chips are present on the
> + same SPI bus. Valid values are 0-3. Defaults to 0.
> + - external-clock: Use external clock instead of crystal oscillator.
As mentioned, in the code, can we use the standard fixed clock bindings here.
We don't actually care what the value is, but it might be nice to be able to
power down the clock if we are suspending or something..
> + - external-vref: Use external voltage reference
> + - vref-supply: Phandle to the external reference voltage supply. (only valid in combination with `external-vref`)
Just use the optional regulator stuff and get rid of the bool.
> + - ch0-width: width for channel0. Valid widths are 16 and 24bits.
> + - ch1-width: width for channel1. Valid widths are 16 and 24bits.
As I asked in the code, are these a function of the wiring etc or are
they something we should really be leaving up to userspace (with a sensible
default).
> +
> +
> +Example:
> +adc@0 {
> + compatible = "microchip,mcp3911";
> + reg = <0>;
> + spi-max-frequency = <20000000>;
> + device-addr = <0>;
> + ch0-width = <16>;
> + ch1-width = <24>;
> + external-clock;
> + external-vref;
> + vref-supply = <&vref_reg>;
> +};
Hi Peter,
Thanks for the comments!
On Sat, Jul 21, 2018 at 11:19:48PM +0200, Peter Meerwald-Stadler wrote:
> Hello,
>
> > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
>
> some comments below...
>
> > Signed-off-by: Marcus Folkesson <[email protected]>
> > Signed-off-by: Kent Gustavsson <[email protected]>
> > ---
> > drivers/iio/adc/Kconfig | 10 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/mcp3911.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 455 insertions(+)
> > create mode 100644 drivers/iio/adc/mcp3911.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 15606f237480..f9a41fa96fcc 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -501,6 +501,16 @@ config MCP3422
> > This driver can also be built as a module. If so, the module will be
> > called mcp3422.
> >
> > +config MCP3911
> > + tristate "Microchip Technology MCP3911 driver"
> > + depends on SPI
> > + help
> > + Say yes here to build support for Microchip Technology's MCP3911
> > + analog to digital converter.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called mcp3911.
> > +
> > config MEDIATEK_MT6577_AUXADC
> > tristate "MediaTek AUXADC driver"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 28a9423997f3..3cfebfff7d26 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> > obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > +obj-$(CONFIG_MCP3911) += mcp3911.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > new file mode 100644
> > index 000000000000..be74cb15827b
> > --- /dev/null
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -0,0 +1,444 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Microchip MCP3911, Two-channel Analog Front End
> > + *
> > + * Copyright (C) 2018 Marcus Folkesson <[email protected]>
> > + * Copyright (C) 2018 Kent Gustavsson <[email protected]>
> > + *
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define MCP3911_REG_CHANNEL0 0x00
> > +#define MCP3911_REG_CHANNEL1 0x03
> > +#define MCP3911_REG_MOD 0x06
> > +#define MCP3911_REG_PHASE 0x07
> > +
> > +#define MCP3911_REG_GAIN 0x09
> > +#define MCP3911_GAIN_MASK(ch) (0x7 << 3*ch)
>
> space around * operator, maybe parenthesis around variable, i.e
> (0x07 << (3 * (ch)))
>
Will do
> > +#define MCP3911_GAIN_VAL(ch, val) ((val << 3*ch) & MCP3911_GAIN_MASK(ch))
> > +
> > +#define MCP3911_REG_STATUSCOM 0x0a
> > +#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)
> > +#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
> > +#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
> > +#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1)
> > +
> > +#define MCP3911_REG_CONFIG 0x0c
> > +#define MCP3911_CONFIG_CLKEXT BIT(1)
> > +#define MCP3911_CONFIG_VREFEXT BIT(2)
> > +
> > +#define MCP3911_REG_OFFCAL_CH0 0x0e
> > +#define MCP3911_REG_GAINCAL_CH0 0x11
> > +#define MCP3911_REG_OFFCAL_CH1 0x14
> > +#define MCP3911_REG_GAINCAL_CH1 0x17
> > +#define MCP3911_REG_VREFCAL 0x1a
> > +
> > +#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
> > +#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
> > +#define MCP3911_GAINCAL(x) (MCP3911_REG_GAINCAL_CH0 + x * 6)
> > +
>
> delete newline
>
Will do
> > +
> > +/* Internal voltage reference in uV */
> > +#define MCP3911_INT_VREF_UV 1200000
> > +
> > +#define REG_READ(reg, id) (((reg << 1) | (id << 5) | (1 << 0)) & 0xff)
> > +#define REG_WRITE(reg, id) (((reg << 1) | (id << 5) | (0 << 0)) & 0xff)
>
> MCP3911_ prefix please
> parenthesis around variables
Will do
>
> > +
> > +#define MCP3911_NUM_CHANNELS 2
> > +
> > +
> > +struct mcp3911 {
> > + struct spi_device *spi;
> > + struct device_node *np;
> > + struct mutex lock;
> > +
> > + u32 gain[MCP3911_NUM_CHANNELS];
> > + u32 width[MCP3911_NUM_CHANNELS];
> > +
> > + u32 dev_addr;
> > + bool vrefext;
> > + struct regulator *vref;
> > +};
> > +
> > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > +{
> > + int ret;
> > +
> > + reg = REG_READ(reg, adc->dev_addr);
> > + ret = spi_write_then_read(adc->spi, ®, 1, val, len);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val <<= ((4-len)*8);
>
> space around - and * operator, here and elsewhere
Will do
>
> shouldn't the endiness conversion happen before the value is shifted?
> (here and below)?
>
You are right
> > + be32_to_cpus(val);
> > + dev_dbg(&adc->spi->dev, "Reading 0x%x from register 0x%x\n", *val,
> > + reg>>1);
> > + return ret;
> > +}
> > +
> > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > +{
> > + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> > +
> > + cpu_to_be32s(&val);
> > + val >>= (3-len)*8;
> > + val |= REG_WRITE(reg, adc->dev_addr);
> > +
> > + return spi_write(adc->spi, &val, len+1);
> > +}
> > +
> > +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> > + u32 val, u8 len)
> > +{
> > + u32 tmp;
> > + int ret;
> > +
> > + ret = mcp3911_read(adc, reg, &tmp, len);
> > + if (ret)
> > + return ret;
> > +
> > + val &= mask;
> > + val |= tmp & ~mask;
> > + return mcp3911_write(adc, reg, val, len);
> > +}
> > +
> > +static int mcp3911_get_hwgain(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_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct mcp3911 *adc = iio_priv(indio_dev);
> > + int ret = -EINVAL;
> > +
> > + mutex_lock(&adc->lock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = mcp3911_read(adc,
> > + MCP3911_CHANNEL(channel->channel), val, 3);
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_OFFSET:
> > + ret = mcp3911_read(adc,
> > + MCP3911_OFFCAL(channel->channel), val, 3);
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > + ret = mcp3911_get_hwgain(adc, channel->channel, val);
> > + if (ret)
> > + goto out;
> > +
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + if (adc->vrefext) {
> > + ret = regulator_get_voltage(adc->vref);
> > + if (ret < 0) {
> > + dev_err(indio_dev->dev.parent,
> > + "failed to get vref voltage:%d\n", ret);
>
> start message consistently with upper/lowercase
> maybe space before :
>
I will consistently use lowercase, thanks.
> > + goto out;
> > + }
> > +
> > + *val = ret / 1000;
> > + } else {
> > + *val = MCP3911_INT_VREF_UV;
> > + }
> > +
> > + /* apply with gain value */
> > + *val /= adc->gain[channel->channel];
> > + *val2 = adc->width[channel->channel];
> > +
> > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > + break;
> > + }
> > +
> > +out:
> > + mutex_unlock(&adc->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int val,
> > + int val2, long mask)
> > +{
> > + struct mcp3911 *adc = iio_priv(indio_dev);
> > + int ret = -EINVAL;
> > +
> > + mutex_lock(&adc->lock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_OFFSET:
> > +
>
> val2 should probably be zero and checked?
>
Will do
> > + /* 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);
> > + if (ret)
> > + goto out;
> > +
> > + break;
> > +
> > + case IIO_CHAN_INFO_HARDWAREGAIN:
>
> val2?
Will do
>
> > + if (!is_power_of_2(val) && val <= 32) {
>
> the check looks suspicious, maybe || val > 32
Yep, much better :-)
>
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + adc->gain[channel->channel] = val;
> > +
> > + val = ilog2(val);
> > + ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> > + MCP3911_GAIN_MASK(channel->channel),
> > + MCP3911_GAIN_VAL(channel->channel,
> > + val), 1);
> > + break;
> > + }
> > +
> > +out:
> > + mutex_unlock(&adc->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_chan_spec mcp3911_channels[] = {
> > + {
>
> maybe use a MACRO(), e.g. MCP3911_CHANNEL(idx) ...
I will create a macro for this, thanks
>
> > + .type = IIO_VOLTAGE,
> > + .indexed = 1,
> > + .channel = 0,
> > + .address = MCP3911_REG_CHANNEL0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_OFFSET) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .indexed = 1,
> > + .channel = 1,
> > + .address = MCP3911_REG_CHANNEL1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_OFFSET) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > + },
> > +};
> > +
> > +static const struct iio_info mcp3911_info = {
> > + .read_raw = mcp3911_read_raw,
> > + .write_raw = mcp3911_write_raw,
> > +};
> > +
> > +static int mcp3911_config_of(struct mcp3911 *adc)
> > +{
> > + u32 configreg;
> > + u32 statuscomreg;
> > + int ret;
> > +
> > + of_property_read_u32(adc->np, "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, &configreg, 2);
> > + if (ret)
> > + return ret;
> > +
> > + adc->vrefext = of_property_read_bool(adc->np, "external-vref");
> > + if (adc->vrefext) {
> > + dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> > + configreg |= MCP3911_CONFIG_VREFEXT;
> > +
> > + } else {
> > + dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> > + configreg &= ~MCP3911_CONFIG_VREFEXT;
> > + }
> > +
> > + if (of_property_read_bool(adc->np, "external-clock")) {
> > + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> > + configreg |= MCP3911_CONFIG_CLKEXT;
> > + } else {
> > + dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> > + configreg &= ~MCP3911_CONFIG_CLKEXT;
> > + }
> > +
> > + ret = mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > + if (ret)
> > + return ret;
> > +
> > +
> > + ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &statuscomreg, 2);
> > + if (ret)
> > + return ret;
> > +
>
> no duplicate newlines (here and elsewhere)
>
Ok
> > +
> > + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> > + switch (adc->width[0]) {
> > + case 24:
> > + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> > + break;
> > + case 16:
> > + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> > + break;
> > + default:
> > + adc->width[0] = 24;
> > + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> > + break;
> > + }
> > +
> > + of_property_read_u32(adc->np, "ch1-width", &adc->width[1]);
> > + switch (adc->width[1]) {
> > + case 24:
> > + statuscomreg &= ~MCP3911_STATUSCOM_CH1_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 1 into 24bit mode\n");
> > + break;
> > + case 16:
> > + statuscomreg |= MCP3911_STATUSCOM_CH1_24WIDTH;
> > + dev_dbg(&adc->spi->dev, "set channel 1 into 16bit mode\n");
> > + break;
> > + default:
> > + adc->width[1] = 24;
> > + dev_info(&adc->spi->dev, "invalid width for channel 1. Use 24bit.\n");
> > + break;
> > + }
> > +
> > + return mcp3911_write(adc, MCP3911_REG_STATUSCOM, statuscomreg, 2);
> > +}
> > +
> > +static int mcp3911_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct mcp3911 *adc;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + adc = iio_priv(indio_dev);
> > + adc->spi = spi;
> > + adc->np = spi->dev.of_node;
> > +
> > + ret = mcp3911_config_of(adc);
> > + if (ret)
> > + return ret;
> > +
> > + if (adc->vrefext) {
> > + adc->vref = devm_regulator_get(&adc->spi->dev, "vref");
> > + if (IS_ERR(adc->vref))
> > + return PTR_ERR(adc->vref);
> > +
> > + ret = regulator_enable(adc->vref);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + /* Store gain values to better calculate scale values */
> > + mcp3911_get_hwgain(adc, 0, &adc->gain[0]);
> > + mcp3911_get_hwgain(adc, 1, &adc->gain[1]);
> > +
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->dev.of_node = spi->dev.of_node;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &mcp3911_info;
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + indio_dev->channels = mcp3911_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> > +
> > + mutex_init(&adc->lock);
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret)
> > + goto reg_disable;
> > +
> > + return ret;
> > +
> > +reg_disable:
> > + if (adc->vref)
> > + regulator_disable(adc->vref);
> > +
> > + return ret;
> > +}
> > +
> > +static int 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);
> > +
> > + if (adc->vref)
> > + regulator_disable(adc->vref);
> > +
> > + return 0;
> > +}
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mcp3911_dt_ids[] = {
> > + { .compatible = "microchip,mcp3911" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> > +#endif
> > +
> > +static const struct spi_device_id mcp3911_id[] = {
> > + { "mcp3911", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> > +
> > +static struct spi_driver mcp3911_driver = {
> > + .driver = {
> > + .name = "mcp3911",
> > + .of_match_table = of_match_ptr(mcp3911_dt_ids),
> > + },
> > + .probe = mcp3911_probe,
> > + .remove = mcp3911_remove,
> > + .id_table = mcp3911_id,
> > +};
> > +module_spi_driver(mcp3911_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <[email protected]>");
> > +MODULE_AUTHOR("Kent Gustavsson <[email protected]>");
> > +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> > +MODULE_LICENSE("GPL v2");
> >
>
> --
>
> Peter Meerwald-Stadler
> Mobile: +43 664 24 44 418
Thanks,
Marcus Folkesson
Hi Jonathan,
Thanks, all good catches.
On Sun, Jul 22, 2018 at 09:08:38AM +0100, Jonathan Cameron wrote:
> On Sat, 21 Jul 2018 23:19:48 +0200 (CEST)
> Peter Meerwald-Stadler <[email protected]> wrote:
>
> > Hello,
> >
> > > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> >
> > some comments below...
>
> +CC Mark for the unusual SPI addressing stuff. I'm mostly interested in what
> precedent there is for bindings etc.
>
Yep, I'm not entirely sure that the SPI framework can handle multiple
clients on the same CS.
The reason why we created device-addr is that the chip supports that and
may have hardcoded chip address from factory.
The chip address is also part of the protocol so we have to specify it.
> >
> > > Signed-off-by: Marcus Folkesson <[email protected]>
> > > Signed-off-by: Kent Gustavsson <[email protected]>
> > > ---
> > > drivers/iio/adc/Kconfig | 10 ++
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/mcp3911.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 455 insertions(+)
> > > create mode 100644 drivers/iio/adc/mcp3911.c
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 15606f237480..f9a41fa96fcc 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -501,6 +501,16 @@ config MCP3422
> > > This driver can also be built as a module. If so, the module will be
> > > called mcp3422.
> > >
> > > +config MCP3911
> > > + tristate "Microchip Technology MCP3911 driver"
> > > + depends on SPI
> > > + help
> > > + Say yes here to build support for Microchip Technology's MCP3911
> > > + analog to digital converter.
> > > +
> > > + This driver can also be built as a module. If so, the module will be
> > > + called mcp3911.
> > > +
> > > config MEDIATEK_MT6577_AUXADC
> > > tristate "MediaTek AUXADC driver"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 28a9423997f3..3cfebfff7d26 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> > > obj-$(CONFIG_MAX9611) += max9611.o
> > > obj-$(CONFIG_MCP320X) += mcp320x.o
> > > obj-$(CONFIG_MCP3422) += mcp3422.o
> > > +obj-$(CONFIG_MCP3911) += mcp3911.o
> > > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
> > > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
> > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > > new file mode 100644
> > > index 000000000000..be74cb15827b
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/mcp3911.c
> > > @@ -0,0 +1,444 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for Microchip MCP3911, Two-channel Analog Front End
> > > + *
> > > + * Copyright (C) 2018 Marcus Folkesson <[email protected]>
> > > + * Copyright (C) 2018 Kent Gustavsson <[email protected]>
> > > + *
>
> No need for blank line here.
>
Ok
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#define MCP3911_REG_CHANNEL0 0x00
> > > +#define MCP3911_REG_CHANNEL1 0x03
> > > +#define MCP3911_REG_MOD 0x06
> > > +#define MCP3911_REG_PHASE 0x07
> > > +
> > > +#define MCP3911_REG_GAIN 0x09
> > > +#define MCP3911_GAIN_MASK(ch) (0x7 << 3*ch)
> >
> > space around * operator, maybe parenthesis around variable, i.e
> > (0x07 << (3 * (ch)))
> >
> > > +#define MCP3911_GAIN_VAL(ch, val) ((val << 3*ch) & MCP3911_GAIN_MASK(ch))
> > > +
> > > +#define MCP3911_REG_STATUSCOM 0x0a
> > > +#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)
> > > +#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
> > > +#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
> > > +#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1)
> > > +
> > > +#define MCP3911_REG_CONFIG 0x0c
> > > +#define MCP3911_CONFIG_CLKEXT BIT(1)
> > > +#define MCP3911_CONFIG_VREFEXT BIT(2)
> > > +
> > > +#define MCP3911_REG_OFFCAL_CH0 0x0e
> > > +#define MCP3911_REG_GAINCAL_CH0 0x11
> > > +#define MCP3911_REG_OFFCAL_CH1 0x14
> > > +#define MCP3911_REG_GAINCAL_CH1 0x17
> > > +#define MCP3911_REG_VREFCAL 0x1a
> > > +
> > > +#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
> > > +#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
> > > +#define MCP3911_GAINCAL(x) (MCP3911_REG_GAINCAL_CH0 + x * 6)
> > > +
> >
> > delete newline
> >
> > > +
> > > +/* Internal voltage reference in uV */
> > > +#define MCP3911_INT_VREF_UV 1200000
> > > +
> > > +#define REG_READ(reg, id) (((reg << 1) | (id << 5) | (1 << 0)) & 0xff)
> > > +#define REG_WRITE(reg, id) (((reg << 1) | (id << 5) | (0 << 0)) & 0xff)
> >
> > MCP3911_ prefix please
> > parenthesis around variables
> >
> > > +
> > > +#define MCP3911_NUM_CHANNELS 2
> > > +
> > > +
> > > +struct mcp3911 {
> > > + struct spi_device *spi;
> > > + struct device_node *np;
> > > + struct mutex lock;
> > > +
> > > + u32 gain[MCP3911_NUM_CHANNELS];
> > > + u32 width[MCP3911_NUM_CHANNELS];
> > > +
> > > + u32 dev_addr;
> > > + bool vrefext;
> > > + struct regulator *vref;
> > > +};
> > > +
> > > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > > +{
> > > + int ret;
> > > +
> > > + reg = REG_READ(reg, adc->dev_addr);
> > > + ret = spi_write_then_read(adc->spi, ®, 1, val, len);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val <<= ((4-len)*8);
> >
> > space around - and * operator, here and elsewhere
> >
> > shouldn't the endiness conversion happen before the value is shifted?
> > (here and below)?
> >
> > > + be32_to_cpus(val);
> > > + dev_dbg(&adc->spi->dev, "Reading 0x%x from register 0x%x\n", *val,
> > > + reg>>1);
> > > + return ret;
> > > +}
> > > +
> > > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > > +{
> > > + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> > > +
> > > + cpu_to_be32s(&val);
> > > + val >>= (3-len)*8;
> Hmm. It might be worth considering regmap here to handle all this stuff for
> you rather than re rolling the same stuff.
>
We were looking at regmap, but it does not seems to support registers of
different size.
This chip has register values of 8, 16 and 24 bits.
> > > + val |= REG_WRITE(reg, adc->dev_addr);
> > > +
> > > + return spi_write(adc->spi, &val, len+1);
> > > +}
> > > +
> > > +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
> > > + u32 val, u8 len)
> > > +{
> > > + u32 tmp;
> > > + int ret;
> > > +
> > > + ret = mcp3911_read(adc, reg, &tmp, len);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + val &= mask;
> > > + val |= tmp & ~mask;
> > > + return mcp3911_write(adc, reg, val, len);
> > > +}
> > > +
> > > +static int mcp3911_get_hwgain(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_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *channel, int *val,
> > > + int *val2, long mask)
> > > +{
> > > + struct mcp3911 *adc = iio_priv(indio_dev);
> > > + int ret = -EINVAL;
> > > +
> > > + mutex_lock(&adc->lock);
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + ret = mcp3911_read(adc,
> > > + MCP3911_CHANNEL(channel->channel), val, 3);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > +
> > > + case IIO_CHAN_INFO_OFFSET:
> > > + ret = mcp3911_read(adc,
> > > + MCP3911_OFFCAL(channel->channel), val, 3);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > +
> > > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > > + ret = mcp3911_get_hwgain(adc, channel->channel, val);
>
> I'm not convinced it's useful to expose this as it right control for this
> is scale.
>
Hmm, all other drivers that are using HARDWAREGAIN (ina2xx-adc, stx104 +
a few more that are not ADC:s) are, what I can tell, exposing it.
But maybe it should'nt.
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > +
> > > + case IIO_CHAN_INFO_SCALE:
> > > + if (adc->vrefext) {
> > > + ret = regulator_get_voltage(adc->vref);
> > > + if (ret < 0) {
> > > + dev_err(indio_dev->dev.parent,
> > > + "failed to get vref voltage:%d\n", ret);
> >
> > start message consistently with upper/lowercase
> > maybe space before :
> >
> > > + goto out;
> > > + }
> > > +
> > > + *val = ret / 1000;
> > > + } else {
> > > + *val = MCP3911_INT_VREF_UV;
> > > + }
> > > +
> > > + /* apply with gain value */
> > > + *val /= adc->gain[channel->channel];
> > > + *val2 = adc->width[channel->channel];
> > > +
> > > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > > + break;
> > > + }
> > > +
> > > +out:
> > > + mutex_unlock(&adc->lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int mcp3911_write_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *channel, int val,
> > > + int val2, long mask)
> > > +{
> > > + struct mcp3911 *adc = iio_priv(indio_dev);
> > > + int ret = -EINVAL;
> > > +
> > > + mutex_lock(&adc->lock);
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_OFFSET:
> > > +
> >
> > val2 should probably be zero and checked?
> >
> > > + /* 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);
> > > + if (ret)
> > > + goto out;
>
> We go there anyway so why bother with the goto?
>
Yep, the goto will be removed.
> > > +
> > > + break;
> > > +
> > > + case IIO_CHAN_INFO_HARDWAREGAIN:
>
> Default choice (by precedent) is to control variable gain
> front ends via the scale parameter. Hardware gain
> is not meant to have any 'visible' impact on the output
> value - most commonly used when the thing we are measuring
> is not amplitude of anything.
Hmm, Ok. I'm not sure I understand how hardware gain is supposed to work
then.
Maybe I just remove it.
>
> >
> > val2?
> >
> > > + if (!is_power_of_2(val) && val <= 32) {
> >
> > the check looks suspicious, maybe || val > 32
> >
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + adc->gain[channel->channel] = val;
> > > +
> > > + val = ilog2(val);
> > > + ret = mcp3911_update(adc, MCP3911_REG_GAIN,
> > > + MCP3911_GAIN_MASK(channel->channel),
> > > + MCP3911_GAIN_VAL(channel->channel,
> > > + val), 1);
> > > + break;
> > > + }
> > > +
> > > +out:
> > > + mutex_unlock(&adc->lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static const struct iio_chan_spec mcp3911_channels[] = {
> > > + {
> >
> > maybe use a MACRO(), e.g. MCP3911_CHANNEL(idx) ...
> >
> > > + .type = IIO_VOLTAGE,
> > > + .indexed = 1,
> > > + .channel = 0,
> > > + .address = MCP3911_REG_CHANNEL0,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_OFFSET) |
> > > + BIT(IIO_CHAN_INFO_SCALE) |
> > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > > + },
> > > + {
> > > + .type = IIO_VOLTAGE,
> > > + .indexed = 1,
> > > + .channel = 1,
> > > + .address = MCP3911_REG_CHANNEL1,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_OFFSET) |
> > > + BIT(IIO_CHAN_INFO_SCALE) |
> > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> > > + },
> > > +};
> > > +
> > > +static const struct iio_info mcp3911_info = {
> > > + .read_raw = mcp3911_read_raw,
> > > + .write_raw = mcp3911_write_raw,
> > > +};
> > > +
> > > +static int mcp3911_config_of(struct mcp3911 *adc)
> > > +{
> > > + u32 configreg;
> > > + u32 statuscomreg;
> > > + int ret;
> > > +
> > > + of_property_read_u32(adc->np, "device-addr", &adc->dev_addr);
> This is 'interesting' - I wonder if there is any precedence for it.
>
I guess we still need it since the device may have a hardcoded (from
factory) address that we need to deal with.
> Mark,
> > > + 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, &configreg, 2);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + adc->vrefext = of_property_read_bool(adc->np, "external-vref");
>
> Why not just use the presence or lack of a regulator being supplied to indicate
> this? Use the regulator_get_optional functionality to avoid getting a stub
> regulator if you need to know there isn't one there to connect.
> Here you need the _optional form.
Ah!
I wanted it to work that way, but got the dummy regulator when no
regulator was provided.
So _optional is the thing.
Thanks!
>
> > > + if (adc->vrefext) {
> > > + dev_dbg(&adc->spi->dev, "use external voltage reference\n");
> > > + configreg |= MCP3911_CONFIG_VREFEXT;
> > > +
> > > + } else {
> > > + dev_dbg(&adc->spi->dev, "use internal voltage reference (1.2V)\n");
> > > + configreg &= ~MCP3911_CONFIG_VREFEXT;
> > > + }
> > > +
> > > + if (of_property_read_bool(adc->np, "external-clock")) {
> > > + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n");
> > > + configreg |= MCP3911_CONFIG_CLKEXT;
> > > + } else {
> > > + dev_dbg(&adc->spi->dev, "use crystal oscillator as clocksource\n");
> > > + configreg &= ~MCP3911_CONFIG_CLKEXT;
> > > + }
>
> Sort of feels like this should be handled a bit like the regulator.
> The kernel has bindings and software support for clocks. Would be nice
> to use them.
>
Indeed. I will go for the clocks instead.
> > > +
> > > + ret = mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > > + if (ret)
> > > + return ret;
> > > +
> > > +
> > > + ret = mcp3911_read(adc, MCP3911_REG_STATUSCOM, &statuscomreg, 2);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > no duplicate newlines (here and elsewhere)
> >
> > > +
> > > + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> > > + switch (adc->width[0]) {
> > > + case 24:
> > > + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> > > + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> > > + break;
> > > + case 16:
> > > + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> > > + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> > > + break;
> > > + default:
> > > + adc->width[0] = 24;
> > > + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> > > + break;
> > > + }
> This feels like something that isn't really a dt choice, as it's not down to
> wiring but rather down to precision desired.
You are right. I will remove them and stick to 24bit width.
>
> Now variable resolution isn't something IIO has traditionally dealt
> with very well - particularly as it causes problems when we add in
> buffered interfaces (as it changes the data layout etc).
>
> > > +
> > > + of_property_read_u32(adc->np, "ch1-width", &adc->width[1]);
> > > + switch (adc->width[1]) {
> > > + case 24:
> > > + statuscomreg &= ~MCP3911_STATUSCOM_CH1_24WIDTH;
> > > + dev_dbg(&adc->spi->dev, "set channel 1 into 24bit mode\n");
> > > + break;
> > > + case 16:
> > > + statuscomreg |= MCP3911_STATUSCOM_CH1_24WIDTH;
> > > + dev_dbg(&adc->spi->dev, "set channel 1 into 16bit mode\n");
> > > + break;
> > > + default:
> > > + adc->width[1] = 24;
> > > + dev_info(&adc->spi->dev, "invalid width for channel 1. Use 24bit.\n");
> > > + break;
> > > + }
> > > +
> > > + return mcp3911_write(adc, MCP3911_REG_STATUSCOM, statuscomreg, 2);
> > > +}
> > > +
> > > +static int mcp3911_probe(struct spi_device *spi)
> > > +{
> > > + struct iio_dev *indio_dev;
> > > + struct mcp3911 *adc;
> > > + int ret;
> > > +
> > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + adc = iio_priv(indio_dev);
> > > + adc->spi = spi;
> > > + adc->np = spi->dev.of_node;
>
> np is is rather unusual naming. Why not of_node?
> Also, why do we need to keep a copy of this?
I guess the drivers I was looking at called it 'np'.
However, I guess there is no need to keep a copy of it.
I will remove it.
>
> > > +
> > > + ret = mcp3911_config_of(adc);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (adc->vrefext) {
> > > + adc->vref = devm_regulator_get(&adc->spi->dev, "vref");
>
> As I mention above, use devm_regulator_get_optional if you need
> to be able to do something different dependent on whether the regulator
> is actually there, if not just use devm_regulator_get and without
> any if (adc->vrefext) and you'll get a 'fake' regulator which you can
> enable and disable without it doing anything.
Got it. Thanks!
>
> > > + if (IS_ERR(adc->vref))
> > > + return PTR_ERR(adc->vref);
> > > +
> > > + ret = regulator_enable(adc->vref);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > +
> > > + /* Store gain values to better calculate scale values */
> > > + mcp3911_get_hwgain(adc, 0, &adc->gain[0]);
> > > + mcp3911_get_hwgain(adc, 1, &adc->gain[1]);
> > > +
> > > + indio_dev->dev.parent = &spi->dev;
> > > + indio_dev->dev.of_node = spi->dev.of_node;
> > > + indio_dev->name = spi_get_device_id(spi)->name;
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->info = &mcp3911_info;
> > > + spi_set_drvdata(spi, indio_dev);
> > > +
> > > + indio_dev->channels = mcp3911_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels);
> > > +
> > > + mutex_init(&adc->lock);
> > > +
> > > + ret = iio_device_register(indio_dev);
> > > + if (ret)
> > > + goto reg_disable;
> > > +
> > > + return ret;
> > > +
> > > +reg_disable:
> > > + if (adc->vref)
> > > + regulator_disable(adc->vref);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int 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);
> > > +
> > > + if (adc->vref)
> > > + regulator_disable(adc->vref);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#if defined(CONFIG_OF)
> > > +static const struct of_device_id mcp3911_dt_ids[] = {
> > > + { .compatible = "microchip,mcp3911" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids);
> > > +#endif
> > > +
> > > +static const struct spi_device_id mcp3911_id[] = {
> > > + { "mcp3911", 0 },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(spi, mcp3911_id);
> > > +
> > > +static struct spi_driver mcp3911_driver = {
> > > + .driver = {
> > > + .name = "mcp3911",
> > > + .of_match_table = of_match_ptr(mcp3911_dt_ids),
> > > + },
> > > + .probe = mcp3911_probe,
> > > + .remove = mcp3911_remove,
> > > + .id_table = mcp3911_id,
> > > +};
> > > +module_spi_driver(mcp3911_driver);
> > > +
> > > +MODULE_AUTHOR("Marcus Folkesson <[email protected]>");
> > > +MODULE_AUTHOR("Kent Gustavsson <[email protected]>");
> > > +MODULE_DESCRIPTION("Microchip Technology MCP3911");
> > > +MODULE_LICENSE("GPL v2");
> > >
> >
>
Best regards
Marcus Folkesson
Hi Jonathan,
On Sun, Jul 22, 2018 at 09:11:11AM +0100, Jonathan Cameron wrote:
> On Sat, 21 Jul 2018 21:59:22 +0200
> Marcus Folkesson <[email protected]> wrote:
>
[snip]
> > +Optional properties:
> > + - device-addr: Device address when multiple MCP3911 chips are present on the
> > + same SPI bus. Valid values are 0-3. Defaults to 0.
> > + - external-clock: Use external clock instead of crystal oscillator.
> As mentioned, in the code, can we use the standard fixed clock bindings here.
> We don't actually care what the value is, but it might be nice to be able to
> power down the clock if we are suspending or something..
>
> > + - external-vref: Use external voltage reference
> > + - vref-supply: Phandle to the external reference voltage supply. (only valid in combination with `external-vref`)
>
> Just use the optional regulator stuff and get rid of the bool.
>
> > + - ch0-width: width for channel0. Valid widths are 16 and 24bits.
> > + - ch1-width: width for channel1. Valid widths are 16 and 24bits.
>
> As I asked in the code, are these a function of the wiring etc or are
> they something we should really be leaving up to userspace (with a sensible
> default).
>
I agree with all of your comments.
I will remove the channel width properties and fix the regulator/clock
bindings.
Thanks,
Best regards
Marcus Folkesson
On Sun, 22 Jul 2018 21:00:51 +0200
Marcus Folkesson <[email protected]> wrote:
> Hi Jonathan,
>
> Thanks, all good catches.
>
> On Sun, Jul 22, 2018 at 09:08:38AM +0100, Jonathan Cameron wrote:
> > On Sat, 21 Jul 2018 23:19:48 +0200 (CEST)
> > Peter Meerwald-Stadler <[email protected]> wrote:
> >
> > > Hello,
> > >
> > > > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > >
> > > some comments below...
> >
> > +CC Mark for the unusual SPI addressing stuff. I'm mostly interested in what
> > precedent there is for bindings etc.
> >
>
> Yep, I'm not entirely sure that the SPI framework can handle multiple
> clients on the same CS.
> The reason why we created device-addr is that the chip supports that and
> may have hardcoded chip address from factory.
> The chip address is also part of the protocol so we have to specify it.
It will certainly be 'interesting' if we ever try to support it.
>
...
...
> > > > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > > > +{
> > > > + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> > > > +
> > > > + cpu_to_be32s(&val);
> > > > + val >>= (3-len)*8;
> > Hmm. It might be worth considering regmap here to handle all this stuff for
> > you rather than re rolling the same stuff.
> >
>
> We were looking at regmap, but it does not seems to support registers of
> different size.
> This chip has register values of 8, 16 and 24 bits.
Fair enough. I thought we were really looking at autoincrement, of
the address for a fixed sized register - which is fine in regmap.
Those wider registers are described as having ADDR and ADDR+1 etc.
>
> > > > + val |= REG_WRITE(reg, adc->dev_addr);
> > > > +
> > > > + return spi_write(adc->spi, &val, len+1);
> > > > +}
> > > > +
> > > > +
> > > > +static int mcp3911_read_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *channel, int *val,
> > > > + int *val2, long mask)
> > > > +{
> > > > + struct mcp3911 *adc = iio_priv(indio_dev);
> > > > + int ret = -EINVAL;
> > > > +
> > > > + mutex_lock(&adc->lock);
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + ret = mcp3911_read(adc,
> > > > + MCP3911_CHANNEL(channel->channel), val, 3);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_OFFSET:
> > > > + ret = mcp3911_read(adc,
> > > > + MCP3911_OFFCAL(channel->channel), val, 3);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > > > + ret = mcp3911_get_hwgain(adc, channel->channel, val);
> >
> > I'm not convinced it's useful to expose this as it right control for this
> > is scale.
> >
>
> Hmm, all other drivers that are using HARDWAREGAIN (ina2xx-adc, stx104 +
> a few more that are not ADC:s) are, what I can tell, exposing it.
>
> But maybe it should'nt.
Yup, as ever things are a bit messy. However, best to not expose it unless
there is a very good reason.
> > > > +
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_HARDWAREGAIN:
> >
> > Default choice (by precedent) is to control variable gain
> > front ends via the scale parameter. Hardware gain
> > is not meant to have any 'visible' impact on the output
> > value - most commonly used when the thing we are measuring
> > is not amplitude of anything.
>
> Hmm, Ok. I'm not sure I understand how hardware gain is supposed to work
> then.
For this use case it isn't.
> Maybe I just remove it.
That's the best plan.
...
> > > > +
> > > > +static int mcp3911_config_of(struct mcp3911 *adc)
> > > > +{
> > > > + u32 configreg;
> > > > + u32 statuscomreg;
> > > > + int ret;
> > > > +
> > > > + of_property_read_u32(adc->np, "device-addr", &adc->dev_addr);
> > This is 'interesting' - I wonder if there is any precedence for it.
> >
>
> I guess we still need it since the device may have a hardcoded (from
> factory) address that we need to deal with.
Fair enough. Still good to hear from Mark on whether there are other similar
devices so the binding can be consistent.
> > > > +
> > > > + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> > > > + switch (adc->width[0]) {
> > > > + case 24:
> > > > + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> > > > + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> > > > + break;
> > > > + case 16:
> > > > + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> > > > + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> > > > + break;
> > > > + default:
> > > > + adc->width[0] = 24;
> > > > + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> > > > + break;
> > > > + }
> > This feels like something that isn't really a dt choice, as it's not down to
> > wiring but rather down to precision desired.
>
> You are right. I will remove them and stick to 24bit width.
>
That's the easiest option. If anyone 'really' wants 16bit we'll figure it out
later.
..
Thanks
Jonathan