[PATCH v4 0/2] Driver for Microchip digital potentiometers

[PATCH v4 0/2] Driver for Microchip digital potentiometers

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

This is the fourth attempt for a driver for these chips.

Thanks for review comments from Greg Kroah-Hartman, Crt Mori,
Daniel Baluta, Lars-Peter Clauson, Andreas Dannenberg and Peter
Meerwald. I think and hope I got it all sorted.

Changes since v3:
- Use i2c_smbus_read_word_swapped (Peter)
- Use devm_iio_device_register and drop the mcp4531_remove op (Peter)
- Add defines for a few magic numbers (Peter)
- Deduplicate channel params with a macro (Peter)
- Mention the i2c client address options (Peter)
- Whitespace and other trivial nits (Peter)

Changes since v2:
- Change naming from mcp4xxx_dpot to mcp4531 (Daniel)
- Added links to datasheet in commit message and source (Daniel)
- Rename from pot to potentiometer (Daniel, Crt)
- Use IIO_RESISTANCE instead of IIO_STEPS (Crt, Lars-Peter)
- Don't use wildcards in MAINTAINERS and point to the iio list (Crt)
- Avoid races by not caching values (Crt)
- Spell Microchip correctly (Andreas)

Changes since v1:
- Make it an IIO driver instead (Greg)
- Don't convolute the code with big obscure macros (Greg)
- Inline the bits from mcp4xxx_dpot.h that are actually used
  and drop that file (me)
- Better Changelog (Greg)

Cheers,
Peter

Peter Rosin (2):
  iio: resistance: Document that resistance can be output
  iio: mcp4531: Driver for Microchip digital potentiometers

 Documentation/ABI/testing/sysfs-bus-iio |    2 +
 MAINTAINERS                             |    6 +
 drivers/iio/Kconfig                     |    1 +
 drivers/iio/Makefile                    |    1 +
 drivers/iio/potentiometer/Kconfig       |   20 ++++
 drivers/iio/potentiometer/Makefile      |    6 +
 drivers/iio/potentiometer/mcp4531.c     |  196 +++++++++++++++++++++++++++++++
 7 files changed, 232 insertions(+)
 create mode 100644 drivers/iio/potentiometer/Kconfig
 create mode 100644 drivers/iio/potentiometer/Makefile
 create mode 100644 drivers/iio/potentiometer/mcp4531.c

-- 
1.7.10.4

[PATCH v4 1/2] iio: resistance: Document that resistance can be output

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/ABI/testing/sysfs-bus-iio |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 9398484196c4..2eea468f704d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1475,6 +1475,8 @@ Description:
 
 What:		/sys/bus/iio/devices/iio:deviceX/in_resistance_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_resistanceX_raw
+What:		/sys/bus/iio/devices/iio:deviceX/out_resistance_raw
+What:		/sys/bus/iio/devices/iio:deviceX/out_resistanceX_raw
 KernelVersion:	4.3
 Contact:	linux-iio@vger.kernel.org
 Description:
-- 
1.7.10.4

[PATCH v4 2/2] iio: mcp4531: Driver for Microchip digital potentiometers

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Add support for Microchip digital potentiometers and rheostats
	MCP4531, MCP4532, MCP4551, MCP4552
	MCP4631, MCP4632, MCP4651, MCP4652

These are either single (45xx) or dual (46xx) wipers with either
129 (4x3x) or 257 (4x5x) steps, and configured either as
potentiometers (4xx1) or rheostats (4xx2). They all use i2c
client addresses of the 0101xxx form.

Datasheet: http://www.microchip.com/downloads/en/DeviceDoc/22096b.pdf

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 MAINTAINERS                         |    6 ++
 drivers/iio/Kconfig                 |    1 +
 drivers/iio/Makefile                |    1 +
 drivers/iio/potentiometer/Kconfig   |   20 ++++
 drivers/iio/potentiometer/Makefile  |    6 ++
 drivers/iio/potentiometer/mcp4531.c |  196 +++++++++++++++++++++++++++++++++++
 6 files changed, 230 insertions(+)
 create mode 100644 drivers/iio/potentiometer/Kconfig
 create mode 100644 drivers/iio/potentiometer/Makefile
 create mode 100644 drivers/iio/potentiometer/mcp4531.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b60e2b2369d2..27862156c7a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6600,6 +6600,12 @@ W:	http://linuxtv.org
 S:	Maintained
 F:	drivers/media/radio/radio-maxiradio*
 
