[PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Chris Coffey]

This patch adds driver support for the Microchip MCP41xxx/42xxx family
of digital potentiometers:

DEVICE      Wipers  Positions   Resistance (kOhm)
MCP41010    1       256         10
MCP41050    1       256         50
MCP41100    1       256         100
MCP42010    2       256         10
MCP42050    2       256         50
MCP42100    2       256         100

Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf

Signed-off-by: Chris Coffey <cmc@babblebit.net>
---
 .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++
 drivers/iio/potentiometer/Kconfig                  |  12 ++
 drivers/iio/potentiometer/Makefile                 |   1 +
 drivers/iio/potentiometer/mcp41010.c               | 216 +++++++++++++++++++++
 4 files changed, 258 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
 create mode 100644 drivers/iio/potentiometer/mcp41010.c

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
new file mode 100644
index 0000000000..17565acace
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
@@ -0,0 +1,29 @@
+* Microchip MCP41010/41050/41100/42010/42050/42100 Digital Potentiometer
+  driver
+
+Datasheet publicly available at:
+http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in
+
+        Documentation/devicetree/bindings/spi/spi-bus.txt
+
+must be specified.
+
+Required properties:
+	- compatible:  	Must be one of the following, depending on the
+			model:
+			"microchip,mcp41010"
+			"microchip,mcp41050"
+			"microchip,mcp41100"
+			"microchip,mcp42010"
+			"microchip,mcp42050"
+			"microchip,mcp42100"
+
+Example:
+mcp41010: potentiometer@0 {
+	compatible = "mcp41010";
+	reg = <0>;
+	spi-max-frequency = <500000>;
+};
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 79ec2eba49..6303cbe799 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -90,6 +90,18 @@ config MCP4531
 	  To compile this driver as a module, choose M here: the
 	  module will be called mcp4531.
 
+config MCP41010
+	tristate "Microchip MCP41xxx/MCP42xxx Digital Potentiometer driver"
+	depends on SPI
+	help
+	  Say yes here to build support for the Microchip
+	  MCP41010, MCP41050, MCP41100,
+	  MCP42010, MCP42050, MCP42100
+	  digital potentiometer chips.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mcp41010.
+
 config TPL0102
 	tristate "Texas Instruments digital potentiometer driver"
 	depends on I2C
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 4af657883c..8ff55138cf 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_MAX5487) += max5487.o
 obj-$(CONFIG_MCP4018) += mcp4018.o
 obj-$(CONFIG_MCP4131) += mcp4131.o
 obj-$(CONFIG_MCP4531) += mcp4531.o
+obj-$(CONFIG_MCP41010) += mcp41010.o
 obj-$(CONFIG_TPL0102) += tpl0102.o
diff --git a/drivers/iio/potentiometer/mcp41010.c b/drivers/iio/potentiometer/mcp41010.c
new file mode 100644
index 0000000000..4068e8eb57
--- /dev/null
+++ b/drivers/iio/potentiometer/mcp41010.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Industrial I/O driver for Microchip digital potentiometers
+ *
+ * Copyright (c) 2018 Chris Coffey <cmc@babblebit.net>
+ * Based on: Slawomir Stepien's code from mcp4131.c
+ *
+ * Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
+ *
+ * DEVID	#Wipers	#Positions	Resistance (kOhm)
+ * mcp41010	1	256		10
+ * mcp41050	1	256		50
+ * mcp41100	1	256		100
+ * mcp42010	2	256		10
+ * mcp42050	2	256		50
+ * mcp42100	2	256		100
+ *
+ */
+
+#include <linux/cache.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+
+#define MCP41010_MAX_WIPERS	2
+#define MCP41010_WRITE		(0x01 << 4)
+#define MCP41010_WIPER_MAX	255
+#define MCP41010_WIPER_ENABLE	BIT(0)
+
+struct mcp41010_cfg {
+	int wipers;
+	int kohms;
+};
+
+enum mcp41010_type {
+	MCP41010 = 0,
+	MCP41050,
+	MCP41100,
+	MCP42010,
+	MCP42050,
+	MCP42100,
+};
+
+static const struct mcp41010_cfg mcp41010_cfg[] = {
+	[MCP41010] = { .wipers = 1, .kohms =  10, },
+	[MCP41050] = { .wipers = 1, .kohms =  50, },
+	[MCP41100] = { .wipers = 1, .kohms = 100, },
+	[MCP42010] = { .wipers = 2, .kohms =  10, },
+	[MCP42050] = { .wipers = 2, .kohms =  50, },
+	[MCP42100] = { .wipers = 2, .kohms = 100, },
+};
+
+struct mcp41010_data {
+	struct spi_device *spi;
+	const struct mcp41010_cfg *cfg;
+	struct mutex lock; /* Protect write sequences */
+	unsigned int value[MCP41010_MAX_WIPERS]; /* Cache wiper values */
+	u8 buf[2] ____cacheline_aligned;
+};
+
+#define MCP41010_CHANNEL(ch) {					\
+	.type = IIO_RESISTANCE,					\
+	.indexed = 1,						\
+	.output = 1,						\
+	.channel = (ch),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+}
+
+static const struct iio_chan_spec mcp41010_channels[] = {
+	MCP41010_CHANNEL(0),
+	MCP41010_CHANNEL(1),
+};
+
+static int mcp41010_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct mcp41010_data *data = iio_priv(indio_dev);
+	int channel = chan->channel;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = data->value[channel];
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1000 * data->cfg->kohms;
+		*val2 = MCP41010_WIPER_MAX;
+		return IIO_VAL_FRACTIONAL;
+	}
+
+	return -EINVAL;
+}
+
+static int mcp41010_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	int err;
+	struct mcp41010_data *data = iio_priv(indio_dev);
+	int channel = chan->channel;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > MCP41010_WIPER_MAX || val < 0)
+			return -EINVAL;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->lock);
+
+	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
+	data->buf[0] |= MCP41010_WRITE;
+	data->buf[1] = val & 0xff;
+
+	err = spi_write(data->spi, data->buf, 2);
+	if (!err)
+		data->value[channel] = val;
+
+	mutex_unlock(&data->lock);
+
+	return err;
+}
+
+static const struct iio_info mcp41010_info = {
+	.read_raw = mcp41010_read_raw,
+	.write_raw = mcp41010_write_raw,
+};
+
+static int mcp41010_probe(struct spi_device *spi)
+{
+	int err;
+	struct device *dev = &spi->dev;
+	unsigned long devid = spi_get_device_id(spi)->driver_data;
+	struct mcp41010_data *data;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
+	data->spi = spi;
+	data->cfg = &mcp41010_cfg[devid];
+
+	mutex_init(&data->lock);
+
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &mcp41010_info;
+	indio_dev->channels = mcp41010_channels;
+	indio_dev->num_channels = data->cfg->wipers;
+	indio_dev->name = spi_get_device_id(spi)->name;
+
+	err = devm_iio_device_register(dev, indio_dev);
+	if (err) {
+		dev_info(&spi->dev, "Unable to register %s\n", indio_dev->name);
+		return err;
+	}
+
+	return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id mcp41010_dt_ids[] = {
+	{ .compatible = "microchip,mcp41010",
+		.data = &mcp41010_cfg[MCP41010] },
+	{ .compatible = "microchip,mcp41050",
+		.data = &mcp41010_cfg[MCP41050] },
+	{ .compatible = "microchip,mcp41100",
+		.data = &mcp41010_cfg[MCP41100] },
+	{ .compatible = "microchip,mcp42010",
+		.data = &mcp41010_cfg[MCP42010] },
+	{ .compatible = "microchip,mcp42050",
+		.data = &mcp41010_cfg[MCP42050] },
+	{ .compatible = "microchip,mcp42100",
+		.data = &mcp41010_cfg[MCP42100] },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mcp41010_dt_ids);
+#endif /* CONFIG_OF */
+
+static const struct spi_device_id mcp41010_id[] = {
+	{ "mcp41010", MCP41010 },
+	{ "mcp41050", MCP41050 },
+	{ "mcp41100", MCP41100 },
+	{ "mcp42010", MCP42010 },
+	{ "mcp42050", MCP42050 },
+	{ "mcp42100", MCP42100 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp41010_id);
+
+static struct spi_driver mcp41010_driver = {
+	.driver = {
+		.name	= "mcp41010",
+		.of_match_table = of_match_ptr(mcp41010_dt_ids),
+	},
+	.probe		= mcp41010_probe,
+	.id_table	= mcp41010_id,
+};
+
+module_spi_driver(mcp41010_driver);
+
+MODULE_AUTHOR("Chris Coffey <cmc@babblebit.net>");
+MODULE_DESCRIPTION("MCP41010 digital potentiometer");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Himanshu Jha]

On Tue, Nov 06, 2018 at 11:31:30AM +0000, Chris Coffey wrote:
> This patch adds driver support for the Microchip MCP41xxx/42xxx family
> of digital potentiometers:
> 
> DEVICE      Wipers  Positions   Resistance (kOhm)
> MCP41010    1       256         10
> MCP41050    1       256         50
> MCP41100    1       256         100
> MCP42010    2       256         10
> MCP42050    2       256         50
> MCP42100    2       256         100
> 
> Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> 
> Signed-off-by: Chris Coffey <cmc@babblebit.net>
> ---
>  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++

WARNING: DT binding docs and includes should be a separate patch. 
See: Documentation/devicetree/bindings/submitting-patches.txt

Please run checkpatch.pl on this patch once.

> +#define MCP41010_MAX_WIPERS	2
> +#define MCP41010_WRITE		(0x01 << 4)

#define MCP41010_WRITE			BIT(4) ?

> +#define MCP41010_WIPER_MAX	255
> +#define MCP41010_WIPER_ENABLE	BIT(0)
> +
> +struct mcp41010_cfg {
> +	int wipers;
> +	int kohms;
> +};
> +
> +enum mcp41010_type {
> +	MCP41010 = 0,

0 initialisation is implicit, hence `= 0` not
required.

> +	MCP41050,
> +	MCP41100,
> +	MCP42010,
> +	MCP42050,
> +	MCP42100,
> +};

[]

> +static int mcp41010_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	int err;
> +	struct mcp41010_data *data = iio_priv(indio_dev);
> +	int channel = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val > MCP41010_WIPER_MAX || val < 0)
> +			return -EINVAL;
> +		break;

This looks weird.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->lock);
> +
> +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> +	data->buf[0] |= MCP41010_WRITE;
> +	data->buf[1] = val & 0xff;
> +
> +	err = spi_write(data->spi, data->buf, 2);
> +	if (!err)
> +		data->value[channel] = val;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return err;
> +}

