[PATCH 0/2] iio: potentiometer: Add driver support for AD5110

[PATCH 0/2] iio: potentiometer: Add driver support for AD5110

[Mugilraj Dhavachelvan]

Add dt-bindings and driver support for AD5110, a Nonvolatile 
Digital Potentiometer.

Mugilraj Dhavachelvan (2):
  dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices
  iio: potentiometer: Add driver support for AD5110

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/potentiometer/Kconfig             |  10 +
 drivers/iio/potentiometer/Makefile            |   1 +
 drivers/iio/potentiometer/ad5110.c            | 333 ++++++++++++++++++
 5 files changed, 352 insertions(+)
 create mode 100644 drivers/iio/potentiometer/ad5110.c

-- 
2.25.1

[PATCH 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices

[Mugilraj Dhavachelvan]

Add AD5110, a Nonvolatile Digital Potentiometer into
trivial-devices.yaml.

Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 8341e9d23c1e..55c8a6ce5057 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -33,6 +33,8 @@ properties:
           - ad,ad7414
             # ADM9240:  Complete System Hardware Monitor for uProcessor-Based Systems
           - ad,adm9240
+            # AD5110 - Nonvolatile Digital Potentiometer
+          - adi,ad5110
             # Analog Devices ADP5585 Keypad Decoder and I/O Expansion
           - adi,adp5585
             # Analog Devices ADP5585 Keypad Decoder and I/O Expansion with support for Row5
-- 
2.25.1

[PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Mugilraj Dhavachelvan]

The AD5110/AD5112/AD5114 provide a nonvolatile solution
for 128-/64-/32-position adjustment applications, offering
guaranteed low resistor tolerance errors of ±8% and up to
±6 mA current density.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>
---
 MAINTAINERS                        |   6 +
 drivers/iio/potentiometer/Kconfig  |  10 +
 drivers/iio/potentiometer/Makefile |   1 +
 drivers/iio/potentiometer/ad5110.c | 333 +++++++++++++++++++++++++++++
 4 files changed, 350 insertions(+)
 create mode 100644 drivers/iio/potentiometer/ad5110.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bd7aff0c120f..cf6e73b87b6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -452,6 +452,12 @@ S:	Maintained
 W:	https://parisc.wiki.kernel.org/index.php/AD1889
 F:	sound/pci/ad1889.*
 
+AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
+M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/potentiometer/ad5110.c
+
 AD525X ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 4cac0173db8b..832df8da2bc6 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -6,6 +6,16 @@
 
 menu "Digital potentiometers"
 
+config AD5110
+	tristate "Analog Devices AD5110 and similar Digital Potentiometer driver"
+	depends on I2C
+	help
+	  Say yes here to build support for the Analog Devices AD5110, AD5112 
+	  and AD5114 digital potentiometer chip.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad5110.
+
 config AD5272
 	tristate "Analog Devices AD5272 and similar Digital Potentiometer driver"
 	depends on I2C
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 091adf3cdd0b..5ebb8e3bbd76 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AD5110) += ad5110.o
 obj-$(CONFIG_AD5272) += ad5272.o
 obj-$(CONFIG_DS1803) += ds1803.o
 obj-$(CONFIG_MAX5432) += max5432.o