+MCP4531 MICROCHIP DIGITAL POTENTIOMETER DRIVER
+M:	Peter Rosin <peda@axentia.se>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/potentiometer/mcp4531.c
+
 MEDIA DRIVERS FOR RENESAS - VSP1
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 4011effe4c05..7cc87f322655 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -73,6 +73,7 @@ source "drivers/iio/orientation/Kconfig"
 if IIO_TRIGGER
    source "drivers/iio/trigger/Kconfig"
 endif #IIO_TRIGGER
+source "drivers/iio/potentiometer/Kconfig"
 source "drivers/iio/pressure/Kconfig"
 source "drivers/iio/proximity/Kconfig"
 source "drivers/iio/temperature/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 698afc2d17ce..121c814e366b 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -23,6 +23,7 @@ obj-y += imu/
 obj-y += light/
 obj-y += magnetometer/
 obj-y += orientation/
+obj-y += potentiometer/
 obj-y += pressure/
 obj-y += proximity/
 obj-y += temperature/
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
new file mode 100644
index 000000000000..fd75db73e582
--- /dev/null
+++ b/drivers/iio/potentiometer/Kconfig
@@ -0,0 +1,20 @@
+#
+# Potentiometer drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Digital potentiometers"
+
+config MCP4531
+	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
+	depends on I2C
+	help
+	  Say yes here to build support for the Microchip
+	  MCP4531, MCP4532, MCP4551, MCP4552,
+	  MCP4631, MCP4632, MCP4651, MCP4652
+	  digital potentiomenter chips.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mcp4531.
+
+endmenu
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
new file mode 100644
index 000000000000..8afe49227012
--- /dev/null
+++ b/drivers/iio/potentiometer/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O potentiometer drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_MCP4531) += mcp4531.o
diff --git a/drivers/iio/potentiometer/mcp4531.c b/drivers/iio/potentiometer/mcp4531.c
new file mode 100644
index 000000000000..04faf56dabe0
--- /dev/null
+++ b/drivers/iio/potentiometer/mcp4531.c
@@ -0,0 +1,196 @@
+/*
+ * Industrial I/O driver for Microchip digital potentiometers
+ * Copyright (c) 2015  Axentia Technologies AB
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * Datasheet: http://www.microchip.com/downloads/en/DeviceDoc/22096b.pdf
+ *
+ * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
+ * mcp4531	1	129		5, 10, 50, 100          010111x
+ * mcp4532	1	129		5, 10, 50, 100          01011xx
+ * mcp4551	1	257		5, 10, 50, 100          010111x
+ * mcp4552	1	257		5, 10, 50, 100          01011xx
+ * mcp4631	2	129		5, 10, 50, 100          0101xxx
+ * mcp4632	2	129		5, 10, 50, 100          01011xx
+ * mcp4651	2	257		5, 10, 50, 100          0101xxx
+ * mcp4652	2	257		5, 10, 50, 100          01011xx
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+
+#include <linux/iio/iio.h>
+
+#define MCP4531_CONF(wipers, max_pos, uid) \
+	((((wipers) & 0xf) << 10) | \
+	 (((max_pos) & 0xf) << 6) | \
+	 ((uid) & 0x3f))
+
+#define MCP4531_UID(conf)     ((conf) & 0x3f)
+#define MCP4531_MAX_POS(conf) (((conf) >> 6) & 0xf)
+#define MCP4531_WIPERS(conf)  (((conf) >> 10) & 0xf)
+
+#define MCP4531_WRITE (0 << 2)
+#define MCP4531_INCR  (1 << 2)
+#define MCP4531_DECR  (2 << 2)
+#define MCP4531_READ  (3 << 2)
+
+#define MCP4531_WIPER_SHIFT (4)
+
+enum mcp4531_devid {
+	MCP4531_ID = MCP4531_CONF(1, 7, 0),
+	MCP4532_ID = MCP4531_CONF(1, 7, 1),
+	MCP4551_ID = MCP4531_CONF(1, 8, 2),
+	MCP4552_ID = MCP4531_CONF(1, 8, 3),
+	MCP4631_ID = MCP4531_CONF(2, 7, 4),
+	MCP4632_ID = MCP4531_CONF(2, 7, 5),
+	MCP4651_ID = MCP4531_CONF(2, 8, 6),
+	MCP4652_ID = MCP4531_CONF(2, 8, 7),
+};
+
+struct mcp4531_data {
+	struct i2c_client *client;
+	unsigned int max_pos;
+	unsigned long devid;
+};
+
+#define MCP4531_CHANNEL(ch) {				\
+	.type = IIO_RESISTANCE,				\
+	.indexed = 1,					\
+	.output = 1,					\
+	.channel = (ch),				\
+	.address = (ch) << MCP4531_WIPER_SHIFT,		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+}
+
+static const struct iio_chan_spec mcp4531_channels[] = {
+	MCP4531_CHANNEL(0),
+	MCP4531_CHANNEL(1),
+};
+
+static int mcp4531_get_value(struct iio_dev *indio_dev,
+			     int address, int *val)
+{
+	struct mcp4531_data *data = iio_priv(indio_dev);
+	s32 ret;
+
+	ret = i2c_smbus_read_word_swapped(data->client,
+					  MCP4531_READ | address);
+	if (ret < 0)
+		return ret;
+	*val = ret;
+	return 0;
+}
+
+static int mcp4531_set_value(struct iio_dev *indio_dev,
+			     int address, int val)
+{
+	struct mcp4531_data *data = iio_priv(indio_dev);
+
+	if (val >= data->max_pos || val < 0)
+		return -EINVAL;
+
+	return i2c_smbus_write_byte_data(data->client,
+					 MCP4531_WRITE | address | (val >> 8),
+					 val & 0xff);
+}
+
+static int mcp4531_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	int err;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		err = mcp4531_get_value(indio_dev, chan->address, val);
+		if (err < 0)
+			return err;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int mcp4531_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return mcp4531_set_value(indio_dev, chan->address, val);
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info mcp4531_info = {
+	.read_raw = mcp4531_read_raw,
+	.write_raw = mcp4531_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp4531_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	unsigned long devid = id->driver_data;
+	struct mcp4531_data *data;
+	struct iio_dev *indio_dev;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_err(dev, "SMBUS Word Data not supported\n");
+		return -EIO;
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->devid = devid;
+
+	data->max_pos = (1 << MCP4531_MAX_POS(devid)) + 1;
+
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &mcp4531_info;
+	indio_dev->channels = mcp4531_channels;
+	indio_dev->num_channels = MCP4531_WIPERS(devid);
+	indio_dev->name = client->name;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id mcp4531_id[] = {
+	{ "mcp4531", MCP4531_ID },
+	{ "mcp4532", MCP4532_ID },
+	{ "mcp4551", MCP4551_ID },
+	{ "mcp4552", MCP4552_ID },
+	{ "mcp4631", MCP4631_ID },
+	{ "mcp4632", MCP4632_ID },
+	{ "mcp4651", MCP4651_ID },
+	{ "mcp4652", MCP4652_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, mcp4531_id);
+
+static struct i2c_driver mcp4531_driver = {
+	.driver = {
+		.name	= "mcp4531",
+	},
+	.probe		= mcp4531_probe,
+	.id_table	= mcp4531_id,
+};
+
+module_i2c_driver(mcp4531_driver);
+
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_DESCRIPTION("MCP4531 digital potentiometer");
+MODULE_LICENSE("GPL");
-- 
1.7.10.4

Re: [PATCH v4 2/2] iio: mcp4531: Driver for Microchip digital potentiometers

[Jonathan Cameron]

On 22 September 2015 16:36:49 BST, Peter Rosin <peda@lysator.liu.se> wrote:
>From: Peter Rosin <peda@axentia.se>
>
>Add support for Microchip digital potentiometers and rheostats
>	MCP4531, MCP4532, MCP4551, MCP4552
>	MCP4631, MCP4632, MCP4651, MCP4652
>
>These are either single (45xx) or dual (46xx) wipers with either
>129 (4x3x) or 257 (4x5x) steps, and configured either as
>potentiometers (4xx1) or rheostats (4xx2). They all use i2c
>client addresses of the 0101xxx form.
>
>Datasheet: http://www.microchip.com/downloads/en/DeviceDoc/22096b.pdf
>
>Signed-off-by: Peter Rosin <peda@axentia.se>

Mostly fine though I think you may have gone a bit too much for conciseness over
 clarity.  Also the scale appears known so should be exposed to userspace.
>---
> MAINTAINERS                         |    6 ++
> drivers/iio/Kconfig                 |    1 +
> drivers/iio/Makefile                |    1 +
> drivers/iio/potentiometer/Kconfig   |   20 ++++
> drivers/iio/potentiometer/Makefile  |    6 ++
>drivers/iio/potentiometer/mcp4531.c |  196
>+++++++++++++++++++++++++++++++++++
> 6 files changed, 230 insertions(+)
> create mode 100644 drivers/iio/potentiometer/Kconfig
> create mode 100644 drivers/iio/potentiometer/Makefile
> create mode 100644 drivers/iio/potentiometer/mcp4531.c
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index b60e2b2369d2..27862156c7a7 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -6600,6 +6600,12 @@ W:	http://linuxtv.org
> S:	Maintained
> F:	drivers/media/radio/radio-maxiradio*
> 
>+MCP4531 MICROCHIP DIGITAL POTENTIOMETER DRIVER
>+M:	Peter Rosin <peda@axentia.se>
>+L:	linux-iio@vger.kernel.org
>+S:	Maintained
>+F:	drivers/iio/potentiometer/mcp4531.c
>+
> MEDIA DRIVERS FOR RENESAS - VSP1
> M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> L:	linux-media@vger.kernel.org
>diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>index 4011effe4c05..7cc87f322655 100644
>--- a/drivers/iio/Kconfig
>+++ b/drivers/iio/Kconfig
>@@ -73,6 +73,7 @@ source "drivers/iio/orientation/Kconfig"
> if IIO_TRIGGER
>    source "drivers/iio/trigger/Kconfig"
> endif #IIO_TRIGGER
>+source "drivers/iio/potentiometer/Kconfig"
> source "drivers/iio/pressure/Kconfig"
> source "drivers/iio/proximity/Kconfig"
> source "drivers/iio/temperature/Kconfig"
>diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>index 698afc2d17ce..121c814e366b 100644
>--- a/drivers/iio/Makefile
>+++ b/drivers/iio/Makefile
>@@ -23,6 +23,7 @@ obj-y += imu/
> obj-y += light/
> obj-y += magnetometer/
> obj-y += orientation/
>+obj-y += potentiometer/
> obj-y += pressure/
> obj-y += proximity/
> obj-y += temperature/
>diff --git a/drivers/iio/potentiometer/Kconfig
>b/drivers/iio/potentiometer/Kconfig
>new file mode 100644
>index 000000000000..fd75db73e582
>--- /dev/null
>+++ b/drivers/iio/potentiometer/Kconfig
>@@ -0,0 +1,20 @@
>+#
>+# Potentiometer drivers
>+#
>+# When adding new entries keep the list in alphabetical order
>+
>+menu "Digital potentiometers"
>+
>+config MCP4531
>+	tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
>+	depends on I2C
>+	help
>+	  Say yes here to build support for the Microchip
>+	  MCP4531, MCP4532, MCP4551, MCP4552,
>+	  MCP4631, MCP4632, MCP4651, MCP4652
>+	  digital potentiomenter chips.
>+
>+	  To compile this driver as a module, choose M here: the
>+	  module will be called mcp4531.
>+
>+endmenu
>diff --git a/drivers/iio/potentiometer/Makefile
>b/drivers/iio/potentiometer/Makefile
>new file mode 100644
>index 000000000000..8afe49227012
>--- /dev/null
>+++ b/drivers/iio/potentiometer/Makefile
>@@ -0,0 +1,6 @@
>+#
>+# Makefile for industrial I/O potentiometer drivers
>+#
>+
>+# When adding new entries keep the list in alphabetical order
>+obj-$(CONFIG_MCP4531) += mcp4531.o
>diff --git a/drivers/iio/potentiometer/mcp4531.c
>b/drivers/iio/potentiometer/mcp4531.c
>new file mode 100644
>index 000000000000..04faf56dabe0
>--- /dev/null
>+++ b/drivers/iio/potentiometer/mcp4531.c
>@@ -0,0 +1,196 @@
>+/*
>+ * Industrial I/O driver for Microchip digital potentiometers
>+ * Copyright (c) 2015  Axentia Technologies AB
>+ * Author: Peter Rosin <peda@axentia.se>
>+ *
>+ * Datasheet:
>http://www.microchip.com/downloads/en/DeviceDoc/22096b.pdf
>+ *
>+ * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
>+ * mcp4531	1	129		5, 10, 50, 100          010111x
>+ * mcp4532	1	129		5, 10, 50, 100          01011xx
>+ * mcp4551	1	257		5, 10, 50, 100          010111x
>+ * mcp4552	1	257		5, 10, 50, 100          01011xx
>+ * mcp4631	2	129		5, 10, 50, 100          0101xxx
>+ * mcp4632	2	129		5, 10, 50, 100          01011xx
>+ * mcp4651	2	257		5, 10, 50, 100          0101xxx
>+ * mcp4652	2	257		5, 10, 50, 100          01011xx
>+ *
>+ * This program is free software; you can redistribute it and/or
>modify it
>+ * under the terms of the GNU General Public License version 2 as
>published by
>+ * the Free Software Foundation.
>+ */
>+
>+#include <linux/module.h>
>+#include <linux/i2c.h>
>+#include <linux/err.h>
>+
>+#include <linux/iio/iio.h>
>+
>+#define MCP4531_CONF(wipers, max_pos, uid) \
>+	((((wipers) & 0xf) << 10) | \
>+	 (((max_pos) & 0xf) << 6) | \
>+	 ((uid) & 0x3f))

