[PATCH v3 1/3] iio: adc: add support for mcp3911

[PATCH v3 1/3] iio: adc: add support for mcp3911

[Marcus Folkesson]

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 <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v3:
    	- rename adc_clk to clki
    	- add error handling/cleanup for clock
    v2:
    	- cleanups and bugfixes (thanks Peter Meerwald-Stadler)
    	- drop hardware gain
    	- use the presence or lack of regulator to indicate if we go for internal or external voltage reference
    	- do not store device node in private struct
    	- drop support to set width in devicetree
    	- use the presence or lack of clock to indicate if we go for internal or external clock

 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/mcp3911.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 378 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..fca8e6a304ec
--- /dev/null
+++ b/drivers/iio/adc/mcp3911.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Microchip MCP3911, Two-channel Analog Front End
+ *
+ * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
+ * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
+ *
+ */
+#include <linux/clk.h>
+#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_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)
+
+/* Internal voltage reference in uV */
+#define MCP3911_INT_VREF_UV		1200000
+
+#define MCP3911_REG_READ(reg, id)	((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
+#define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
+
+#define MCP3911_NUM_CHANNELS		2
+
+struct mcp3911 {
+	struct spi_device *spi;
+	struct mutex lock;
+	struct regulator *vref;
+	struct clk *clki;
+	u32 dev_addr;
+};
+
+static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
+{
+	int ret;
+
+	reg = MCP3911_REG_READ(reg, adc->dev_addr);
+	ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
+	if (ret < 0)
+		return ret;
+
+	be32_to_cpus(val);
+	*val >>= ((4 - len) * 8);
+	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);
+
+	val <<= (3 - len) * 8;
+	cpu_to_be32s(&val);
+	val |= MCP3911_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_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_SCALE:
+		if (adc->vref) {
+			ret = regulator_get_voltage(adc->vref);
+			if (ret < 0) {
+				dev_err(indio_dev->dev.parent,
+					"failed to get vref voltage: %d\n",
+				       ret);
+				goto out;
+			}
+
+			*val = ret / 1000;
+		} else {
+			*val = MCP3911_INT_VREF_UV;
+		}
+
+		*val2 = 24;
+		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:
+		if (val2 != 0) {
+			ret = -EINVAL;
+			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);
+		break;
+	}
+
+out:
+	mutex_unlock(&adc->lock);
+	return ret;
+}
+
+#define MCP3911_CHAN(idx) {					\
+		.type = IIO_VOLTAGE,				\
+		.indexed = 1,					\
+		.channel = idx,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+			BIT(IIO_CHAN_INFO_OFFSET) |		\
+			BIT(IIO_CHAN_INFO_SCALE),		\
+}
+
+static const struct iio_chan_spec mcp3911_channels[] = {
+	MCP3911_CHAN(0),
+	MCP3911_CHAN(1),
+};
+
+static const struct iio_info mcp3911_info = {
+	.read_raw = mcp3911_read_raw,
+	.write_raw = mcp3911_write_raw,
+};
+
+static int mcp3911_config(struct mcp3911 *adc, struct device_node *of_node)
+{
+	u32 configreg;
+	int ret;
+
+	of_property_read_u32(of_node, "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;
+
+	if (adc->vref) {
+		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 (adc->clki) {
+		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;
+	}
+
+	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 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->vref = devm_regulator_get_optional(&adc->spi->dev, "vref");
+	if (IS_ERR(adc->vref)) {
+
+		if (PTR_ERR(adc->vref) == -ENODEV) {
+			adc->vref = NULL;
+		} else {
+			dev_err(&adc->spi->dev,
+					"failed to get regulator (%ld)\n",
+					PTR_ERR(adc->vref));
+			return PTR_ERR(adc->vref);
+		}
+
+	} else {
+		ret = regulator_enable(adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
+	if (IS_ERR(adc->clki)) {
+		if (PTR_ERR(adc->clki) == -ENOENT) {
+			adc->clki = NULL;
+		} else {
+			dev_err(&adc->spi->dev,
+					"failed to get adc clk (%ld)\n",
+					PTR_ERR(adc->clki));
+			ret = PTR_ERR(adc->clki);
+			goto reg_disable;
+		}
+	} else {
+		ret = clk_prepare_enable(adc->clki);
+		if (ret < 0) {
+			dev_err(&adc->spi->dev,
+				"Failed to enable clki: %d\n", ret);
+			goto reg_disable;
+		}
+	}
+
+	ret = mcp3911_config(adc, spi->dev.of_node);
+	if (ret)
+		goto clk_disable;
+
+	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 clk_disable;
+
+	return ret;
+
+clk_disable:
+	if (adc->clki)
+		clk_disable_unprepare(adc->clki);
+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);
+	if (adc->clki)
+		clk_disable_unprepare(adc->clki);
+
+	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 <marcus.folkesson@gmail.com>");
+MODULE_AUTHOR("Kent Gustavsson <kent@minoris.se>");
+MODULE_DESCRIPTION("Microchip Technology MCP3911");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0.rc2

[PATCH v3 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Marcus Folkesson]

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 <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v3:
    	- add bindings for interrupt
    	- prefix device-addr with `microchip`
    	- drop `clock-names`
    v2:
    	- drop channel width
    	- drop `external_vref`
    	- replace `external-clock` with proper clock bindings

 .../devicetree/bindings/iio/adc/mcp3911.txt        | 32 ++++++++++++++++++++++
 1 file changed, 32 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..432da7a28783
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
@@ -0,0 +1,32 @@
+* 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:
+ - clocks: Phandle and clock identifier for sampling clock
+ - interrupt-parent: Phandle to the parent interrupt controller
+	see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+ - interrupts: IRQ line for the ADC
+	see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+ - microchip,device-addr: Device address when multiple MCP3911 chips are present on the
+	same SPI bus. Valid values are 0-3. Defaults to 0.
+ - vref-supply: Phandle to the external reference voltage supply.
+
+Example:
+adc@0 {
+	compatible = "microchip,mcp3911";
+	reg = <0>;
+	interrupt-parent = <&gpio5>;
+	interrupts = <15 IRQ_TYPE_EDGE_RISING>;
+	spi-max-frequency = <20000000>;
+	microchip,device-addr = <0>;
+	vref-supply = <&vref_reg>;
+	clocks = <&xtal>;
+};
-- 
2.11.0.rc2

[PATCH v3 3/3] MAINTAINERS: Add entry for mcp3911 ADC driver

[Marcus Folkesson]

Add an entry for mcp3911 ADC driver and add myself and
Kent Gustavsson as maintainers of this driver.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Signed-off-by: Kent Gustavsson <kent@minoris.se>
---

Notes:
    v3:
    	- no changes
    v2:
    	- no changes

 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:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP / ATMEL MCP3911 ADC DRIVER
+M:	Marcus Folkesson <marcus.folkesson@gmail.com>
+M:	Kent Gustavsson <kent@minoris.se>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/adc/mcp3911.c
+F:	Documentation/devicetree/bindings/iio/adc/mcp3911.txt
+
 MICROCHIP USB251XB DRIVER
 M:	Richard Leitner <richard.leitner@skidata.com>
 L:	linux-usb@vger.kernel.org
-- 
2.11.0.rc2

Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Andy Shevchenko]

On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
<marcus.folkesson@gmail.com> 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 <marcus.folkesson@gmail.com>

> Signed-off-by: Kent Gustavsson <kent@minoris.se>

What is this? Why it's here (presense and location in the message)?

> + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>

> + *

Redundant.

> +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> +{
> +       int ret;
> +
> +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> +       if (ret < 0)
> +               return ret;
> +
> +       be32_to_cpus(val);
> +       *val >>= ((4 - len) * 8);
> +       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);
> +
> +       val <<= (3 - len) * 8;
> +       cpu_to_be32s(&val);
> +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> +
> +       return spi_write(adc->spi, &val, len + 1);
> +}

Can't you use REGMAP_SPI ?

> +       of_property_read_u32(of_node, "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);

Isn't what we called CS (chip select)?

> +       if (IS_ERR(adc->vref)) {

> +

Redundant.

> +       if (adc->clki)

Seems to be redundant if clock is optional (it should be NULL and API
is NULL-aware).

> +               clk_disable_unprepare(adc->clki);

> +       if (adc->clki)
> +               clk_disable_unprepare(adc->clki);

Ditto.

> +#if defined(CONFIG_OF)

This prevents playing with the device on ACPI enabled platforms.

> +               .of_match_table = of_match_ptr(mcp3911_dt_ids),

Ditto for macro.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Jonathan Cameron]

On Thu, 2 Aug 2018 22:52:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> <marcus.folkesson@gmail.com> 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 <marcus.folkesson@gmail.com>  
> 
> > Signed-off-by: Kent Gustavsson <kent@minoris.se>  
> 
> What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting
as gatekeeper / upstreamer you should set the mail to be from Kent
and put your own Signed-off after his to basically act as a submaintainer
certifying you believe his sign off and all that entails.

If it is a bit of a joint effort then that's fine but for copyright
purposes there should be some indication of the split.


> 
> > + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>  
> 
> > + *  
> 
> Redundant.
> 
> > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > +{
> > +       int ret;
> > +
> > +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> > +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       be32_to_cpus(val);
> > +       *val >>= ((4 - len) * 8);
> > +       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);
> > +
> > +       val <<= (3 - len) * 8;
> > +       cpu_to_be32s(&val);
> > +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> > +
> > +       return spi_write(adc->spi, &val, len + 1);
> > +}  
> 
> Can't you use REGMAP_SPI ?
My instinct is not really. The magic device-addr doesn't help.
You could work around it but it's not that nice and you'd have
to create the regmap mapping on the fly or carry static ones
for each value of htat.



> 
> > +       of_property_read_u32(of_node, "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);  
> 
> Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted
in the control byte to allow you to 'share' a chip select line between multiple
chips (crazy but true).

> 
> > +       if (IS_ERR(adc->vref)) {  
> 
> > +  
> 
> Redundant.
> 
> > +       if (adc->clki)  
> 
> Seems to be redundant if clock is optional (it should be NULL and API
> is NULL-aware).
> 
> > +               clk_disable_unprepare(adc->clki);  
> 
> > +       if (adc->clki)
> > +               clk_disable_unprepare(adc->clki);  
> 
> Ditto.
> 
> > +#if defined(CONFIG_OF)  
> 
> This prevents playing with the device on ACPI enabled platforms.
Yeah, that trickery is still little known (and I forget about it from
time to time).

The upshot for those who have missed this is you can use a magic
acpi device to instantiate based on device tree bindings :)

So we need to strip all this 'obvious' protection stuff out of drivers.

> 
> > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),  
> 
> Ditto for macro.
> 

Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Marcus Folkesson]

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

Hi Andy and Jonathan,


On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
> On Thu, 2 Aug 2018 22:52:00 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> > <marcus.folkesson@gmail.com> 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 <marcus.folkesson@gmail.com>  
> > 
> > > Signed-off-by: Kent Gustavsson <kent@minoris.se>  
> > 
> > What is this? Why it's here (presense and location in the message)?
> To clarify... If Kent wrote the patch and you are simply acting
> as gatekeeper / upstreamer you should set the mail to be from Kent
> and put your own Signed-off after his to basically act as a submaintainer
> certifying you believe his sign off and all that entails.
> 
> If it is a bit of a joint effort then that's fine but for copyright
> purposes there should be some indication of the split.

First, thank you Andy for noticing.

I actually intended to use Co-Developed-by (a pretty new tag)
in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..

From Documentation/process/submitting-patches.rst ::

	A Co-Developed-by: states that the patch was also created by another developer
	along with the original author.  This is useful at times when multiple people
	work on a single patch.  Note, this person also needs to have a Signed-off-by:
	line in the patch as well.

I will switch order and add the Co-Developed-by-tag.
Is this correct?

Co-Developed-by: Kent Gustavsson <kent@minoris.se>   
Signed-off-by: Kent Gustavsson <kent@minoris.se>  
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>  

> 
> 
> > 
> > > + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>  
> > 
> > > + *  
> > 
> > Redundant.
> > 
> > > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > > +{
> > > +       int ret;
> > > +
> > > +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> > > +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       be32_to_cpus(val);
> > > +       *val >>= ((4 - len) * 8);
> > > +       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);
> > > +
> > > +       val <<= (3 - len) * 8;
> > > +       cpu_to_be32s(&val);
> > > +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> > > +
> > > +       return spi_write(adc->spi, &val, len + 1);
> > > +}  
> > 
> > Can't you use REGMAP_SPI ?
> My instinct is not really. The magic device-addr doesn't help.
> You could work around it but it's not that nice and you'd have
> to create the regmap mapping on the fly or carry static ones
> for each value of htat.
> 
> 
> 
> > 
> > > +       of_property_read_u32(of_node, "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);  
> > 
> > Isn't what we called CS (chip select)?
> Nope. We went around this in an earlier revision. It's an address transmitted
> in the control byte to allow you to 'share' a chip select line between multiple
> chips (crazy but true).
> 
> > 
> > > +       if (IS_ERR(adc->vref)) {  
> > 
> > > +  
> > 
> > Redundant.
> > 
> > > +       if (adc->clki)  
> > 
> > Seems to be redundant if clock is optional (it should be NULL and API
> > is NULL-aware).
> > 
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > > +       if (adc->clki)
> > > +               clk_disable_unprepare(adc->clki);  
> > 
> > Ditto.
> > 
> > > +#if defined(CONFIG_OF)  
> > 
> > This prevents playing with the device on ACPI enabled platforms.

I will remove the defined(CONFIG_OF) and not use the of_match_ptr()
macro. 

> Yeah, that trickery is still little known (and I forget about it from
> time to time).
> 
> The upshot for those who have missed this is you can use a magic
> acpi device to instantiate based on device tree bindings :)
> 
> So we need to strip all this 'obvious' protection stuff out of drivers.

Jonathan,
Do you want me to do the same changes for drivers in iio/ ?
I'm probably not the only one looking at other drivers when writing my
own, so I guess this is a rather frequent issue for new drivers?


> 
> > 
> > > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),  
> > 
> > Ditto for macro.
> > 
> 