[]

> +static int mcp41010_probe(struct spi_device *spi)
> +{
> +	int err;
> +	struct device *dev = &spi->dev;
> +	unsigned long devid = spi_get_device_id(spi)->driver_data;
> +	struct mcp41010_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);
> +	data->spi = spi;
> +	data->cfg = &mcp41010_cfg[devid];
> +
> +	mutex_init(&data->lock);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &mcp41010_info;
> +	indio_dev->channels = mcp41010_channels;
> +	indio_dev->num_channels = data->cfg->wipers;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +
> +	err = devm_iio_device_register(dev, indio_dev);
> +	if (err) {
> +		dev_info(&spi->dev, "Unable to register %s\n", indio_dev->name);
> +		return err;
> +	}

I think direct return is preferred.
Nevermind ...


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

Hi!

Some comments inline...

On 2018-11-06 12:31, Chris Coffey wrote:
> This patch adds driver support for the Microchip MCP41xxx/42xxx family
> of digital potentiometers:
> 
> DEVICE      Wipers  Positions   Resistance (kOhm)
> MCP41010    1       256         10
> MCP41050    1       256         50
> MCP41100    1       256         100
> MCP42010    2       256         10
> MCP42050    2       256         50
> MCP42100    2       256         100
> 
> Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> 
> Signed-off-by: Chris Coffey <cmc@babblebit.net>
> ---
>  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++
>  drivers/iio/potentiometer/Kconfig                  |  12 ++
>  drivers/iio/potentiometer/Makefile                 |   1 +
>  drivers/iio/potentiometer/mcp41010.c               | 216 +++++++++++++++++++++
>  4 files changed, 258 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
>  create mode 100644 drivers/iio/potentiometer/mcp41010.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
> new file mode 100644
> index 0000000000..17565acace
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
> @@ -0,0 +1,29 @@
> +* Microchip MCP41010/41050/41100/42010/42050/42100 Digital Potentiometer
> +  driver
> +
> +Datasheet publicly available at:
> +http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in
> +
> +        Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +must be specified.
> +
> +Required properties:
> +	- compatible:  	Must be one of the following, depending on the
> +			model:
> +			"microchip,mcp41010"
> +			"microchip,mcp41050"
> +			"microchip,mcp41100"
> +			"microchip,mcp42010"
> +			"microchip,mcp42050"
> +			"microchip,mcp42100"
> +
> +Example:
> +mcp41010: potentiometer@0 {
> +	compatible = "mcp41010";
> +	reg = <0>;
> +	spi-max-frequency = <500000>;
> +};
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 79ec2eba49..6303cbe799 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -90,6 +90,18 @@ config MCP4531
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called mcp4531.
>  
> +config MCP41010
> +	tristate "Microchip MCP41xxx/MCP42xxx Digital Potentiometer driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for the Microchip
> +	  MCP41010, MCP41050, MCP41100,
> +	  MCP42010, MCP42050, MCP42100
> +	  digital potentiometer chips.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called mcp41010.
> +
>  config TPL0102
>  	tristate "Texas Instruments digital potentiometer driver"
>  	depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 4af657883c..8ff55138cf 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -11,4 +11,5 @@ obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4018) += mcp4018.o
>  obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
> +obj-$(CONFIG_MCP41010) += mcp41010.o
>  obj-$(CONFIG_TPL0102) += tpl0102.o
> diff --git a/drivers/iio/potentiometer/mcp41010.c b/drivers/iio/potentiometer/mcp41010.c
> new file mode 100644
> index 0000000000..4068e8eb57
> --- /dev/null
> +++ b/drivers/iio/potentiometer/mcp41010.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Industrial I/O driver for Microchip digital potentiometers
> + *
> + * Copyright (c) 2018 Chris Coffey <cmc@babblebit.net>
> + * Based on: Slawomir Stepien's code from mcp4131.c
> + *
> + * Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> + *
> + * DEVID	#Wipers	#Positions	Resistance (kOhm)
> + * mcp41010	1	256		10
> + * mcp41050	1	256		50
> + * mcp41100	1	256		100
> + * mcp42010	2	256		10
> + * mcp42050	2	256		50
> + * mcp42100	2	256		100
> + *
> + */
> +
> +#include <linux/cache.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +#define MCP41010_MAX_WIPERS	2
> +#define MCP41010_WRITE		(0x01 << 4)
> +#define MCP41010_WIPER_MAX	255
> +#define MCP41010_WIPER_ENABLE	BIT(0)
> +
> +struct mcp41010_cfg {
> +	int wipers;
> +	int kohms;
> +};
> +
> +enum mcp41010_type {
> +	MCP41010 = 0,
> +	MCP41050,
> +	MCP41100,
> +	MCP42010,
> +	MCP42050,
> +	MCP42100,
> +};
> +
> +static const struct mcp41010_cfg mcp41010_cfg[] = {
> +	[MCP41010] = { .wipers = 1, .kohms =  10, },
> +	[MCP41050] = { .wipers = 1, .kohms =  50, },
> +	[MCP41100] = { .wipers = 1, .kohms = 100, },
> +	[MCP42010] = { .wipers = 2, .kohms =  10, },
> +	[MCP42050] = { .wipers = 2, .kohms =  50, },
> +	[MCP42100] = { .wipers = 2, .kohms = 100, },
> +};
> +
> +struct mcp41010_data {
> +	struct spi_device *spi;
> +	const struct mcp41010_cfg *cfg;
> +	struct mutex lock; /* Protect write sequences */
> +	unsigned int value[MCP41010_MAX_WIPERS]; /* Cache wiper values */
> +	u8 buf[2] ____cacheline_aligned;
> +};
> +
> +#define MCP41010_CHANNEL(ch) {					\
> +	.type = IIO_RESISTANCE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (ch),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec mcp41010_channels[] = {
> +	MCP41010_CHANNEL(0),
> +	MCP41010_CHANNEL(1),
> +};
> +
> +static int mcp41010_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct mcp41010_data *data = iio_priv(indio_dev);
> +	int channel = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = data->value[channel];
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1000 * data->cfg->kohms;
> +		*val2 = MCP41010_WIPER_MAX;
> +		return IIO_VAL_FRACTIONAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mcp41010_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	int err;
> +	struct mcp41010_data *data = iio_priv(indio_dev);
> +	int channel = chan->channel;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val > MCP41010_WIPER_MAX || val < 0)
> +			return -EINVAL;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->lock);
> +
> +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> +	data->buf[0] |= MCP41010_WRITE;

Will this not clobber the other channel for mcp42xxx chips???

> +	data->buf[1] = val & 0xff;
> +
> +	err = spi_write(data->spi, data->buf, 2);
> +	if (!err)
> +		data->value[channel] = val;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return err;
> +}
> +
> +static const struct iio_info mcp41010_info = {
> +	.read_raw = mcp41010_read_raw,
> +	.write_raw = mcp41010_write_raw,
> +};
> +
> +static int mcp41010_probe(struct spi_device *spi)
> +{
> +	int err;
> +	struct device *dev = &spi->dev;
> +	unsigned long devid = spi_get_device_id(spi)->driver_data;
> +	struct mcp41010_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);
> +	data->spi = spi;
> +	data->cfg = &mcp41010_cfg[devid];

I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
well, but that's not a valid reason for not doing it here AFAICT...

Cheers,
Peter