This strike me as harder to read than it need be. I think I would be clearer done 
By setting dev_id to an index into an array to static const structs with the various 
parts explicitly brought out as fields.
Preferably assigned with c99 syntax for really easy reading :)

>+
>+#define MCP4531_UID(conf)     ((conf) & 0x3f)
>+#define MCP4531_MAX_POS(conf) (((conf) >> 6) & 0xf)
>+#define MCP4531_WIPERS(conf)  (((conf) >> 10) & 0xf)
>+
>+#define MCP4531_WRITE (0 << 2)
>+#define MCP4531_INCR  (1 << 2)
>+#define MCP4531_DECR  (2 << 2)
>+#define MCP4531_READ  (3 << 2)
>+
>+#define MCP4531_WIPER_SHIFT (4)
>+
>+enum mcp4531_devid {
>+	MCP4531_ID = MCP4531_CONF(1, 7, 0),
>+	MCP4532_ID = MCP4531_CONF(1, 7, 1),
>+	MCP4551_ID = MCP4531_CONF(1, 8, 2),
>+	MCP4552_ID = MCP4531_CONF(1, 8, 3),
>+	MCP4631_ID = MCP4531_CONF(2, 7, 4),
>+	MCP4632_ID = MCP4531_CONF(2, 7, 5),
>+	MCP4651_ID = MCP4531_CONF(2, 8, 6),
>+	MCP4652_ID = MCP4531_CONF(2, 8, 7),
>+};
>+
>+struct mcp4531_data {
>+	struct i2c_client *client;
>+	unsigned int max_pos;
>+	unsigned long devid;
>+};
>+
>+#define MCP4531_CHANNEL(ch) {				\
>+	.type = IIO_RESISTANCE,				\
>+	.indexed = 1,					\
>+	.output = 1,					\
>+	.channel = (ch),				\
>+	.address = (ch) << MCP4531_WIPER_SHIFT,		\
Why not drop specifying address and use channel with the shift applied at call site?