Best regards,
Marcus Folkesson

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

Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Andy Shevchenko]

On Sat, Aug 4, 2018 at 9:55 AM, Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
> On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
>> On Thu, 2 Aug 2018 22:52:00 +0300
>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>> > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
>> > <marcus.folkesson@gmail.com> 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 <marcus.folkesson@gmail.com>
>> >
>> > > Signed-off-by: Kent Gustavsson <kent@minoris.se>
>> >
>> > What is this? Why it's here (presense and location in the message)?
>> To clarify... If Kent wrote the patch and you are simply acting
>> as gatekeeper / upstreamer you should set the mail to be from Kent
>> and put your own Signed-off after his to basically act as a submaintainer
>> certifying you believe his sign off and all that entails.

Yep. And here the ordering is incorrect, right?

>> If it is a bit of a joint effort then that's fine but for copyright
>> purposes there should be some indication of the split.
>
> First, thank you Andy for noticing.
>
> I actually intended to use Co-Developed-by (a pretty new tag)
> in combination with Signed-off-by.
> But the tag must have disappeared in some preparation stage..
>
> From Documentation/process/submitting-patches.rst ::
>
>         A Co-Developed-by: states that the patch was also created by another developer
>         along with the original author.  This is useful at times when multiple people
>         work on a single patch.  Note, this person also needs to have a Signed-off-by:
>         line in the patch as well.
>
> I will switch order and add the Co-Developed-by-tag.
> Is this correct?
>
> Co-Developed-by: Kent Gustavsson <kent@minoris.se>
> Signed-off-by: Kent Gustavsson <kent@minoris.se>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Yep, this looks good!