diff --git a/drivers/iio/potentiometer/ad5110.c b/drivers/iio/potentiometer/ad5110.c
new file mode 100644
index 000000000000..26eb7a7aa15b
--- /dev/null
+++ b/drivers/iio/potentiometer/ad5110.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Analog Devices AD5110 digital potentiometer driver
+ *
+ * Copyright (C) 2021 Mugilraj Dhavachelvan <dmugil2000@gmail.com>
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+/* AD5110 commands */
+#define AD5110_EEPROM_WR	1
+#define AD5110_RDAC_WR	2
+#define AD5110_SHUTDOWN	3
+#define AD5110_RESET	4
+#define AD5110_RDAC_RD	5
+#define AD5110_EEPROM_RD	6
+
+/* AD5110_EEPROM_RD data */
+#define AD5110_WIPER_POS	0
+#define AD5110_RESISTOR_TOL	1
+
+#define WIPER_RESISTANCE	70
+
+struct ad5110_cfg {
+	int max_pos;
+	int kohms;
+	int shift;
+};
+
+enum ad5110_type {
+	AD5110_10,
+	AD5110_80,
+	AD5112_05,
+	AD5112_10,
+	AD5112_80,
+	AD5114_10,
+	AD5114_80,
+};
+
+static const struct ad5110_cfg ad5110_cfg[] = {
+	[AD5110_10] = { .max_pos = 128, .kohms = 10 },
+	[AD5110_80] = { .max_pos = 128, .kohms = 80 },
+	[AD5112_05] = { .max_pos = 64, .kohms = 5, .shift = 1 },
+	[AD5112_10] = { .max_pos = 64, .kohms = 10, .shift = 1 },
+	[AD5112_80] = { .max_pos = 64, .kohms = 80, .shift = 1 },
+	[AD5114_10] = { .max_pos = 32, .kohms = 10, .shift = 2 },
+	[AD5114_80] = { .max_pos = 32, .kohms = 80, .shift = 2 },
+};
+
+struct ad5110_data {
+	struct i2c_client       *client;
+	s16			tol;		/* resistor tolerance */
+	bool			enable;
+	struct mutex            lock;
+	const struct ad5110_cfg	*cfg;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8			buf[2] ____cacheline_aligned;
+};
+
+static const struct iio_chan_spec ad5110_channels[] = {
+	{
+		.type = IIO_RESISTANCE,
+		.output = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
+						BIT(IIO_CHAN_INFO_ENABLE),
+	}
+};
+
+static int ad5110_read(struct ad5110_data *data, u8 cmd, int *val)
+{
+	int ret;
+
+	mutex_lock(&data->lock);
+	data->buf[0] = cmd;
+	data->buf[1] = *val;
+
+	ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
+	if (ret < 0)
+		goto error;
+
+	ret = i2c_master_recv_dmasafe(data->client, data->buf, 1);
+	if (ret < 0)
+		goto error;
+
+	*val = data->buf[0];
+	ret = 0;
+
+error:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val)
+{
+	int ret;
+
+	mutex_lock(&data->lock);
+	data->buf[0] = cmd;
+	data->buf[1] = val;
+
+	ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
+	mutex_unlock(&data->lock);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val)
+{
+	int ret;
+
+	ret = ad5110_read(data, cmd, &val);
+	if (ret)
+		return ret;
+
+	data->tol = FIELD_GET(GENMASK(6, 3), val);
+	data->tol = ((val & GENMASK(2, 0)) * 1000 / 8) + data->tol * 1000;
+	data->tol = data->cfg->kohms * data->tol / 100;
+	if (!(val & BIT(7)))
+		data->tol *= -1;
+
+	return 0;
+}
+
+static ssize_t ad5110_eeprom_read(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad5110_data *data = iio_priv(indio_dev);
+	int val = AD5110_WIPER_POS;
+	int ret;
+
+	ret = ad5110_read(data, AD5110_EEPROM_RD, &val);
+	if (ret)
+		return ret;
+
+	return iio_format_value(buf, IIO_VAL_INT, 1, &val);
+}
+
+static ssize_t ad5110_eeprom_write(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad5110_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad5110_write(data, AD5110_EEPROM_WR, 0);
+	if (ret) {
+		dev_err(&data->client->dev, "RDAC to EEPROM write failed\n");
+		return ret;
+	}
+	msleep(20);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
+		       ad5110_eeprom_read,
+		       ad5110_eeprom_write, 0);
+
+static struct attribute *ad5110_attributes[] = {
+	&iio_dev_attr_wiper_pos_eeprom.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad5110_attribute_group = {
+	.attrs = ad5110_attributes,
+};
+
+static int ad5110_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ad5110_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ad5110_read(data, AD5110_RDAC_RD, val);
+		if (ret)
+			return ret;
+
+		*val = *val >> data->cfg->shift;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = WIPER_RESISTANCE * data->cfg->max_pos;
+		*val2 = 1000 * data->cfg->kohms + data->tol;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1000 * data->cfg->kohms + data->tol;
+		*val2 = data->cfg->max_pos;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_ENABLE:
+		*val = data->enable;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5110_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct ad5110_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val >= data->cfg->max_pos || val < 0)
+			return -EINVAL;
+
+		return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
+	case IIO_CHAN_INFO_ENABLE:
+		if (val < 0 || val > 1)
+			return -EINVAL;
+		if (data->enable == val)
+			return 0;
+		ret = ad5110_write(data, AD5110_SHUTDOWN, val);
+		if (ret)
+			return ret;
+		data->enable = val;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ad5110_info = {
+	.read_raw = ad5110_read_raw,
+	.write_raw = ad5110_write_raw,
+	.attrs = &ad5110_attribute_group,
+};
+
+#define AD5110_COMPATIBLE(of_compatible, cfg) {	\
+			.compatible = of_compatible,	\
+			.data = &ad5110_cfg[cfg],	\
+}
+
+static const struct of_device_id ad5110_of_match[] = {
+	AD5110_COMPATIBLE("adi,ad5110-10", AD5110_10),
+	AD5110_COMPATIBLE("adi,ad5110-80", AD5110_80),
+	AD5110_COMPATIBLE("adi,ad5112-05", AD5112_05),
+	AD5110_COMPATIBLE("adi,ad5112-10", AD5112_10),
+	AD5110_COMPATIBLE("adi,ad5112-80", AD5112_80),
+	AD5110_COMPATIBLE("adi,ad5114-10", AD5114_10),
+	AD5110_COMPATIBLE("adi,ad5114-80", AD5114_80),
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad5110_of_match);
+
+static const struct i2c_device_id ad5110_id[] = {
+	{ "ad5110-10", AD5110_10 },
+	{ "ad5110-80", AD5110_80 },
+	{ "ad5112-05", AD5112_05 },
+	{ "ad5112-10", AD5112_10 },
+	{ "ad5112-80", AD5112_80 },
+	{ "ad5114-10", AD5114_10 },
+	{ "ad5114-80", AD5114_80 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad5110_id);
+
+static int ad5110_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct ad5110_data *data;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+	data->cfg = device_get_match_data(dev);
+	if (!data->cfg)
+		data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];
+
+	/* refresh RDAC register with EEPROM */
+	ret = ad5110_write(data, AD5110_RESET, 0);
+	if (ret) {
+		dev_err(dev, "Refresh RDAC with EEPROM failed\n");
+		return ret;
+	}
+
+	ret = ad5110_resistor_tol(data, AD5110_EEPROM_RD, AD5110_RESISTOR_TOL);
+	if (ret) {
+		dev_err(dev, "Read resistor tolerance failed\n");
+		return ret;
+	}
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad5110_info;
+	indio_dev->channels = ad5110_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ad5110_channels);
+	indio_dev->name = client->name;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static struct i2c_driver ad5110_driver = {
+	.driver = {
+		.name	= "ad5110",
+		.of_match_table = ad5110_of_match,
+	},
+	.probe_new	= ad5110_probe,
+	.id_table	= ad5110_id,
+};
+
+module_i2c_driver(ad5110_driver);
+
+MODULE_AUTHOR("Mugilraj Dhavachelvan <dmugil2000@gmail.com>");
+MODULE_DESCRIPTION("AD5110 digital potentiometer");
+MODULE_LICENSE("GPL v2");
+
-- 
2.25.1

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Lars-Peter Clausen]