>+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>+}
>+
>+static const struct iio_chan_spec mcp4531_channels[] = {
>+	MCP4531_CHANNEL(0),
>+	MCP4531_CHANNEL(1),
>+};
>+
>+static int mcp4531_get_value(struct iio_dev *indio_dev,
>+			     int address, int *val)
>+{
>+	struct mcp4531_data *data = iio_priv(indio_dev);
>+	s32 ret;
>+
>+	ret = i2c_smbus_read_word_swapped(data->client,
>+					  MCP4531_READ | address);
>+	if (ret < 0)
>+		return ret;
>+	*val = ret;
>+	return 0;
>+}
>+
>+static int mcp4531_set_value(struct iio_dev *indio_dev,
>+			     int address, int val)
>+{
>+	struct mcp4531_data *data = iio_priv(indio_dev);
>+
>+	if (val >= data->max_pos || val < 0)
>+		return -EINVAL;
>+
>+	return i2c_smbus_write_byte_data(data->client,
>+					 MCP4531_WRITE | address | (val >> 8),
>+					 val & 0xff);
>+}
As each of these wrappers is so brief and only used once I would favour just merging them in where called.

>+
>+static int mcp4531_read_raw(struct iio_dev *indio_dev,
>+			    struct iio_chan_spec const *chan,
>+			    int *val, int *val2, long mask)
>+{
>+	int err;
>+
>+	switch (mask) {
>+	case IIO_CHAN_INFO_RAW:
>+		err = mcp4531_get_value(indio_dev, chan->address, val);
>+		if (err < 0)
>+			return err;
>+		return IIO_VAL_INT;
>+	}
No scale known? Be nice to relate to real world values.  I gather this may need
 platform data of some type (can't open data sheet on phone for some reason)

>+
>+	return -EINVAL;
>+}
>+
>+static int mcp4531_write_raw(struct iio_dev *indio_dev,
>+			     struct iio_chan_spec const *chan,
>+			     int val, int val2, long mask)
>+{
>+	switch (mask) {
>+	case IIO_CHAN_INFO_RAW:
>+		return mcp4531_set_value(indio_dev, chan->address, val);
>+	}
>+
>+	return -EINVAL;
>+}
>+
>+static const struct iio_info mcp4531_info = {
>+	.read_raw = mcp4531_read_raw,
>+	.write_raw = mcp4531_write_raw,
>+	.driver_module = THIS_MODULE,
>+};
>+
>+static int mcp4531_probe(struct i2c_client *client,
>+			 const struct i2c_device_id *id)
>+{
>+	struct device *dev = &client->dev;
>+	unsigned long devid = id->driver_data;
>+	struct mcp4531_data *data;
>+	struct iio_dev *indio_dev;
>+
>+	if (!i2c_check_functionality(client->adapter,
>+				     I2C_FUNC_SMBUS_WORD_DATA)) {
>+		dev_err(dev, "SMBUS Word Data not supported\n");
>+		return -EIO;
>+	}
>+
>+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>+	if (!indio_dev)
>+		return -ENOMEM;
>+	data = iio_priv(indio_dev);
>+	i2c_set_clientdata(client, indio_dev);
>+	data->client = client;
>+	data->devid = devid;
>+
>+	data->max_pos = (1 << MCP4531_MAX_POS(devid)) + 1;
Derive at use site rather than here.

>+
>+	indio_dev->dev.parent = dev;
>+	indio_dev->info = &mcp4531_info;
>+	indio_dev->channels = mcp4531_channels;
>+	indio_dev->num_channels = MCP4531_WIPERS(devid);
>+	indio_dev->name = client->name;
>+
>+	return devm_iio_device_register(dev, indio_dev);
>+}
>+
>+static const struct i2c_device_id mcp4531_id[] = {
>+	{ "mcp4531", MCP4531_ID },
>+	{ "mcp4532", MCP4532_ID },
>+	{ "mcp4551", MCP4551_ID },
>+	{ "mcp4552", MCP4552_ID },
>+	{ "mcp4631", MCP4631_ID },
>+	{ "mcp4632", MCP4632_ID },
>+	{ "mcp4651", MCP4651_ID },
>+	{ "mcp4652", MCP4652_ID },
>+	{}
>+};
>+MODULE_DEVICE_TABLE(i2c, mcp4531_id);
>+
>+static struct i2c_driver mcp4531_driver = {
>+	.driver = {
>+		.name	= "mcp4531",
>+	},
>+	.probe		= mcp4531_probe,
>+	.id_table	= mcp4531_id,
>+};
>+
>+module_i2c_driver(mcp4531_driver);
>+
>+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
>+MODULE_DESCRIPTION("MCP4531 digital potentiometer");
>+MODULE_LICENSE("GPL");

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4 2/2] iio: mcp4531: Driver for Microchip digital potentiometers