> +
> +	mutex_init(&data->lock);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &mcp41010_info;
> +	indio_dev->channels = mcp41010_channels;
> +	indio_dev->num_channels = data->cfg->wipers;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +
> +	err = devm_iio_device_register(dev, indio_dev);
> +	if (err) {
> +		dev_info(&spi->dev, "Unable to register %s\n", indio_dev->name);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mcp41010_dt_ids[] = {
> +	{ .compatible = "microchip,mcp41010",
> +		.data = &mcp41010_cfg[MCP41010] },
> +	{ .compatible = "microchip,mcp41050",
> +		.data = &mcp41010_cfg[MCP41050] },
> +	{ .compatible = "microchip,mcp41100",
> +		.data = &mcp41010_cfg[MCP41100] },
> +	{ .compatible = "microchip,mcp42010",
> +		.data = &mcp41010_cfg[MCP42010] },
> +	{ .compatible = "microchip,mcp42050",
> +		.data = &mcp41010_cfg[MCP42050] },
> +	{ .compatible = "microchip,mcp42100",
> +		.data = &mcp41010_cfg[MCP42100] },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mcp41010_dt_ids);
> +#endif /* CONFIG_OF */
> +
> +static const struct spi_device_id mcp41010_id[] = {
> +	{ "mcp41010", MCP41010 },
> +	{ "mcp41050", MCP41050 },
> +	{ "mcp41100", MCP41100 },
> +	{ "mcp42010", MCP42010 },
> +	{ "mcp42050", MCP42050 },
> +	{ "mcp42100", MCP42100 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, mcp41010_id);
> +
> +static struct spi_driver mcp41010_driver = {
> +	.driver = {
> +		.name	= "mcp41010",
> +		.of_match_table = of_match_ptr(mcp41010_dt_ids),
> +	},
> +	.probe		= mcp41010_probe,
> +	.id_table	= mcp41010_id,
> +};
> +
> +module_spi_driver(mcp41010_driver);
> +
> +MODULE_AUTHOR("Chris Coffey <cmc@babblebit.net>");
> +MODULE_DESCRIPTION("MCP41010 digital potentiometer");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

SGkhDQoNClNvbWUgY29tbWVudHMgaW5saW5lLi4uDQoNCk9uIDIwMTgtMTEtMDYgMTI6MzEsIENo
cmlzIENvZmZleSB3cm90ZToNCj4gVGhpcyBwYXRjaCBhZGRzIGRyaXZlciBzdXBwb3J0IGZvciB0
aGUgTWljcm9jaGlwIE1DUDQxeHh4LzQyeHh4IGZhbWlseQ0KPiBvZiBkaWdpdGFsIHBvdGVudGlv
bWV0ZXJzOg0KPiANCj4gREVWSUNFICAgICAgV2lwZXJzICBQb3NpdGlvbnMgICBSZXNpc3RhbmNl
IChrT2htKQ0KPiBNQ1A0MTAxMCAgICAxICAgICAgIDI1NiAgICAgICAgIDEwDQo+IE1DUDQxMDUw
ICAgIDEgICAgICAgMjU2ICAgICAgICAgNTANCj4gTUNQNDExMDAgICAgMSAgICAgICAyNTYgICAg
ICAgICAxMDANCj4gTUNQNDIwMTAgICAgMiAgICAgICAyNTYgICAgICAgICAxMA0KPiBNQ1A0MjA1
MCAgICAyICAgICAgIDI1NiAgICAgICAgIDUwDQo+IE1DUDQyMTAwICAgIDIgICAgICAgMjU2ICAg
ICAgICAgMTAwDQo+IA0KPiBEYXRhc2hlZXQ6IGh0dHA6Ly93dzEubWljcm9jaGlwLmNvbS9kb3du
bG9hZHMvZW4vZGV2aWNlZG9jLzExMTk1Yy5wZGYNCj4gDQo+IFNpZ25lZC1vZmYtYnk6IENocmlz
IENvZmZleSA8Y21jQGJhYmJsZWJpdC5uZXQ+DQo+IC0tLQ0KPiAgLi4uL2JpbmRpbmdzL2lpby9w
b3RlbnRpb21ldGVyL21jcDQxMDEwLnR4dCAgICAgICAgfCAgMjkgKysrDQo+ICBkcml2ZXJzL2lp
by9wb3RlbnRpb21ldGVyL0tjb25maWcgICAgICAgICAgICAgICAgICB8ICAxMiArKw0KPiAgZHJp
dmVycy9paW8vcG90ZW50aW9tZXRlci9NYWtlZmlsZSAgICAgICAgICAgICAgICAgfCAgIDEgKw0K
PiAgZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9tY3A0MTAxMC5jICAgICAgICAgICAgICAgfCAy
MTYgKysrKysrKysrKysrKysrKysrKysrDQo+ICA0IGZpbGVzIGNoYW5nZWQsIDI1OCBpbnNlcnRp
b25zKCspDQo+ICBjcmVhdGUgbW9kZSAxMDA2NDQgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2Jp
bmRpbmdzL2lpby9wb3RlbnRpb21ldGVyL21jcDQxMDEwLnR4dA0KPiAgY3JlYXRlIG1vZGUgMTAw
NjQ0IGRyaXZlcnMvaWlvL3BvdGVudGlvbWV0ZXIvbWNwNDEwMTAuYw0KPiANCj4gZGlmZiAtLWdp
dCBhL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy9paW8vcG90ZW50aW9tZXRlci9t
Y3A0MTAxMC50eHQgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvaWlvL3BvdGVu
dGlvbWV0ZXIvbWNwNDEwMTAudHh0DQo+IG5ldyBmaWxlIG1vZGUgMTAwNjQ0DQo+IGluZGV4IDAw
MDAwMDAwMDAuLjE3NTY1YWNhY2UNCj4gLS0tIC9kZXYvbnVsbA0KPiArKysgYi9Eb2N1bWVudGF0
aW9uL2RldmljZXRyZWUvYmluZGluZ3MvaWlvL3BvdGVudGlvbWV0ZXIvbWNwNDEwMTAudHh0DQo+
IEBAIC0wLDAgKzEsMjkgQEANCj4gKyogTWljcm9jaGlwIE1DUDQxMDEwLzQxMDUwLzQxMTAwLzQy
MDEwLzQyMDUwLzQyMTAwIERpZ2l0YWwgUG90ZW50aW9tZXRlcg0KPiArICBkcml2ZXINCj4gKw0K
PiArRGF0YXNoZWV0IHB1YmxpY2x5IGF2YWlsYWJsZSBhdDoNCj4gK2h0dHA6Ly93dzEubWljcm9j
aGlwLmNvbS9kb3dubG9hZHMvZW4vZGV2aWNlZG9jLzExMTk1Yy5wZGYNCj4gKw0KPiArVGhlIG5v
ZGUgZm9yIHRoaXMgZHJpdmVyIG11c3QgYmUgYSBjaGlsZCBub2RlIG9mIGEgU1BJIGNvbnRyb2xs
ZXIsIGhlbmNlDQo+ICthbGwgbWFuZGF0b3J5IHByb3BlcnRpZXMgZGVzY3JpYmVkIGluDQo+ICsN
Cj4gKyAgICAgICAgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL3NwaS9zcGktYnVz
LnR4dA0KPiArDQo+ICttdXN0IGJlIHNwZWNpZmllZC4NCj4gKw0KPiArUmVxdWlyZWQgcHJvcGVy
dGllczoNCj4gKwktIGNvbXBhdGlibGU6ICAJTXVzdCBiZSBvbmUgb2YgdGhlIGZvbGxvd2luZywg
ZGVwZW5kaW5nIG9uIHRoZQ0KPiArCQkJbW9kZWw6DQo+ICsJCQkibWljcm9jaGlwLG1jcDQxMDEw
Ig0KPiArCQkJIm1pY3JvY2hpcCxtY3A0MTA1MCINCj4gKwkJCSJtaWNyb2NoaXAsbWNwNDExMDAi
DQo+ICsJCQkibWljcm9jaGlwLG1jcDQyMDEwIg0KPiArCQkJIm1pY3JvY2hpcCxtY3A0MjA1MCIN
Cj4gKwkJCSJtaWNyb2NoaXAsbWNwNDIxMDAiDQo+ICsNCj4gK0V4YW1wbGU6DQo+ICttY3A0MTAx
MDogcG90ZW50aW9tZXRlckAwIHsNCj4gKwljb21wYXRpYmxlID0gIm1jcDQxMDEwIjsNCj4gKwly
ZWcgPSA8MD47DQo+ICsJc3BpLW1heC1mcmVxdWVuY3kgPSA8NTAwMDAwPjsNCj4gK307DQo+IGRp
ZmYgLS1naXQgYS9kcml2ZXJzL2lpby9wb3RlbnRpb21ldGVyL0tjb25maWcgYi9kcml2ZXJzL2lp
by9wb3RlbnRpb21ldGVyL0tjb25maWcNCj4gaW5kZXggNzllYzJlYmE0OS4uNjMwM2NiZTc5OSAx
MDA2NDQNCj4gLS0tIGEvZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9LY29uZmlnDQo+ICsrKyBi
L2RyaXZlcnMvaWlvL3BvdGVudGlvbWV0ZXIvS2NvbmZpZw0KPiBAQCAtOTAsNiArOTAsMTggQEAg
Y29uZmlnIE1DUDQ1MzENCj4gIAkgIFRvIGNvbXBpbGUgdGhpcyBkcml2ZXIgYXMgYSBtb2R1bGUs
IGNob29zZSBNIGhlcmU6IHRoZQ0KPiAgCSAgbW9kdWxlIHdpbGwgYmUgY2FsbGVkIG1jcDQ1MzEu
DQo+ICANCj4gK2NvbmZpZyBNQ1A0MTAxMA0KPiArCXRyaXN0YXRlICJNaWNyb2NoaXAgTUNQNDF4
eHgvTUNQNDJ4eHggRGlnaXRhbCBQb3RlbnRpb21ldGVyIGRyaXZlciINCj4gKwlkZXBlbmRzIG9u
IFNQSQ0KPiArCWhlbHANCj4gKwkgIFNheSB5ZXMgaGVyZSB0byBidWlsZCBzdXBwb3J0IGZvciB0
aGUgTWljcm9jaGlwDQo+ICsJICBNQ1A0MTAxMCwgTUNQNDEwNTAsIE1DUDQxMTAwLA0KPiArCSAg
TUNQNDIwMTAsIE1DUDQyMDUwLCBNQ1A0MjEwMA0KPiArCSAgZGlnaXRhbCBwb3RlbnRpb21ldGVy
IGNoaXBzLg0KPiArDQo+ICsJICBUbyBjb21waWxlIHRoaXMgZHJpdmVyIGFzIGEgbW9kdWxlLCBj
aG9vc2UgTSBoZXJlOiB0aGUNCj4gKwkgIG1vZHVsZSB3aWxsIGJlIGNhbGxlZCBtY3A0MTAxMC4N
Cj4gKw0KPiAgY29uZmlnIFRQTDAxMDINCj4gIAl0cmlzdGF0ZSAiVGV4YXMgSW5zdHJ1bWVudHMg
ZGlnaXRhbCBwb3RlbnRpb21ldGVyIGRyaXZlciINCj4gIAlkZXBlbmRzIG9uIEkyQw0KPiBkaWZm
IC0tZ2l0IGEvZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9NYWtlZmlsZSBiL2RyaXZlcnMvaWlv
L3BvdGVudGlvbWV0ZXIvTWFrZWZpbGUNCj4gaW5kZXggNGFmNjU3ODgzYy4uOGZmNTUxMzhjZiAx
MDA2NDQNCj4gLS0tIGEvZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9NYWtlZmlsZQ0KPiArKysg
Yi9kcml2ZXJzL2lpby9wb3RlbnRpb21ldGVyL01ha2VmaWxlDQo+IEBAIC0xMSw0ICsxMSw1IEBA
IG9iai0kKENPTkZJR19NQVg1NDg3KSArPSBtYXg1NDg3Lm8NCj4gIG9iai0kKENPTkZJR19NQ1A0
MDE4KSArPSBtY3A0MDE4Lm8NCj4gIG9iai0kKENPTkZJR19NQ1A0MTMxKSArPSBtY3A0MTMxLm8N
Cj4gIG9iai0kKENPTkZJR19NQ1A0NTMxKSArPSBtY3A0NTMxLm8NCj4gK29iai0kKENPTkZJR19N
Q1A0MTAxMCkgKz0gbWNwNDEwMTAubw0KPiAgb2JqLSQoQ09ORklHX1RQTDAxMDIpICs9IHRwbDAx
MDIubw0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9tY3A0MTAxMC5j
IGIvZHJpdmVycy9paW8vcG90ZW50aW9tZXRlci9tY3A0MTAxMC5jDQo+IG5ldyBmaWxlIG1vZGUg
MTAwNjQ0DQo+IGluZGV4IDAwMDAwMDAwMDAuLjQwNjhlOGViNTcNCj4gLS0tIC9kZXYvbnVsbA0K
PiArKysgYi9kcml2ZXJzL2lpby9wb3RlbnRpb21ldGVyL21jcDQxMDEwLmMNCj4gQEAgLTAsMCAr
MSwyMTYgQEANCj4gKy8vIFNQRFgtTGljZW5zZS1JZGVudGlmaWVyOiBHUEwtMi4wDQo+ICsvKg0K
PiArICogSW5kdXN0cmlhbCBJL08gZHJpdmVyIGZvciBNaWNyb2NoaXAgZGlnaXRhbCBwb3RlbnRp
b21ldGVycw0KPiArICoNCj4gKyAqIENvcHlyaWdodCAoYykgMjAxOCBDaHJpcyBDb2ZmZXkgPGNt
Y0BiYWJibGViaXQubmV0Pg0KPiArICogQmFzZWQgb246IFNsYXdvbWlyIFN0ZXBpZW4ncyBjb2Rl
IGZyb20gbWNwNDEzMS5jDQo+ICsgKg0KPiArICogRGF0YXNoZWV0OiBodHRwOi8vd3cxLm1pY3Jv
Y2hpcC5jb20vZG93bmxvYWRzL2VuL2RldmljZWRvYy8xMTE5NWMucGRmDQo+ICsgKg0KPiArICog
REVWSUQJI1dpcGVycwkjUG9zaXRpb25zCVJlc2lzdGFuY2UgKGtPaG0pDQo+ICsgKiBtY3A0MTAx
MAkxCTI1NgkJMTANCj4gKyAqIG1jcDQxMDUwCTEJMjU2CQk1MA0KPiArICogbWNwNDExMDAJMQky
NTYJCTEwMA0KPiArICogbWNwNDIwMTAJMgkyNTYJCTEwDQo+ICsgKiBtY3A0MjA1MAkyCTI1NgkJ
NTANCj4gKyAqIG1jcDQyMTAwCTIJMjU2CQkxMDANCj4gKyAqDQo+ICsgKi8NCj4gKw0KPiArI2lu
Y2x1ZGUgPGxpbnV4L2NhY2hlLmg+DQo+ICsjaW5jbHVkZSA8bGludXgvZXJyLmg+DQo+ICsjaW5j
bHVkZSA8bGludXgvZXhwb3J0Lmg+DQo+ICsjaW5jbHVkZSA8bGludXgvaWlvL2lpby5oPg0KPiAr
I2luY2x1ZGUgPGxpbnV4L2lpby90eXBlcy5oPg0KPiArI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5o
Pg0KPiArI2luY2x1ZGUgPGxpbnV4L211dGV4Lmg+DQo+ICsjaW5jbHVkZSA8bGludXgvb2YuaD4N
Cj4gKyNpbmNsdWRlIDxsaW51eC9zcGkvc3BpLmg+DQo+ICsNCj4gKyNkZWZpbmUgTUNQNDEwMTBf
TUFYX1dJUEVSUwkyDQo+ICsjZGVmaW5lIE1DUDQxMDEwX1dSSVRFCQkoMHgwMSA8PCA0KQ0KPiAr
I2RlZmluZSBNQ1A0MTAxMF9XSVBFUl9NQVgJMjU1DQo+ICsjZGVmaW5lIE1DUDQxMDEwX1dJUEVS
X0VOQUJMRQlCSVQoMCkNCj4gKw0KPiArc3RydWN0IG1jcDQxMDEwX2NmZyB7DQo+ICsJaW50IHdp
cGVyczsNCj4gKwlpbnQga29obXM7DQo+ICt9Ow0KPiArDQo+ICtlbnVtIG1jcDQxMDEwX3R5cGUg
ew0KPiArCU1DUDQxMDEwID0gMCwNCj4gKwlNQ1A0MTA1MCwNCj4gKwlNQ1A0MTEwMCwNCj4gKwlN
Q1A0MjAxMCwNCj4gKwlNQ1A0MjA1MCwNCj4gKwlNQ1A0MjEwMCwNCj4gK307DQo+ICsNCj4gK3N0
YXRpYyBjb25zdCBzdHJ1Y3QgbWNwNDEwMTBfY2ZnIG1jcDQxMDEwX2NmZ1tdID0gew0KPiArCVtN
Q1A0MTAxMF0gPSB7IC53aXBlcnMgPSAxLCAua29obXMgPSAgMTAsIH0sDQo+ICsJW01DUDQxMDUw
XSA9IHsgLndpcGVycyA9IDEsIC5rb2htcyA9ICA1MCwgfSwNCj4gKwlbTUNQNDExMDBdID0geyAu
d2lwZXJzID0gMSwgLmtvaG1zID0gMTAwLCB9LA0KPiArCVtNQ1A0MjAxMF0gPSB7IC53aXBlcnMg
PSAyLCAua29obXMgPSAgMTAsIH0sDQo+ICsJW01DUDQyMDUwXSA9IHsgLndpcGVycyA9IDIsIC5r
b2htcyA9ICA1MCwgfSwNCj4gKwlbTUNQNDIxMDBdID0geyAud2lwZXJzID0gMiwgLmtvaG1zID0g
MTAwLCB9LA0KPiArfTsNCj4gKw0KPiArc3RydWN0IG1jcDQxMDEwX2RhdGEgew0KPiArCXN0cnVj
dCBzcGlfZGV2aWNlICpzcGk7DQo+ICsJY29uc3Qgc3RydWN0IG1jcDQxMDEwX2NmZyAqY2ZnOw0K
PiArCXN0cnVjdCBtdXRleCBsb2NrOyAvKiBQcm90ZWN0IHdyaXRlIHNlcXVlbmNlcyAqLw0KPiAr
CXVuc2lnbmVkIGludCB2YWx1ZVtNQ1A0MTAxMF9NQVhfV0lQRVJTXTsgLyogQ2FjaGUgd2lwZXIg
dmFsdWVzICovDQo+ICsJdTggYnVmWzJdIF9fX19jYWNoZWxpbmVfYWxpZ25lZDsNCj4gK307DQo+
ICsNCj4gKyNkZWZpbmUgTUNQNDEwMTBfQ0hBTk5FTChjaCkgewkJCQkJXA0KPiArCS50eXBlID0g
SUlPX1JFU0lTVEFOQ0UsCQkJCQlcDQo+ICsJLmluZGV4ZWQgPSAxLAkJCQkJCVwNCj4gKwkub3V0
cHV0ID0gMSwJCQkJCQlcDQo+ICsJLmNoYW5uZWwgPSAoY2gpLAkJCQkJXA0KPiArCS5pbmZvX21h
c2tfc2VwYXJhdGUgPSBCSVQoSUlPX0NIQU5fSU5GT19SQVcpLAkJXA0KPiArCS5pbmZvX21hc2tf
c2hhcmVkX2J5X3R5cGUgPSBCSVQoSUlPX0NIQU5fSU5GT19TQ0FMRSksCVwNCj4gK30NCj4gKw0K
PiArc3RhdGljIGNvbnN0IHN0cnVjdCBpaW9fY2hhbl9zcGVjIG1jcDQxMDEwX2NoYW5uZWxzW10g
PSB7DQo+ICsJTUNQNDEwMTBfQ0hBTk5FTCgwKSwNCj4gKwlNQ1A0MTAxMF9DSEFOTkVMKDEpLA0K
PiArfTsNCj4gKw0KPiArc3RhdGljIGludCBtY3A0MTAxMF9yZWFkX3JhdyhzdHJ1Y3QgaWlvX2Rl
diAqaW5kaW9fZGV2LA0KPiArCQkJICAgIHN0cnVjdCBpaW9fY2hhbl9zcGVjIGNvbnN0ICpjaGFu
LA0KPiArCQkJICAgIGludCAqdmFsLCBpbnQgKnZhbDIsIGxvbmcgbWFzaykNCj4gK3sNCj4gKwlz
dHJ1Y3QgbWNwNDEwMTBfZGF0YSAqZGF0YSA9IGlpb19wcml2KGluZGlvX2Rldik7DQo+ICsJaW50
IGNoYW5uZWwgPSBjaGFuLT5jaGFubmVsOw0KPiArDQo+ICsJc3dpdGNoIChtYXNrKSB7DQo+ICsJ
Y2FzZSBJSU9fQ0hBTl9JTkZPX1JBVzoNCj4gKwkJKnZhbCA9IGRhdGEtPnZhbHVlW2NoYW5uZWxd
Ow0KPiArCQlyZXR1cm4gSUlPX1ZBTF9JTlQ7DQo+ICsNCj4gKwljYXNlIElJT19DSEFOX0lORk9f
U0NBTEU6DQo+ICsJCSp2YWwgPSAxMDAwICogZGF0YS0+Y2ZnLT5rb2htczsNCj4gKwkJKnZhbDIg
PSBNQ1A0MTAxMF9XSVBFUl9NQVg7DQo+ICsJCXJldHVybiBJSU9fVkFMX0ZSQUNUSU9OQUw7DQo+
ICsJfQ0KPiArDQo+ICsJcmV0dXJuIC1FSU5WQUw7DQo+ICt9DQo+ICsNCj4gK3N0YXRpYyBpbnQg
bWNwNDEwMTBfd3JpdGVfcmF3KHN0cnVjdCBpaW9fZGV2ICppbmRpb19kZXYsDQo+ICsJCQkgICAg
IHN0cnVjdCBpaW9fY2hhbl9zcGVjIGNvbnN0ICpjaGFuLA0KPiArCQkJICAgICBpbnQgdmFsLCBp
bnQgdmFsMiwgbG9uZyBtYXNrKQ0KPiArew0KPiArCWludCBlcnI7DQo+ICsJc3RydWN0IG1jcDQx
MDEwX2RhdGEgKmRhdGEgPSBpaW9fcHJpdihpbmRpb19kZXYpOw0KPiArCWludCBjaGFubmVsID0g
Y2hhbi0+Y2hhbm5lbDsNCj4gKw0KPiArCXN3aXRjaCAobWFzaykgew0KPiArCWNhc2UgSUlPX0NI
QU5fSU5GT19SQVc6DQo+ICsJCWlmICh2YWwgPiBNQ1A0MTAxMF9XSVBFUl9NQVggfHwgdmFsIDwg
MCkNCj4gKwkJCXJldHVybiAtRUlOVkFMOw0KPiArCQlicmVhazsNCj4gKw0KPiArCWRlZmF1bHQ6
DQo+ICsJCXJldHVybiAtRUlOVkFMOw0KPiArCX0NCj4gKw0KPiArCW11dGV4X2xvY2soJmRhdGEt
PmxvY2spOw0KPiArDQo+ICsJZGF0YS0+YnVmWzBdID0gTUNQNDEwMTBfV0lQRVJfRU5BQkxFIDw8
IGNoYW5uZWw7DQo+ICsJZGF0YS0+YnVmWzBdIHw9IE1DUDQxMDEwX1dSSVRFOw0KDQpXaWxsIHRo
aXMgbm90IGNsb2JiZXIgdGhlIG90aGVyIGNoYW5uZWwgZm9yIG1jcDQyeHh4IGNoaXBzPz8/DQoN
Cj4gKwlkYXRhLT5idWZbMV0gPSB2YWwgJiAweGZmOw0KPiArDQo+ICsJZXJyID0gc3BpX3dyaXRl
KGRhdGEtPnNwaSwgZGF0YS0+YnVmLCAyKTsNCj4gKwlpZiAoIWVycikNCj4gKwkJZGF0YS0+dmFs
dWVbY2hhbm5lbF0gPSB2YWw7DQo+ICsNCj4gKwltdXRleF91bmxvY2soJmRhdGEtPmxvY2spOw0K
PiArDQo+ICsJcmV0dXJuIGVycjsNCj4gK30NCj4gKw0KPiArc3RhdGljIGNvbnN0IHN0cnVjdCBp
aW9faW5mbyBtY3A0MTAxMF9pbmZvID0gew0KPiArCS5yZWFkX3JhdyA9IG1jcDQxMDEwX3JlYWRf
cmF3LA0KPiArCS53cml0ZV9yYXcgPSBtY3A0MTAxMF93cml0ZV9yYXcsDQo+ICt9Ow0KPiArDQo+
ICtzdGF0aWMgaW50IG1jcDQxMDEwX3Byb2JlKHN0cnVjdCBzcGlfZGV2aWNlICpzcGkpDQo+ICt7
DQo+ICsJaW50IGVycjsNCj4gKwlzdHJ1Y3QgZGV2aWNlICpkZXYgPSAmc3BpLT5kZXY7DQo+ICsJ
dW5zaWduZWQgbG9uZyBkZXZpZCA9IHNwaV9nZXRfZGV2aWNlX2lkKHNwaSktPmRyaXZlcl9kYXRh
Ow0KPiArCXN0cnVjdCBtY3A0MTAxMF9kYXRhICpkYXRhOw0KPiArCXN0cnVjdCBpaW9fZGV2ICpp
bmRpb19kZXY7DQo+ICsNCj4gKwlpbmRpb19kZXYgPSBkZXZtX2lpb19kZXZpY2VfYWxsb2MoZGV2
LCBzaXplb2YoKmRhdGEpKTsNCj4gKwlpZiAoIWluZGlvX2RldikNCj4gKwkJcmV0dXJuIC1FTk9N
RU07DQo+ICsNCj4gKwlkYXRhID0gaWlvX3ByaXYoaW5kaW9fZGV2KTsNCj4gKwlzcGlfc2V0X2Ry
dmRhdGEoc3BpLCBpbmRpb19kZXYpOw0KPiArCWRhdGEtPnNwaSA9IHNwaTsNCj4gKwlkYXRhLT5j
ZmcgPSAmbWNwNDEwMTBfY2ZnW2RldmlkXTsNCg0KSSdtIG1pc3NpbmcgYSAib2ZfZGV2aWNlX2dl
dF9tYXRjaF9kYXRhKGRldikiIGNhbGwgc29tZXdoZXJlIGluIGhlcmUsIHNlZSBlLmcuIHRoZQ0K
bWF4NTQ4MS5jIGZpbGUgaW4gdGhpcyBkaXJlY3RvcnkuIFllcywgdGhhdCdzIG1pc3NpbmcgZnJv
bSB0aGUgbWNwNDEzMSBkcml2ZXIgYXMNCndlbGwsIGJ1dCB0aGF0J3Mgbm90IGEgdmFsaWQgcmVh
c29uIGZvciBub3QgZG9pbmcgaXQgaGVyZSBBRkFJQ1QuLi4NCg0KQ2hlZXJzLA0KUGV0ZXINCg0K
PiArDQo+ICsJbXV0ZXhfaW5pdCgmZGF0YS0+bG9jayk7DQo+ICsNCj4gKwlpbmRpb19kZXYtPmRl
di5wYXJlbnQgPSBkZXY7DQo+ICsJaW5kaW9fZGV2LT5pbmZvID0gJm1jcDQxMDEwX2luZm87DQo+
ICsJaW5kaW9fZGV2LT5jaGFubmVscyA9IG1jcDQxMDEwX2NoYW5uZWxzOw0KPiArCWluZGlvX2Rl
di0+bnVtX2NoYW5uZWxzID0gZGF0YS0+Y2ZnLT53aXBlcnM7DQo+ICsJaW5kaW9fZGV2LT5uYW1l
ID0gc3BpX2dldF9kZXZpY2VfaWQoc3BpKS0+bmFtZTsNCj4gKw0KPiArCWVyciA9IGRldm1faWlv
X2RldmljZV9yZWdpc3RlcihkZXYsIGluZGlvX2Rldik7DQo+ICsJaWYgKGVycikgew0KPiArCQlk
ZXZfaW5mbygmc3BpLT5kZXYsICJVbmFibGUgdG8gcmVnaXN0ZXIgJXNcbiIsIGluZGlvX2Rldi0+
bmFtZSk7DQo+ICsJCXJldHVybiBlcnI7DQo+ICsJfQ0KPiArDQo+ICsJcmV0dXJuIDA7DQo+ICt9
DQo+ICsNCj4gKyNpZiBkZWZpbmVkKENPTkZJR19PRikNCj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3Qg
b2ZfZGV2aWNlX2lkIG1jcDQxMDEwX2R0X2lkc1tdID0gew0KPiArCXsgLmNvbXBhdGlibGUgPSAi
bWljcm9jaGlwLG1jcDQxMDEwIiwNCj4gKwkJLmRhdGEgPSAmbWNwNDEwMTBfY2ZnW01DUDQxMDEw
XSB9LA0KPiArCXsgLmNvbXBhdGlibGUgPSAibWljcm9jaGlwLG1jcDQxMDUwIiwNCj4gKwkJLmRh
dGEgPSAmbWNwNDEwMTBfY2ZnW01DUDQxMDUwXSB9LA0KPiArCXsgLmNvbXBhdGlibGUgPSAibWlj
cm9jaGlwLG1jcDQxMTAwIiwNCj4gKwkJLmRhdGEgPSAmbWNwNDEwMTBfY2ZnW01DUDQxMTAwXSB9
LA0KPiArCXsgLmNvbXBhdGlibGUgPSAibWljcm9jaGlwLG1jcDQyMDEwIiwNCj4gKwkJLmRhdGEg
PSAmbWNwNDEwMTBfY2ZnW01DUDQyMDEwXSB9LA0KPiArCXsgLmNvbXBhdGlibGUgPSAibWljcm9j
aGlwLG1jcDQyMDUwIiwNCj4gKwkJLmRhdGEgPSAmbWNwNDEwMTBfY2ZnW01DUDQyMDUwXSB9LA0K
PiArCXsgLmNvbXBhdGlibGUgPSAibWljcm9jaGlwLG1jcDQyMTAwIiwNCj4gKwkJLmRhdGEgPSAm
bWNwNDEwMTBfY2ZnW01DUDQyMTAwXSB9LA0KPiArCXt9DQo+ICt9Ow0KPiArTU9EVUxFX0RFVklD
RV9UQUJMRShvZiwgbWNwNDEwMTBfZHRfaWRzKTsNCj4gKyNlbmRpZiAvKiBDT05GSUdfT0YgKi8N
Cj4gKw0KPiArc3RhdGljIGNvbnN0IHN0cnVjdCBzcGlfZGV2aWNlX2lkIG1jcDQxMDEwX2lkW10g
PSB7DQo+ICsJeyAibWNwNDEwMTAiLCBNQ1A0MTAxMCB9LA0KPiArCXsgIm1jcDQxMDUwIiwgTUNQ
NDEwNTAgfSwNCj4gKwl7ICJtY3A0MTEwMCIsIE1DUDQxMTAwIH0sDQo+ICsJeyAibWNwNDIwMTAi
LCBNQ1A0MjAxMCB9LA0KPiArCXsgIm1jcDQyMDUwIiwgTUNQNDIwNTAgfSwNCj4gKwl7ICJtY3A0
MjEwMCIsIE1DUDQyMTAwIH0sDQo+ICsJe30NCj4gK307DQo+ICtNT0RVTEVfREVWSUNFX1RBQkxF
KHNwaSwgbWNwNDEwMTBfaWQpOw0KPiArDQo+ICtzdGF0aWMgc3RydWN0IHNwaV9kcml2ZXIgbWNw
NDEwMTBfZHJpdmVyID0gew0KPiArCS5kcml2ZXIgPSB7DQo+ICsJCS5uYW1lCT0gIm1jcDQxMDEw
IiwNCj4gKwkJLm9mX21hdGNoX3RhYmxlID0gb2ZfbWF0Y2hfcHRyKG1jcDQxMDEwX2R0X2lkcyks
DQo+ICsJfSwNCj4gKwkucHJvYmUJCT0gbWNwNDEwMTBfcHJvYmUsDQo+ICsJLmlkX3RhYmxlCT0g
bWNwNDEwMTBfaWQsDQo+ICt9Ow0KPiArDQo+ICttb2R1bGVfc3BpX2RyaXZlcihtY3A0MTAxMF9k
cml2ZXIpOw0KPiArDQo+ICtNT0RVTEVfQVVUSE9SKCJDaHJpcyBDb2ZmZXkgPGNtY0BiYWJibGVi
aXQubmV0PiIpOw0KPiArTU9EVUxFX0RFU0NSSVBUSU9OKCJNQ1A0MTAxMCBkaWdpdGFsIHBvdGVu
dGlvbWV0ZXIiKTsNCj4gK01PRFVMRV9MSUNFTlNFKCJHUEwgdjIiKTsNCj4gDQoNCg==

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

On 2018-11-06 17:37, Peter Rosin wrote:
> Hi!
> 
> Some comments inline...
> 
>> +
>> +static int mcp41010_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	int err;
>> +	struct mcp41010_data *data = iio_priv(indio_dev);
>> +	int channel = chan->channel;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (val > MCP41010_WIPER_MAX || val < 0)
>> +			return -EINVAL;
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
>> +	data->buf[0] |= MCP41010_WRITE;
> 
> Will this not clobber the other channel for mcp42xxx chips???


I had a peak in the datasheet, and no, it will not. It was just the naming
with ..._WIPER_ENABLE that threw me off. It assumed, from the naming, that
each channel has a separate enable bit in the command byte and that you
were required to keep the state of those intact for each and every command
you sent. After looking at the datasheet, that is obviously not the case,
and the code is fine. But may I ask for a change in naming here?

MCP41010_WIPER_CHANNEL instead of MCP41010_WIPER_ENABLE perhaps?

Cheers,
Peter

> 
>> +	data->buf[1] = val & 0xff;
>> +
>> +	err = spi_write(data->spi, data->buf, 2);
>> +	if (!err)
>> +		data->value[channel] = val;
>> +
>> +	mutex_unlock(&data->lock);
>> +
>> +	return err;
>> +}

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

T24gMjAxOC0xMS0wNiAxNzozNywgUGV0ZXIgUm9zaW4gd3JvdGU6DQo+IEhpIQ0KPiANCj4gU29t
ZSBjb21tZW50cyBpbmxpbmUuLi4NCj4gDQo+PiArDQo+PiArc3RhdGljIGludCBtY3A0MTAxMF93
cml0ZV9yYXcoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiwNCj4+ICsJCQkgICAgIHN0cnVjdCBp
aW9fY2hhbl9zcGVjIGNvbnN0ICpjaGFuLA0KPj4gKwkJCSAgICAgaW50IHZhbCwgaW50IHZhbDIs
IGxvbmcgbWFzaykNCj4+ICt7DQo+PiArCWludCBlcnI7DQo+PiArCXN0cnVjdCBtY3A0MTAxMF9k
YXRhICpkYXRhID0gaWlvX3ByaXYoaW5kaW9fZGV2KTsNCj4+ICsJaW50IGNoYW5uZWwgPSBjaGFu
LT5jaGFubmVsOw0KPj4gKw0KPj4gKwlzd2l0Y2ggKG1hc2spIHsNCj4+ICsJY2FzZSBJSU9fQ0hB
Tl9JTkZPX1JBVzoNCj4+ICsJCWlmICh2YWwgPiBNQ1A0MTAxMF9XSVBFUl9NQVggfHwgdmFsIDwg
MCkNCj4+ICsJCQlyZXR1cm4gLUVJTlZBTDsNCj4+ICsJCWJyZWFrOw0KPj4gKw0KPj4gKwlkZWZh
dWx0Og0KPj4gKwkJcmV0dXJuIC1FSU5WQUw7DQo+PiArCX0NCj4+ICsNCj4+ICsJbXV0ZXhfbG9j
aygmZGF0YS0+bG9jayk7DQo+PiArDQo+PiArCWRhdGEtPmJ1ZlswXSA9IE1DUDQxMDEwX1dJUEVS
X0VOQUJMRSA8PCBjaGFubmVsOw0KPj4gKwlkYXRhLT5idWZbMF0gfD0gTUNQNDEwMTBfV1JJVEU7
DQo+IA0KPiBXaWxsIHRoaXMgbm90IGNsb2JiZXIgdGhlIG90aGVyIGNoYW5uZWwgZm9yIG1jcDQy
eHh4IGNoaXBzPz8/DQoNCg0KSSBoYWQgYSBwZWFrIGluIHRoZSBkYXRhc2hlZXQsIGFuZCBubywg
aXQgd2lsbCBub3QuIEl0IHdhcyBqdXN0IHRoZSBuYW1pbmcNCndpdGggLi4uX1dJUEVSX0VOQUJM
RSB0aGF0IHRocmV3IG1lIG9mZi4gSXQgYXNzdW1lZCwgZnJvbSB0aGUgbmFtaW5nLCB0aGF0DQpl
YWNoIGNoYW5uZWwgaGFzIGEgc2VwYXJhdGUgZW5hYmxlIGJpdCBpbiB0aGUgY29tbWFuZCBieXRl
IGFuZCB0aGF0IHlvdQ0Kd2VyZSByZXF1aXJlZCB0byBrZWVwIHRoZSBzdGF0ZSBvZiB0aG9zZSBp
bnRhY3QgZm9yIGVhY2ggYW5kIGV2ZXJ5IGNvbW1hbmQNCnlvdSBzZW50LiBBZnRlciBsb29raW5n
IGF0IHRoZSBkYXRhc2hlZXQsIHRoYXQgaXMgb2J2aW91c2x5IG5vdCB0aGUgY2FzZSwNCmFuZCB0
aGUgY29kZSBpcyBmaW5lLiBCdXQgbWF5IEkgYXNrIGZvciBhIGNoYW5nZSBpbiBuYW1pbmcgaGVy
ZT8NCg0KTUNQNDEwMTBfV0lQRVJfQ0hBTk5FTCBpbnN0ZWFkIG9mIE1DUDQxMDEwX1dJUEVSX0VO
QUJMRSBwZXJoYXBzPw0KDQpDaGVlcnMsDQpQZXRlcg0KDQo+IA0KPj4gKwlkYXRhLT5idWZbMV0g
PSB2YWwgJiAweGZmOw0KPj4gKw0KPj4gKwllcnIgPSBzcGlfd3JpdGUoZGF0YS0+c3BpLCBkYXRh
LT5idWYsIDIpOw0KPj4gKwlpZiAoIWVycikNCj4+ICsJCWRhdGEtPnZhbHVlW2NoYW5uZWxdID0g
dmFsOw0KPj4gKw0KPj4gKwltdXRleF91bmxvY2soJmRhdGEtPmxvY2spOw0KPj4gKw0KPj4gKwly
ZXR1cm4gZXJyOw0KPj4gK30NCg==

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Chris Coffey]