>> > > +       of_property_read_u32(of_node, "device-addr", &adc->dev_addr);

>> > Isn't what we called CS (chip select)?
>> Nope. We went around this in an earlier revision. It's an address transmitted
>> in the control byte to allow you to 'share' a chip select line between multiple
>> chips (crazy but true).

OK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] dt-bindings: iio: adc: add bindings for mcp3911

[Rob Herring]

On Thu, Aug 02, 2018 at 09:15:53PM +0200, Marcus Folkesson 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 <marcus.folkesson@gmail.com>
> Signed-off-by: Kent Gustavsson <kent@minoris.se>
> ---
> 
> Notes:
>     v3:
>     	- add bindings for interrupt
>     	- prefix device-addr with `microchip`
>     	- drop `clock-names`
>     v2:
>     	- drop channel width
>     	- drop `external_vref`
>     	- replace `external-clock` with proper clock bindings
> 
>  .../devicetree/bindings/iio/adc/mcp3911.txt        | 32 ++++++++++++++++++++++
>  1 file changed, 32 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..432da7a28783
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
> @@ -0,0 +1,32 @@
> +* 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:
> + - clocks: Phandle and clock identifier for sampling clock
> + - interrupt-parent: Phandle to the parent interrupt controller
> +	see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Drop this. It is implied.