[Peter Rosin]

On 2015-09-22 20:22, Jonathan Cameron wrote:
> On 22 September 2015 16:36:49 BST, Peter Rosin <peda@lysator.liu.se> wrote:

*snip* lots of cosmetics that I'll fix in the next spin...

>> +
>> +static int mcp4531_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	int err;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		err = mcp4531_get_value(indio_dev, chan->address, val);
>> +		if (err < 0)
>> +			return err;
>> +		return IIO_VAL_INT;
>> +	}
> No scale known? Be nice to relate to real world values.  I gather this may need
>  platform data of some type (can't open data sheet on phone for some reason)

...but I need more input on this.

Yes, it would be nice and yes it requires platform data, as there is
no way to retrieve if the driver is dealing with a 5, 10, 50 or 100 kOhms
pot/rheostat. You would also have to expose both the resistance between
A to W and between W to B, as you have no way to know how it's been
connected (or you'd need more platform data). Ok, for rheostats you
know how it's connected, since A isn't available, but for pots there
are two resistance values controlled with a single value X i.e.
X*scale/max and (max-X)*scale/max (ignoring the 75 Ohm wiper resistance).

But, I'd rather not go there, as I don't need any of it. Is it Ok to not
provide scaling?

Cheers,
Peter

Re: [PATCH v4 2/2] iio: mcp4531: Driver for Microchip digital potentiometers

[Lars-Peter Clausen]

On 09/23/2015 09:39 AM, Peter Rosin wrote:
> On 2015-09-22 20:22, Jonathan Cameron wrote:
>> On 22 September 2015 16:36:49 BST, Peter Rosin <peda@lysator.liu.se> wrote:
> 
> *snip* lots of cosmetics that I'll fix in the next spin...
> 
>>> +
>>> +static int mcp4531_read_raw(struct iio_dev *indio_dev,
>>> +			    struct iio_chan_spec const *chan,
>>> +			    int *val, int *val2, long mask)
>>> +{
>>> +	int err;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		err = mcp4531_get_value(indio_dev, chan->address, val);
>>> +		if (err < 0)
>>> +			return err;
>>> +		return IIO_VAL_INT;
>>> +	}
>> No scale known? Be nice to relate to real world values.  I gather this may need
>>  platform data of some type (can't open data sheet on phone for some reason)
> 
> ...but I need more input on this.
> 
> Yes, it would be nice and yes it requires platform data, as there is
> no way to retrieve if the driver is dealing with a 5, 10, 50 or 100 kOhms
> pot/rheostat.

You should use the full product name in the ID table for device, it includes
what the resistance is.


> You would also have to expose both the resistance between
> A to W and between W to B, as you have no way to know how it's been
> connected (or you'd need more platform data). Ok, for rheostats you
> know how it's connected, since A isn't available, but for pots there
> are two resistance values controlled with a single value X i.e.
> X*scale/max and (max-X)*scale/max (ignoring the 75 Ohm wiper resistance).

I don't think that is a problem. The scale is still the same for both of
them. Even if you don't provide the scale the user still has to know which
of the terminals uses the inversed and which one uses the non-inversed raw
value for their resistance. So nothing really changes in that regard by
providing the scale or not.

> But, I'd rather not go there, as I don't need any of it. Is it Ok to not
> provide scaling?

Having scaling is not required, but strongly encouraged. It allows creating
generic userspace applications that do not require specific knowledge about
the part they are talking to. Which is one of the main points for having a
framework. Otherwise you could just go with a driver with a custom ABI, or
even implement all the interface logic in userspace using /dev/i2c.