On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:
> The AD5110/AD5112/AD5114 provide a nonvolatile solution
> for 128-/64-/32-position adjustment applications, offering
> guaranteed low resistor tolerance errors of ±8% and up to
> ±6 mA current density.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
> Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>

Thanks for the patch. This looks really good!


> [...]
>
> +static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	data->buf[0] = cmd;
> +	data->buf[1] = val;
> +
> +	ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
> +	mutex_unlock(&data->lock);

This returns the number of bytes written, which can be less then the 
number of bytes requested if there was an error. This should check if 
the right amount of bytes was written and return -EIO otherwise. Same 
for the other places that read/write from I2C.

> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val)
> +{
> +	int ret;
> +
> +	ret = ad5110_read(data, cmd, &val);
> +	if (ret)
> +		return ret;
> +
> +	data->tol = FIELD_GET(GENMASK(6, 3), val);
> +	data->tol = ((val & GENMASK(2, 0)) * 1000 / 8) + data->tol * 1000;
> +	data->tol = data->cfg->kohms * data->tol / 100;

This is not wrong, but it can be implemented a bit simpler. The 
tolerance is encoded as a fixed point number, you don't have to treat 
them as two independent fields, but can treat it as one fixed point number.

     data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 1000 / 100 / 8;


> +	if (!(val & BIT(7)))
> +		data->tol *= -1;
> +
> +	return 0;
> +}
> +
> +static ssize_t ad5110_eeprom_read(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad5110_data *data = iio_priv(indio_dev);
> +	int val = AD5110_WIPER_POS;
> +	int ret;
> +
> +	ret = ad5110_read(data, AD5110_EEPROM_RD, &val);
> +	if (ret)
> +		return ret;
Maybe apply shift to get consistent behavior with `raw`.
> +
> +	return iio_format_value(buf, IIO_VAL_INT, 1, &val);
> +}
> +
> +static ssize_t ad5110_eeprom_write(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad5110_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad5110_write(data, AD5110_EEPROM_WR, 0);
> +	if (ret) {
> +		dev_err(&data->client->dev, "RDAC to EEPROM write failed\n");
> +		return ret;
> +	}
> +	msleep(20);
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
> +		       ad5110_eeprom_read,
> +		       ad5110_eeprom_write, 0);
This is new custom ABI and needs to be documented.
> +static int ad5110_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad5110_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val >= data->cfg->max_pos || val < 0)
val == data->cfg->max_pos is a valid setting. Writing max_pos puts it in 
top-scale mode which gives maximum resistance.
> +			return -EINVAL;
> +
> +		return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (val < 0 || val > 1)
> +			return -EINVAL;
> +		if (data->enable == val)
> +			return 0;
> +		ret = ad5110_write(data, AD5110_SHUTDOWN, val);
Doesn't val have to be inverted to get the right behavior?
> +		if (ret)
> +			return ret;
> +		data->enable = val;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
[...]

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Lars-Peter Clausen]