Thank you for the review. I have a question inline.

On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote:
> On Tue, Nov 06, 2018 at 11:31:30AM +0000, Chris Coffey wrote:
> > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > of digital potentiometers:
> > 
> > DEVICE      Wipers  Positions   Resistance (kOhm)
> > MCP41010    1       256         10
> > MCP41050    1       256         50
> > MCP41100    1       256         100
> > MCP42010    2       256         10
> > MCP42050    2       256         50
> > MCP42100    2       256         100
> > 
> > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > 
> > Signed-off-by: Chris Coffey <cmc@babblebit.net>
> > ---
> >  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++
> 
> WARNING: DT binding docs and includes should be a separate patch. 
> See: Documentation/devicetree/bindings/submitting-patches.txt
> 
> Please run checkpatch.pl on this patch once.
> 

Oops. I ran checkpatch.pl on the individual files, but neglected to run
it on the patch file itself. I'll split the patch in v2.

checkpatch.pl also reports a warning about the MAINTAINERS file:
WARNING: added, moved or deleted file(s), does MAINTAINERS need
updating?

... but looking through the tree, many IIO drivers don't have
corresponding entries in MAINTAINERS; is this one of those checkpatch
warnings that can be safely ignored? Or should all new drivers have a
corresponding entry in MAINTAINERS?