With that, 

Reviewed-by: Rob Herring <robh@kernel.org>

> + - interrupts: IRQ line for the ADC
> +	see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> + - microchip,device-addr: Device address when multiple MCP3911 chips are present on the
> +	same SPI bus. Valid values are 0-3. Defaults to 0.
> + - vref-supply: Phandle to the external reference voltage supply.
> +
> +Example:
> +adc@0 {
> +	compatible = "microchip,mcp3911";
> +	reg = <0>;
> +	interrupt-parent = <&gpio5>;
> +	interrupts = <15 IRQ_TYPE_EDGE_RISING>;
> +	spi-max-frequency = <20000000>;
> +	microchip,device-addr = <0>;
> +	vref-supply = <&vref_reg>;
> +	clocks = <&xtal>;
> +};
> -- 
> 2.11.0.rc2
> 

Re: [PATCH v3 1/3] iio: adc: add support for mcp3911

[Jonathan Cameron]

On Sat, 4 Aug 2018 08:55:06 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Hi Andy and Jonathan,
> 
> 
> On Fri, Aug 03, 2018 at 11:09:22PM +0100, Jonathan Cameron wrote:
> > On Thu, 2 Aug 2018 22:52:00 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >   
> > > On Thu, Aug 2, 2018 at 10:15 PM, Marcus Folkesson
> > > <marcus.folkesson@gmail.com> 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 <marcus.folkesson@gmail.com>    
> > >   
> > > > Signed-off-by: Kent Gustavsson <kent@minoris.se>    
> > > 
> > > What is this? Why it's here (presense and location in the message)?  
> > To clarify... If Kent wrote the patch and you are simply acting
> > as gatekeeper / upstreamer you should set the mail to be from Kent
> > and put your own Signed-off after his to basically act as a submaintainer
> > certifying you believe his sign off and all that entails.
> > 
> > If it is a bit of a joint effort then that's fine but for copyright
> > purposes there should be some indication of the split.  
> 
> First, thank you Andy for noticing.
> 
> I actually intended to use Co-Developed-by (a pretty new tag)
> in combination with Signed-off-by.
> But the tag must have disappeared in some preparation stage..
> 
> From Documentation/process/submitting-patches.rst ::
> 
> 	A Co-Developed-by: states that the patch was also created by another developer
> 	along with the original author.  This is useful at times when multiple people
> 	work on a single patch.  Note, this person also needs to have a Signed-off-by:
> 	line in the patch as well.
> 
> I will switch order and add the Co-Developed-by-tag.
> Is this correct?
> 
> Co-Developed-by: Kent Gustavsson <kent@minoris.se>   
> Signed-off-by: Kent Gustavsson <kent@minoris.se>  
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>  
> 
> > 
> >   
> > >   
> > > > + * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>    
> > >   
> > > > + *    
> > > 
> > > Redundant.
> > >   
> > > > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       reg = MCP3911_REG_READ(reg, adc->dev_addr);
> > > > +       ret = spi_write_then_read(adc->spi, &reg, 1, val, len);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       be32_to_cpus(val);
> > > > +       *val >>= ((4 - len) * 8);
> > > > +       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);
> > > > +
> > > > +       val <<= (3 - len) * 8;
> > > > +       cpu_to_be32s(&val);
> > > > +       val |= MCP3911_REG_WRITE(reg, adc->dev_addr);
> > > > +
> > > > +       return spi_write(adc->spi, &val, len + 1);
> > > > +}    
> > > 
> > > Can't you use REGMAP_SPI ?  
> > My instinct is not really. The magic device-addr doesn't help.
> > You could work around it but it's not that nice and you'd have
> > to create the regmap mapping on the fly or carry static ones
> > for each value of htat.
> > 
> > 
> >   
> > >   
> > > > +       of_property_read_u32(of_node, "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);    
> > > 
> > > Isn't what we called CS (chip select)?  
> > Nope. We went around this in an earlier revision. It's an address transmitted
> > in the control byte to allow you to 'share' a chip select line between multiple
> > chips (crazy but true).
> >   
> > >   
> > > > +       if (IS_ERR(adc->vref)) {    
> > >   
> > > > +    
> > > 
> > > Redundant.
> > >   
> > > > +       if (adc->clki)    
> > > 
> > > Seems to be redundant if clock is optional (it should be NULL and API
> > > is NULL-aware).
> > >   
> > > > +               clk_disable_unprepare(adc->clki);    
> > >   
> > > > +       if (adc->clki)
> > > > +               clk_disable_unprepare(adc->clki);    
> > > 
> > > Ditto.
> > >   
> > > > +#if defined(CONFIG_OF)    
> > > 
> > > This prevents playing with the device on ACPI enabled platforms.  
> 
> I will remove the defined(CONFIG_OF) and not use the of_match_ptr()
> macro. 
> 
> > Yeah, that trickery is still little known (and I forget about it from
> > time to time).
> > 
> > The upshot for those who have missed this is you can use a magic
> > acpi device to instantiate based on device tree bindings :)
> > 
> > So we need to strip all this 'obvious' protection stuff out of drivers.  
> 
> Jonathan,
> Do you want me to do the same changes for drivers in iio/ ?
> I'm probably not the only one looking at other drivers when writing my
> own, so I guess this is a rather frequent issue for new drivers?

A good question.   Hmm. I'm a bit in two minds.  It creates a lot
of churn for something that I don't think is actually very heavily
used.  As you say, the biggest reason to change it will be to stop
others from copying the existing drivers and getting it 'wrong' going
forward.

I don't have strong feelings either way, but don't really want to
see a huge number of patches around this (I haven't checked how
many drivers are effected).  If you want to look at this, then perhaps
do a small set at a time rather than a mass fixup to keep things
manageable?

Thanks,

Jonathan

> 
> 
> >   
> > >   
> > > > +               .of_match_table = of_match_ptr(mcp3911_dt_ids),    
> > > 
> > > Ditto for macro.
> > >   
> >   
> 
> Best regards,
> Marcus Folkesson