On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:
> +static const struct iio_chan_spec ad5110_channels[] = {
> +	{
> +		.type = IIO_RESISTANCE,
> +		.output = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> +						BIT(IIO_CHAN_INFO_ENABLE),
There is only on channel in this device. Why are some `separate` and 
others `shared_by_type`?

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Mugilraj Dhavachelvan]

On 07/08/21 5:41 pm, Lars-Peter Clausen wrote:
> On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:
>> The AD5110/AD5112/AD5114 provide a nonvolatile solution
>> for 128-/64-/32-position adjustment applications, offering
>> guaranteed low resistor tolerance errors of ±8% and up to
>> ±6 mA current density.
>>
>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
>> Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>
> 
> Thanks for the patch. This looks really good> 
> 
>> [...]
>>
>> +static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&data->lock);
>> +    data->buf[0] = cmd;
>> +    data->buf[1] = val;
>> +
>> +    ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
>> +    mutex_unlock(&data->lock);
> 
> This returns the number of bytes written, which can be less then the number of bytes requested if there was an error. This should check if the right amount of bytes was written and return -EIO otherwise. Same for the other places that read/write from I2C.

Fixed in v2.
> 
>> +
>> +    return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val)
>> +{
>> +    int ret;
>> +
>> +    ret = ad5110_read(data, cmd, &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    data->tol = FIELD_GET(GENMASK(6, 3), val);
>> +    data->tol = ((val & GENMASK(2, 0)) * 1000 / 8) + data->tol * 1000;
>> +    data->tol = data->cfg->kohms * data->tol / 100;
> 
> This is not wrong, but it can be implemented a bit simpler. The tolerance is encoded as a fixed point number, you don't have to treat them as two independent fields, but can treat it as one fixed point number.
> 
>     data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 1000 / 100 / 8;
> 
Great, Fixed in v2.
> 
>> +    if (!(val & BIT(7)))
>> +        data->tol *= -1;
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t ad5110_eeprom_read(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +    struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +    struct ad5110_data *data = iio_priv(indio_dev);
>> +    int val = AD5110_WIPER_POS;
>> +    int ret;
>> +
>> +    ret = ad5110_read(data, AD5110_EEPROM_RD, &val);
>> +    if (ret)
>> +        return ret;
> Maybe apply shift to get consistent behavior with `raw`.
Fixed in v2.
>> +
>> +    return iio_format_value(buf, IIO_VAL_INT, 1, &val);
>> +}
>> +
>> +static ssize_t ad5110_eeprom_write(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       const char *buf, size_t len)
>> +{
>> +    struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +    struct ad5110_data *data = iio_priv(indio_dev);
>> +    int ret;
>> +
>> +    ret = ad5110_write(data, AD5110_EEPROM_WR, 0);
>> +    if (ret) {
>> +        dev_err(&data->client->dev, "RDAC to EEPROM write failed\n");
>> +        return ret;
>> +    }
>> +    msleep(20);
>> +
>> +    return len;
>> +}
>> +
>> +static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
>> +               ad5110_eeprom_read,
>> +               ad5110_eeprom_write, 0);
> This is new custom ABI and needs to be documentedI'm not aware of this, fixed in v2.
>> +static int ad5110_write_raw(struct iio_dev *indio_dev,
>> +                struct iio_chan_spec const *chan,
>> +                int val, int val2, long mask)
>> +{
>> +    struct ad5110_data *data = iio_priv(indio_dev);
>> +    int ret;
>> +
>> +    switch (mask) {
>> +    case IIO_CHAN_INFO_RAW:
>> +        if (val >= data->cfg->max_pos || val < 0)
> val == data->cfg->max_pos is a valid setting. Writing max_pos puts it in top-scale mode which gives maximum resistance.
Fixed in v2.
>> +            return -EINVAL;
>> +
>> +        return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
>> +    case IIO_CHAN_INFO_ENABLE:
>> +        if (val < 0 || val > 1)
>> +            return -EINVAL;
>> +        if (data->enable == val)
>> +            return 0;
>> +        ret = ad5110_write(data, AD5110_SHUTDOWN, val);
> Doesn't val have to be inverted to get the right behaviorI just replicated the datasheet operation. 
You mean,
 1 - shutdown off
 0 - shutdown on
If yes, then the user won't get confused with the datasheet and the behavior of the driver?
Or Is it work like this? But yeah even I like this change it's more convenient.
>> +        if (ret)
>> +            return ret;
>> +        data->enable = val;
>> +        return 0;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +}
> [...]
Thanks for feedback!!

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Mugilraj Dhavachelvan]

On 07/08/21 5:47 pm, Lars-Peter Clausen wrote:
> On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:
>> +static const struct iio_chan_spec ad5110_channels[] = {
>> +    {
>> +        .type = IIO_RESISTANCE,
>> +        .output = 1,
>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET),
>> +        .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
>> +                        BIT(IIO_CHAN_INFO_ENABLE),
> There is only on channel in this device. Why are some `separate` and others `shared_by_type`?
> 
No special reason, I'll put everything into 'separate'. Fixed in v2.
> 

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Mugilraj Dhavachelvan]

Sorry some formatting issues happened in my previous mail.

On 07/08/21 10:56 pm, Mugilraj Dhavachelvan wrote:
> 
> 
> On 07/08/21 5:41 pm, Lars-Peter Clausen wrote:
>> On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:
>>> The AD5110/AD5112/AD5114 provide a nonvolatile solution
>>> for 128-/64-/32-position adjustment applications, offering
>>> guaranteed low resistor tolerance errors of ±8% and up to
>>> ±6 mA current density.
>>>
>>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
>>> Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>
>>
>> Thanks for the patch. This looks really good> 
>>
>>> [...]
>>>
>>> +static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val)
>>> +{
>>> +    int ret;
>>> +
>>> +    mutex_lock(&data->lock);
>>> +    data->buf[0] = cmd;
>>> +    data->buf[1] = val;
>>> +
>>> +    ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
>>> +    mutex_unlock(&data->lock);
>>
>> This returns the number of bytes written, which can be less then the number of bytes requested if there was an error. This should check if the right amount of bytes was written and return -EIO otherwise. Same for the other places that read/write from I2C.
> 
> Fixed in v2.
>>
>>> +
>>> +    return ret < 0 ? ret : 0;
>>> +}
>>> +
>>> +static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = ad5110_read(data, cmd, &val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    data->tol = FIELD_GET(GENMASK(6, 3), val);
>>> +    data->tol = ((val & GENMASK(2, 0)) * 1000 / 8) + data->tol * 1000;
>>> +    data->tol = data->cfg->kohms * data->tol / 100;
>>
>> This is not wrong, but it can be implemented a bit simpler. The tolerance is encoded as a fixed point number, you don't have to treat them as two independent fields, but can treat it as one fixed point number.
>>
>>     data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 1000 / 100 / 8;
>>
> Great, Fixed in v2.
>>
>>> +    if (!(val & BIT(7)))
>>> +        data->tol *= -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static ssize_t ad5110_eeprom_read(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      char *buf)
>>> +{
>>> +    struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +    struct ad5110_data *data = iio_priv(indio_dev);
>>> +    int val = AD5110_WIPER_POS;
>>> +    int ret;
>>> +
>>> +    ret = ad5110_read(data, AD5110_EEPROM_RD, &val);
>>> +    if (ret)
>>> +        return ret;
>> Maybe apply shift to get consistent behavior with `raw`.
> Fixed in v2.
>>> +
>>> +    return iio_format_value(buf, IIO_VAL_INT, 1, &val);
>>> +}
>>> +
>>> +static ssize_t ad5110_eeprom_write(struct device *dev,
>>> +                       struct device_attribute *attr,
>>> +                       const char *buf, size_t len)
>>> +{
>>> +    struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +    struct ad5110_data *data = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    ret = ad5110_write(data, AD5110_EEPROM_WR, 0);
>>> +    if (ret) {
>>> +        dev_err(&data->client->dev, "RDAC to EEPROM write failed\n");
>>> +        return ret;
>>> +    }
>>> +    msleep(20);
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
>>> +               ad5110_eeprom_read,
>>> +               ad5110_eeprom_write, 0);
>> This is new custom ABI and needs to be documented

> I'm not aware of this, fixed in v2.
>>> +static int ad5110_write_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int val, int val2, long mask)
>>> +{
>>> +    struct ad5110_data *data = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        if (val >= data->cfg->max_pos || val < 0)
>> val == data->cfg->max_pos is a valid setting. Writing max_pos puts it in top-scale mode which gives maximum resistance.
> Fixed in v2.
>>> +            return -EINVAL;
>>> +
>>> +        return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
>>> +    case IIO_CHAN_INFO_ENABLE:
>>> +        if (val < 0 || val > 1)
>>> +            return -EINVAL;
>>> +        if (data->enable == val)
>>> +            return 0;
>>> +        ret = ad5110_write(data, AD5110_SHUTDOWN, val);
>> Doesn't val have to be inverted to get the right behavior

> I just replicated the datasheet operation. 
> You mean,
>  1 - shutdown off
>  0 - shutdown on
> If yes, then the user won't get confused with the datasheet and the behavior of the driver?
> Or Is it work like this? But yeah even I like this change it's more convenient.
>>> +        if (ret)
>>> +            return ret;
>>> +        data->enable = val;
>>> +        return 0;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +}
>> [...]
> Thanks for feedback!!
> 

Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Jonathan Cameron]