Thanks again,
Chris

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Chris Coffey]

Thank you for the review! A few comments inline.

On Tue, Nov 06, 2018 at 04:37:11PM +0000, Peter Rosin wrote:
> Hi!
> 
> Some comments inline...
> 
> On 2018-11-06 12:31, Chris Coffey wrote:
> > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > of digital potentiometers:
> > 

[snip]

> > +
> > +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> > +	data->buf[0] |= MCP41010_WRITE;
> 
> Will this not clobber the other channel for mcp42xxx chips???
> 

I see you followed up on this in another message, so I'll respond there.

[snip]

> > +static int mcp41010_probe(struct spi_device *spi)
> > +{
> > +	int err;
> > +	struct device *dev = &spi->dev;
> > +	unsigned long devid = spi_get_device_id(spi)->driver_data;
> > +	struct mcp41010_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	spi_set_drvdata(spi, indio_dev);
> > +	data->spi = spi;
> > +	data->cfg = &mcp41010_cfg[devid];
> 
> I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
> max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
> well, but that's not a valid reason for not doing it here AFAICT...
> 
> Cheers,
> Peter
> 

Ah, interesting; I was unaware of of_device_get_match_data(). I'll add
that for v2.

Thanks again,
Chris

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Chris Coffey]

On Tue, Nov 06, 2018 at 08:08:15PM +0000, Peter Rosin wrote:
> On 2018-11-06 17:37, Peter Rosin wrote:
> > Hi!
> > 
> > Some comments inline...
> > 