On Sat, 7 Aug 2021 23:04:18 +0530
Mugilraj Dhavachelvan <dmugil2000@gmail.com> wrote:

> Sorry some formatting issues happened in my previous mail.
> 
> On 07/08/21 10:56 pm, Mugilraj Dhavachelvan wrote:
> > 
> > 
> > On 07/08/21 5:41 pm, Lars-Peter Clausen wrote:  
> >> On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:  
> >>> The AD5110/AD5112/AD5114 provide a nonvolatile solution
> >>> for 128-/64-/32-position adjustment applications, offering
> >>> guaranteed low resistor tolerance errors of ±8% and up to
> >>> ±6 mA current density.
> >>>
> >>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
> >>> Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>  
> >>
> >> Thanks for the patch. This looks really good> 
> >>  
>

...

> >>> +
> >>> +static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
> >>> +               ad5110_eeprom_read,
> >>> +               ad5110_eeprom_write, 0);  
> >> This is new custom ABI and needs to be documented  

We have existing similar ABI in dac/mcp4725 which is simply
called store_eeprom

It's in the main docs
Documentation/ABI/testing/sysfs-bus-iio as storing
device configuration.  I'm guessing this device doesn't have
other configuration so that description will work?


> 
> > I'm not aware of this, fixed in v2.  
> >>> +static int ad5110_write_raw(struct iio_dev *indio_dev,
> >>> +                struct iio_chan_spec const *chan,
> >>> +                int val, int val2, long mask)
> >>> +{
> >>> +    struct ad5110_data *data = iio_priv(indio_dev);
> >>> +    int ret;
> >>> +
> >>> +    switch (mask) {
> >>> +    case IIO_CHAN_INFO_RAW:
> >>> +        if (val >= data->cfg->max_pos || val < 0)  
> >> val == data->cfg->max_pos is a valid setting. Writing max_pos puts it in top-scale mode which gives maximum resistance.  
> > Fixed in v2.  
> >>> +            return -EINVAL;
> >>> +
> >>> +        return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
> >>> +    case IIO_CHAN_INFO_ENABLE:
> >>> +        if (val < 0 || val > 1)
> >>> +            return -EINVAL;
> >>> +        if (data->enable == val)
> >>> +            return 0;
> >>> +        ret = ad5110_write(data, AD5110_SHUTDOWN, val);  
> >> Doesn't val have to be inverted to get the right behavior  
> 
> > I just replicated the datasheet operation. 
> > You mean,
> >  1 - shutdown off
> >  0 - shutdown on
> > If yes, then the user won't get confused with the datasheet and the behavior of the driver?
> > Or Is it work like this? But yeah even I like this change it's more convenient.  

ABI has to be consistent and writing an enable attribute with 1 has to mean enabling it whatever
approach the datasheet takes to describe things.  Most users don't read
datasheets so interface needs to be intuitive.

> >>> +        if (ret)
> >>> +            return ret;
> >>> +        data->enable = val;
> >>> +        return 0;
> >>> +    default:
> >>> +        return -EINVAL;
> >>> +    }
> >>> +}  
> >> [...]  
> > Thanks for feedback!!
> >