[snip]

> >> +
> >> +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> >> +	data->buf[0] |= MCP41010_WRITE;
> > 
> > Will this not clobber the other channel for mcp42xxx chips???
> 
> 
> I had a peak in the datasheet, and no, it will not. It was just the naming
> with ..._WIPER_ENABLE that threw me off. It assumed, from the naming, that
> each channel has a separate enable bit in the command byte and that you
> were required to keep the state of those intact for each and every command
> you sent. After looking at the datasheet, that is obviously not the case,
> and the code is fine. But may I ask for a change in naming here?
> 
> MCP41010_WIPER_CHANNEL instead of MCP41010_WIPER_ENABLE perhaps?
> 
> Cheers,
> Peter
> 

I agree -- my naming for this is indeed ambiguous/confusing. I'll
change it for v2.

Thanks again,
Chris

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Himanshu Jha]

On Wed, Nov 07, 2018 at 09:05:50AM +0000, Chris Coffey wrote:
> Thank you for the review. I have a question inline.
> 
> On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote:
> > On Tue, Nov 06, 2018 at 11:31:30AM +0000, Chris Coffey wrote:
> > > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > > of digital potentiometers:
> > > 
> > > DEVICE      Wipers  Positions   Resistance (kOhm)
> > > MCP41010    1       256         10
> > > MCP41050    1       256         50
> > > MCP41100    1       256         100
> > > MCP42010    2       256         10
> > > MCP42050    2       256         50
> > > MCP42100    2       256         100
> > > 
> > > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > > 
> > > Signed-off-by: Chris Coffey <cmc@babblebit.net>
> > > ---
> > >  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++
> > 
> > WARNING: DT binding docs and includes should be a separate patch. 
> > See: Documentation/devicetree/bindings/submitting-patches.txt
> > 
> > Please run checkpatch.pl on this patch once.
> > 
> 
> Oops. I ran checkpatch.pl on the individual files, but neglected to run
> it on the patch file itself. I'll split the patch in v2.

No problem.

It is better for DT maintainers, and they get less spam. It was recent
effort by Rob IIRC,

"133712a2ec84 checkpatch: DT bindings should be a separate patch"

When you run on source file use `-f` flag and on patch just naked
run is fine.

> checkpatch.pl also reports a warning about the MAINTAINERS file:
> WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?
> 
> ... but looking through the tree, many IIO drivers don't have
> corresponding entries in MAINTAINERS; is this one of those checkpatch
> warnings that can be safely ignored? Or should all new drivers have a
> corresponding entry in MAINTAINERS?

It's upto you actually. 
Not a necessity to be a maintainer.

But when you add your name to MAINTAINERS you would need to review the
patches sent on the driver and maybe test it too.

And if you're not sure, then leave it as-is!

Without the entry in MAINTAINERS too, you can review/test patches
sent on your driver(which is what I do ..)

Obligations! ;)

-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Jonathan Cameron]

On Wed, 7 Nov 2018 15:18:08 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Wed, Nov 07, 2018 at 09:05:50AM +0000, Chris Coffey wrote:
> > Thank you for the review. I have a question inline.
> > 
> > On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote:  
> > > On Tue, Nov 06, 2018 at 11:31:30AM +0000, Chris Coffey wrote:  
> > > > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > > > of digital potentiometers:
> > > > 
> > > > DEVICE      Wipers  Positions   Resistance (kOhm)
> > > > MCP41010    1       256         10
> > > > MCP41050    1       256         50
> > > > MCP41100    1       256         100
> > > > MCP42010    2       256         10
> > > > MCP42050    2       256         50
> > > > MCP42100    2       256         100
> > > > 
> > > > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > > > 
> > > > Signed-off-by: Chris Coffey <cmc@babblebit.net>
> > > > ---
> > > >  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++  
> > > 
> > > WARNING: DT binding docs and includes should be a separate patch. 
> > > See: Documentation/devicetree/bindings/submitting-patches.txt
> > > 
> > > Please run checkpatch.pl on this patch once.
> > >   
> > 
> > Oops. I ran checkpatch.pl on the individual files, but neglected to run
> > it on the patch file itself. I'll split the patch in v2.  
> 
> No problem.
> 
> It is better for DT maintainers, and they get less spam. It was recent
> effort by Rob IIRC,
> 
> "133712a2ec84 checkpatch: DT bindings should be a separate patch"
> 
> When you run on source file use `-f` flag and on patch just naked
> run is fine.
> 
> > checkpatch.pl also reports a warning about the MAINTAINERS file:
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need
> > updating?
> > 
> > ... but looking through the tree, many IIO drivers don't have
> > corresponding entries in MAINTAINERS; is this one of those checkpatch
> > warnings that can be safely ignored? Or should all new drivers have a
> > corresponding entry in MAINTAINERS?  
> 
> It's upto you actually. 
> Not a necessity to be a maintainer.
> 
> But when you add your name to MAINTAINERS you would need to review the
> patches sent on the driver and maybe test it too.
> 
> And if you're not sure, then leave it as-is!
> 
> Without the entry in MAINTAINERS too, you can review/test patches
> sent on your driver(which is what I do ..)
> 
> Obligations! ;)

Yes, mostly MAINTAINERS entries are needed when it's not obvious
from the driver who should be looking at patches.  Often this is
when someone else had taken it over later, or you want all patches
to be cc'd to a list of relevant people (so company mailing lists
for example).  If people want to put themselves in MAINTAINERS
then we never mind, but personally I don't see why every single
driver needs to be listed.

Jonathan

> 

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Jonathan Cameron]

On Tue, 6 Nov 2018 16:37:11 +0000
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> Some comments inline...
A few additions from me.

thanks,

Jonathan

> 
> On 2018-11-06 12:31, Chris Coffey wrote:
> > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > of digital potentiometers:
> > 
> > DEVICE      Wipers  Positions   Resistance (kOhm)
> > MCP41010    1       256         10
> > MCP41050    1       256         50
> > MCP41100    1       256         100
> > MCP42010    2       256         10
> > MCP42050    2       256         50
> > MCP42100    2       256         100
> > 
> > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > 
> > Signed-off-by: Chris Coffey <cmc@babblebit.net>
> > ---
> >  .../bindings/iio/potentiometer/mcp41010.txt        |  29 +++
> >  drivers/iio/potentiometer/Kconfig                  |  12 ++
> >  drivers/iio/potentiometer/Makefile                 |   1 +
> >  drivers/iio/potentiometer/mcp41010.c               | 216 +++++++++++++++++++++
> >  4 files changed, 258 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
> >  create mode 100644 drivers/iio/potentiometer/mcp41010.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
> > new file mode 100644
> > index 0000000000..17565acace
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt
> > @@ -0,0 +1,29 @@
> > +* Microchip MCP41010/41050/41100/42010/42050/42100 Digital Potentiometer
> > +  driver
> > +
> > +Datasheet publicly available at:
> > +http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > +
> > +The node for this driver must be a child node of a SPI controller, hence
> > +all mandatory properties described in
> > +
> > +        Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +must be specified.
> > +
> > +Required properties:
> > +	- compatible:  	Must be one of the following, depending on the
> > +			model:
> > +			"microchip,mcp41010"
> > +			"microchip,mcp41050"
> > +			"microchip,mcp41100"
> > +			"microchip,mcp42010"
> > +			"microchip,mcp42050"
> > +			"microchip,mcp42100"
> > +
> > +Example:
> > +mcp41010: potentiometer@0 {
> > +	compatible = "mcp41010";
> > +	reg = <0>;
> > +	spi-max-frequency = <500000>;
> > +};
> > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> > index 79ec2eba49..6303cbe799 100644
> > --- a/drivers/iio/potentiometer/Kconfig
> > +++ b/drivers/iio/potentiometer/Kconfig
> > @@ -90,6 +90,18 @@ config MCP4531
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called mcp4531.
> >  
> > +config MCP41010
> > +	tristate "Microchip MCP41xxx/MCP42xxx Digital Potentiometer driver"
> > +	depends on SPI
> > +	help
> > +	  Say yes here to build support for the Microchip
> > +	  MCP41010, MCP41050, MCP41100,
> > +	  MCP42010, MCP42050, MCP42100
> > +	  digital potentiometer chips.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called mcp41010.
> > +
> >  config TPL0102
> >  	tristate "Texas Instruments digital potentiometer driver"
> >  	depends on I2C
> > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> > index 4af657883c..8ff55138cf 100644
> > --- a/drivers/iio/potentiometer/Makefile
> > +++ b/drivers/iio/potentiometer/Makefile
> > @@ -11,4 +11,5 @@ obj-$(CONFIG_MAX5487) += max5487.o
> >  obj-$(CONFIG_MCP4018) += mcp4018.o
> >  obj-$(CONFIG_MCP4131) += mcp4131.o
> >  obj-$(CONFIG_MCP4531) += mcp4531.o
> > +obj-$(CONFIG_MCP41010) += mcp41010.o
> >  obj-$(CONFIG_TPL0102) += tpl0102.o
> > diff --git a/drivers/iio/potentiometer/mcp41010.c b/drivers/iio/potentiometer/mcp41010.c
> > new file mode 100644
> > index 0000000000..4068e8eb57
> > --- /dev/null
> > +++ b/drivers/iio/potentiometer/mcp41010.c
> > @@ -0,0 +1,216 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Industrial I/O driver for Microchip digital potentiometers
> > + *
> > + * Copyright (c) 2018 Chris Coffey <cmc@babblebit.net>
> > + * Based on: Slawomir Stepien's code from mcp4131.c
> > + *
> > + * Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf
> > + *
> > + * DEVID	#Wipers	#Positions	Resistance (kOhm)
> > + * mcp41010	1	256		10
> > + * mcp41050	1	256		50
> > + * mcp41100	1	256		100
> > + * mcp42010	2	256		10
> > + * mcp42050	2	256		50
> > + * mcp42100	2	256		100
> > + *
nitpick!  No point in having this blank line.
> > + */
> > +
> > +#include <linux/cache.h>
> > +#include <linux/err.h>
> > +#include <linux/export.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/types.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define MCP41010_MAX_WIPERS	2
> > +#define MCP41010_WRITE		(0x01 << 4)
> > +#define MCP41010_WIPER_MAX	255
> > +#define MCP41010_WIPER_ENABLE	BIT(0)
> > +
> > +struct mcp41010_cfg {
> > +	int wipers;
> > +	int kohms;
> > +};
> > +
> > +enum mcp41010_type {
> > +	MCP41010 = 0,
> > +	MCP41050,
> > +	MCP41100,
> > +	MCP42010,
> > +	MCP42050,
> > +	MCP42100,
> > +};
> > +
> > +static const struct mcp41010_cfg mcp41010_cfg[] = {
> > +	[MCP41010] = { .wipers = 1, .kohms =  10, },
> > +	[MCP41050] = { .wipers = 1, .kohms =  50, },
> > +	[MCP41100] = { .wipers = 1, .kohms = 100, },
> > +	[MCP42010] = { .wipers = 2, .kohms =  10, },
> > +	[MCP42050] = { .wipers = 2, .kohms =  50, },
> > +	[MCP42100] = { .wipers = 2, .kohms = 100, },
> > +};
> > +
> > +struct mcp41010_data {
> > +	struct spi_device *spi;
> > +	const struct mcp41010_cfg *cfg;
> > +	struct mutex lock; /* Protect write sequences */
> > +	unsigned int value[MCP41010_MAX_WIPERS]; /* Cache wiper values */
> > +	u8 buf[2] ____cacheline_aligned;
> > +};
> > +
> > +#define MCP41010_CHANNEL(ch) {					\
> > +	.type = IIO_RESISTANCE,					\
> > +	.indexed = 1,						\
> > +	.output = 1,						\
> > +	.channel = (ch),					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +}
> > +
> > +static const struct iio_chan_spec mcp41010_channels[] = {
> > +	MCP41010_CHANNEL(0),
> > +	MCP41010_CHANNEL(1),
> > +};
> > +
> > +static int mcp41010_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct mcp41010_data *data = iio_priv(indio_dev);
> > +	int channel = chan->channel;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		*val = data->value[channel];
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 1000 * data->cfg->kohms;
> > +		*val2 = MCP41010_WIPER_MAX;
> > +		return IIO_VAL_FRACTIONAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int mcp41010_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	int err;
> > +	struct mcp41010_data *data = iio_priv(indio_dev);
> > +	int channel = chan->channel;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val > MCP41010_WIPER_MAX || val < 0)
> > +			return -EINVAL;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> > +	data->buf[0] |= MCP41010_WRITE;  
> 
> Will this not clobber the other channel for mcp42xxx chips???
> 
> > +	data->buf[1] = val & 0xff;
> > +
> > +	err = spi_write(data->spi, data->buf, 2);
> > +	if (!err)
> > +		data->value[channel] = val;
> > +
> > +	mutex_unlock(&data->lock);
> > +
> > +	return err;
> > +}
> > +
> > +static const struct iio_info mcp41010_info = {
> > +	.read_raw = mcp41010_read_raw,
> > +	.write_raw = mcp41010_write_raw,
> > +};
> > +
> > +static int mcp41010_probe(struct spi_device *spi)
> > +{
> > +	int err;
> > +	struct device *dev = &spi->dev;
> > +	unsigned long devid = spi_get_device_id(spi)->driver_data;
> > +	struct mcp41010_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	spi_set_drvdata(spi, indio_dev);
> > +	data->spi = spi;
> > +	data->cfg = &mcp41010_cfg[devid];  
> 
> I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
> max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
> well, but that's not a valid reason for not doing it here AFAICT...
If you want to use the data elements from the devicetree bindings you'll need
to do that.  However, there is an odd path that actually means it will fall back
to getting the right thing as you have it here.
spi_get_device_id calls spi_match_id which compares the sdev->modalias
with the id table names.  modalias has been carefully constructed
to be the text after the comma only and as such this works as is.

Perhaps it's neater though to just use the devicetree access functions.
 
> 
> Cheers,
> Peter
> 
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	indio_dev->dev.parent = dev;
> > +	indio_dev->info = &mcp41010_info;
> > +	indio_dev->channels = mcp41010_channels;
> > +	indio_dev->num_channels = data->cfg->wipers;
> > +	indio_dev->name = spi_get_device_id(spi)->name;
> > +
> > +	err = devm_iio_device_register(dev, indio_dev);
> > +	if (err) {
> > +		dev_info(&spi->dev, "Unable to register %s\n", indio_dev->name);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mcp41010_dt_ids[] = {
> > +	{ .compatible = "microchip,mcp41010",
> > +		.data = &mcp41010_cfg[MCP41010] },
> > +	{ .compatible = "microchip,mcp41050",
> > +		.data = &mcp41010_cfg[MCP41050] },
> > +	{ .compatible = "microchip,mcp41100",
> > +		.data = &mcp41010_cfg[MCP41100] },
> > +	{ .compatible = "microchip,mcp42010",
> > +		.data = &mcp41010_cfg[MCP42010] },
> > +	{ .compatible = "microchip,mcp42050",
> > +		.data = &mcp41010_cfg[MCP42050] },
> > +	{ .compatible = "microchip,mcp42100",
> > +		.data = &mcp41010_cfg[MCP42100] },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mcp41010_dt_ids);
> > +#endif /* CONFIG_OF */
Drop these CONFIG_OF build protections.  It is possible
to instantiate a device via it's devicetree bindings form
ACPI, without having CONFIG_OF true.  These sort
of protections prevent that approach.

> > +
> > +static const struct spi_device_id mcp41010_id[] = {
> > +	{ "mcp41010", MCP41010 },
> > +	{ "mcp41050", MCP41050 },
> > +	{ "mcp41100", MCP41100 },
> > +	{ "mcp42010", MCP42010 },
> > +	{ "mcp42050", MCP42050 },
> > +	{ "mcp42100", MCP42100 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(spi, mcp41010_id);
> > +
> > +static struct spi_driver mcp41010_driver = {
> > +	.driver = {
> > +		.name	= "mcp41010",
> > +		.of_match_table = of_match_ptr(mcp41010_dt_ids),

Don't use that of_match_ptr wrapper as that also breaks the ACPI
probing method.

It's a rather obscure thing that only came to my attention fairly
recently, so always a bit of a surprise. Not worth going through
and removing protections from existing drivers, but let's not 
break any more!

> > +	},
> > +	.probe		= mcp41010_probe,
> > +	.id_table	= mcp41010_id,
> > +};
> > +
> > +module_spi_driver(mcp41010_driver);
> > +
> > +MODULE_AUTHOR("Chris Coffey <cmc@babblebit.net>");
> > +MODULE_DESCRIPTION("MCP41010 digital potentiometer");
> > +MODULE_LICENSE("GPL v2");
> >   
> 

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

On 2018-11-11 15:55, Jonathan Cameron wrote:
> On Tue, 6 Nov 2018 16:37:11 +0000
> Peter Rosin <peda@axentia.se> wrote:
> 
>> Hi!
>>
>> Some comments inline...
> A few additions from me.

*snip*

>>> +	data = iio_priv(indio_dev);
>>> +	spi_set_drvdata(spi, indio_dev);
>>> +	data->spi = spi;
>>> +	data->cfg = &mcp41010_cfg[devid];  
>>
>> I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
>> max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
>> well, but that's not a valid reason for not doing it here AFAICT...
> If you want to use the data elements from the devicetree bindings you'll need
> to do that.  However, there is an odd path that actually means it will fall back
> to getting the right thing as you have it here.
> spi_get_device_id calls spi_match_id which compares the sdev->modalias
> with the id table names.  modalias has been carefully constructed
> to be the text after the comma only and as such this works as is.
> 
> Perhaps it's neater though to just use the devicetree access functions.

Isn't that part about not looking at the vendor-part of the DT-compatible slightly
fragile? Or will modalias/chip-number clashes be detected by something?

Cheers,
Peter

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Peter Rosin]

T24gMjAxOC0xMS0xMSAxNTo1NSwgSm9uYXRoYW4gQ2FtZXJvbiB3cm90ZToNCj4gT24gVHVlLCA2
IE5vdiAyMDE4IDE2OjM3OjExICswMDAwDQo+IFBldGVyIFJvc2luIDxwZWRhQGF4ZW50aWEuc2U+
IHdyb3RlOg0KPiANCj4+IEhpIQ0KPj4NCj4+IFNvbWUgY29tbWVudHMgaW5saW5lLi4uDQo+IEEg
ZmV3IGFkZGl0aW9ucyBmcm9tIG1lLg0KDQoqc25pcCoNCg0KPj4+ICsJZGF0YSA9IGlpb19wcml2
KGluZGlvX2Rldik7DQo+Pj4gKwlzcGlfc2V0X2RydmRhdGEoc3BpLCBpbmRpb19kZXYpOw0KPj4+
ICsJZGF0YS0+c3BpID0gc3BpOw0KPj4+ICsJZGF0YS0+Y2ZnID0gJm1jcDQxMDEwX2NmZ1tkZXZp
ZF07ICANCj4+DQo+PiBJJ20gbWlzc2luZyBhICJvZl9kZXZpY2VfZ2V0X21hdGNoX2RhdGEoZGV2
KSIgY2FsbCBzb21ld2hlcmUgaW4gaGVyZSwgc2VlIGUuZy4gdGhlDQo+PiBtYXg1NDgxLmMgZmls
ZSBpbiB0aGlzIGRpcmVjdG9yeS4gWWVzLCB0aGF0J3MgbWlzc2luZyBmcm9tIHRoZSBtY3A0MTMx
IGRyaXZlciBhcw0KPj4gd2VsbCwgYnV0IHRoYXQncyBub3QgYSB2YWxpZCByZWFzb24gZm9yIG5v
dCBkb2luZyBpdCBoZXJlIEFGQUlDVC4uLg0KPiBJZiB5b3Ugd2FudCB0byB1c2UgdGhlIGRhdGEg
ZWxlbWVudHMgZnJvbSB0aGUgZGV2aWNldHJlZSBiaW5kaW5ncyB5b3UnbGwgbmVlZA0KPiB0byBk
byB0aGF0LiAgSG93ZXZlciwgdGhlcmUgaXMgYW4gb2RkIHBhdGggdGhhdCBhY3R1YWxseSBtZWFu
cyBpdCB3aWxsIGZhbGwgYmFjaw0KPiB0byBnZXR0aW5nIHRoZSByaWdodCB0aGluZyBhcyB5b3Ug
aGF2ZSBpdCBoZXJlLg0KPiBzcGlfZ2V0X2RldmljZV9pZCBjYWxscyBzcGlfbWF0Y2hfaWQgd2hp
Y2ggY29tcGFyZXMgdGhlIHNkZXYtPm1vZGFsaWFzDQo+IHdpdGggdGhlIGlkIHRhYmxlIG5hbWVz
LiAgbW9kYWxpYXMgaGFzIGJlZW4gY2FyZWZ1bGx5IGNvbnN0cnVjdGVkDQo+IHRvIGJlIHRoZSB0
ZXh0IGFmdGVyIHRoZSBjb21tYSBvbmx5IGFuZCBhcyBzdWNoIHRoaXMgd29ya3MgYXMgaXMuDQo+
IA0KPiBQZXJoYXBzIGl0J3MgbmVhdGVyIHRob3VnaCB0byBqdXN0IHVzZSB0aGUgZGV2aWNldHJl
ZSBhY2Nlc3MgZnVuY3Rpb25zLg0KDQpJc24ndCB0aGF0IHBhcnQgYWJvdXQgbm90IGxvb2tpbmcg
YXQgdGhlIHZlbmRvci1wYXJ0IG9mIHRoZSBEVC1jb21wYXRpYmxlIHNsaWdodGx5DQpmcmFnaWxl
PyBPciB3aWxsIG1vZGFsaWFzL2NoaXAtbnVtYmVyIGNsYXNoZXMgYmUgZGV0ZWN0ZWQgYnkgc29t
ZXRoaW5nPw0KDQpDaGVlcnMsDQpQZXRlcg0K

Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Jonathan Cameron]

On Sun, 11 Nov 2018 16:49:28 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2018-11-11 15:55, Jonathan Cameron wrote:
> > On Tue, 6 Nov 2018 16:37:11 +0000
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> Hi!
> >>
> >> Some comments inline...  
> > A few additions from me.  
> 
> *snip*
> 
> >>> +	data = iio_priv(indio_dev);
> >>> +	spi_set_drvdata(spi, indio_dev);
> >>> +	data->spi = spi;
> >>> +	data->cfg = &mcp41010_cfg[devid];    
> >>
> >> I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
> >> max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
> >> well, but that's not a valid reason for not doing it here AFAICT...  
> > If you want to use the data elements from the devicetree bindings you'll need
> > to do that.  However, there is an odd path that actually means it will fall back
> > to getting the right thing as you have it here.
> > spi_get_device_id calls spi_match_id which compares the sdev->modalias
> > with the id table names.  modalias has been carefully constructed
> > to be the text after the comma only and as such this works as is.
> > 
> > Perhaps it's neater though to just use the devicetree access functions.  
> 
> Isn't that part about not looking at the vendor-part of the DT-compatible slightly
> fragile? Or will modalias/chip-number clashes be detected by something?
Yes :)

Well actually in this case not so bad.

It will use a match on the of_device_id to find which driver to probe if there is
one. The modalias part is only being abused to then figure out which device we
have within the driver.

I definitely prefer the explicit route.  Just wanted to point out it 'worked'
:)

> 
> Cheers,
> Peter