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

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

[Mugilraj Dhavachelvan]

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

Changes since v1:
 - Drop 'shared_by_type' since it's a single channel
 - Add error check for i2c read/write data bytes
 - Simplified calculation for tolerance
 - Add shift for eeprom wiper pos read
 - Change new custom ABI to existing ABI
 - Allow top-scale mode by writting max_pos to val

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            | 339 ++++++++++++++++++
 5 files changed, 358 insertions(+)
 create mode 100644 drivers/iio/potentiometer/ad5110.c

-- 
2.25.1

[PATCH v2 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

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

[Rob Herring]

On Mon, 09 Aug 2021 13:27:19 +0530, Mugilraj Dhavachelvan wrote:
> 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(+)
> 

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

[PATCH v2 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 | 339 +++++++++++++++++++++++++++++
 4 files changed, 356 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..4ba76e968edb
--- /dev/null
+++ b/drivers/iio/potentiometer/ad5110.c
@@ -0,0 +1,339 @@
+// 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) |
+					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 != sizeof(data->buf)) {
+		ret = -EIO;
+		goto error;
+	}
+
+	ret = i2c_master_recv_dmasafe(data->client, data->buf, 1);
+	if (ret != 1) {
+		ret = -EIO;
+		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));
+	if (ret != sizeof(data->buf))
+		ret = -EIO;
+
+	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 = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 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;
+
+	val = val >> data->cfg->shift;
+	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(store_eeprom, 0644,
+		       ad5110_eeprom_read,
+		       ad5110_eeprom_write, 0);
+
+static struct attribute *ad5110_attributes[] = {
+	&iio_dev_attr_store_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 ? 0 : 1);
+		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->enable = 1;
+	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 v2 2/2] iio: potentiometer: Add driver support for AD5110

[Jonathan Cameron]

On Mon,  9 Aug 2021 13:27:20 +0530
Mugilraj Dhavachelvan <dmugil2000@gmail.com> 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>

Looks good to me, but I'll leave it on the list for a little
while to give time for others to take another look.

Thanks,

Jonathan
> ---
>  MAINTAINERS                        |   6 +
>  drivers/iio/potentiometer/Kconfig  |  10 +
>  drivers/iio/potentiometer/Makefile |   1 +
>  drivers/iio/potentiometer/ad5110.c | 339 +++++++++++++++++++++++++++++
>  4 files changed, 356 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..4ba76e968edb
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ad5110.c
> @@ -0,0 +1,339 @@
> +// 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) |
> +					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 != sizeof(data->buf)) {
> +		ret = -EIO;
> +		goto error;
> +	}
> +
> +	ret = i2c_master_recv_dmasafe(data->client, data->buf, 1);
> +	if (ret != 1) {
> +		ret = -EIO;
> +		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));
> +	if (ret != sizeof(data->buf))
> +		ret = -EIO;
> +
> +	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 = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 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;
> +
> +	val = val >> data->cfg->shift;
> +	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(store_eeprom, 0644,
> +		       ad5110_eeprom_read,
> +		       ad5110_eeprom_write, 0);
> +
> +static struct attribute *ad5110_attributes[] = {
> +	&iio_dev_attr_store_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 ? 0 : 1);
> +		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->enable = 1;
> +	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");
> +

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

[Andy Shevchenko]

On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
<dmugil2000@gmail.com> 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.

...

> +/*
> + * 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

> + *

Redundant blank line.

> + */

...

> +#define WIPER_RESISTANCE       70

Missed prefix?

...

> +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) |
> +                                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE),
> +       }

+ comma.

> +};

...

> +       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
> +       if (ret != sizeof(data->buf)) {

> +               ret = -EIO;

Shadowed error code when ret < 0.

> +               goto error;
> +       }

...

> +       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
> +       if (ret != sizeof(data->buf))
> +               ret = -EIO;

Ditto.

> +       mutex_unlock(&data->lock);
> +
> +       return ret < 0 ? ret : 0;
> +}

...

> +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> +       if (!(val & BIT(7)))
> +               data->tol *= -1;

Shouldn't you simple use corresponding sign_extend*()?

...

> +       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);

Each long sleeps like this must be explained.

...

> +static IIO_DEVICE_ATTR(store_eeprom, 0644,
> +                      ad5110_eeprom_read,
> +                      ad5110_eeprom_write, 0);

IIO_DEVICE_ATTR_RW() ?

...

> +static struct attribute *ad5110_attributes[] = {
> +       &iio_dev_attr_store_eeprom.dev_attr.attr,
> +       NULL,

No comma for a terminator line.

> +};

...

> +       data->cfg = device_get_match_data(dev);

> +       if (!data->cfg)
> +               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];

Not sure this is not a dead code since you are using ->probe_new().

...

> +static struct i2c_driver ad5110_driver = {
> +       .driver = {
> +               .name   = "ad5110",
> +               .of_match_table = ad5110_of_match,
> +       },
> +       .probe_new      = ad5110_probe,
> +       .id_table       = ad5110_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ad5110_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Mugilraj Dhavachelvan]

On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
> <dmugil2000@gmail.com> 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.
> 
> ...
> 
> > +/*
> > + * 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
> 
> > + *
> 
> Redundant blank line.
> 
fixed in v3
> > + */
> 
> ...
> 
> > +#define WIPER_RESISTANCE       70
> 
> Missed prefix?
> 
fixed in v3
> ...
> 
> > +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) |
> > +                                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE),
> > +       }
> 
> + comma.
> 
fixed in v3
> > +};
> 
> ...
> 
> > +       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
> > +       if (ret != sizeof(data->buf)) {
> 
> > +               ret = -EIO;
> 
> Shadowed error code when ret < 0.
> 
fixed in v3
> > +               goto error;
> > +       }
> 
> ...
> 
> > +       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
> > +       if (ret != sizeof(data->buf))
> > +               ret = -EIO;
> 
> Ditto.
> 
fixed in v3
> > +       mutex_unlock(&data->lock);
> > +
> > +       return ret < 0 ? ret : 0;
> > +}
> 
> ...
> 
> > +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > +       if (!(val & BIT(7)))
> > +               data->tol *= -1;
> 
> Shouldn't you simple use corresponding sign_extend*()?
> 
I'm not able see any sign_extend for 16 bit. Is there any other way?
> ...
> 
> > +       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);
> 
> Each long sleeps like this must be explained.
> 
fixed in v3
> ...
> 
> > +static IIO_DEVICE_ATTR(store_eeprom, 0644,
> > +                      ad5110_eeprom_read,
> > +                      ad5110_eeprom_write, 0);
> 
> IIO_DEVICE_ATTR_RW() ?
> 
fixed in v3
> ...
> 
> > +static struct attribute *ad5110_attributes[] = {
> > +       &iio_dev_attr_store_eeprom.dev_attr.attr,
> > +       NULL,
> 
> No comma for a terminator line.
> 
fixed in v3
> > +};
> 
> ...
> 
> > +       data->cfg = device_get_match_data(dev);
> 
> > +       if (!data->cfg)
> > +               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];
> 
> Not sure this is not a dead code since you are using ->probe_new().
> 
Even I'm suspecting that and also removing id_table. But I'm not sure of
it so just left as it is.
> ...
> 
> > +static struct i2c_driver ad5110_driver = {
> > +       .driver = {
> > +               .name   = "ad5110",
> > +               .of_match_table = ad5110_of_match,
> > +       },
> > +       .probe_new      = ad5110_probe,
> > +       .id_table       = ad5110_id,
> > +};
> 
> > +
> 
> Redundant blank line
fixed in v3.
> 
> > +module_i2c_driver(ad5110_driver);
> 
Thanks for the feedback :)
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan
<dmugil2000@gmail.com> wrote:
> On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
> > <dmugil2000@gmail.com> wrote:

...

> > > +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > > +       if (!(val & BIT(7)))
> > > +               data->tol *= -1;
> >
> > Shouldn't you simple use corresponding sign_extend*()?
> >
> I'm not able see any sign_extend for 16 bit. Is there any other way?

So, then add it in bitops.h the same way it's done for s32 and s64.

...

> > > +       if (!data->cfg)
> > > +               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];
> >
> > Not sure this is not a dead code since you are using ->probe_new().
> >
> Even I'm suspecting that and also removing id_table. But I'm not sure of
> it so just left as it is.

I²C ID table is good to have without direct use, but ->probe_new() is
called if and only if there is a compatible string or ACPI ID match.
In such case data->cfg may be NULL if and only if the corresponding
table missed it, but this will be a bug anyway, so the above code will
rather hide the bug. Hence, please remove it.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 10:03 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan
> <dmugil2000@gmail.com> wrote:
> > On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
> > > <dmugil2000@gmail.com> wrote:
>
> ...
>
> > > > +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > > > +       if (!(val & BIT(7)))
> > > > +               data->tol *= -1;
> > >
> > > Shouldn't you simple use corresponding sign_extend*()?
> > >
> > I'm not able see any sign_extend for 16 bit. Is there any other way?
>
> So, then add it in bitops.h the same way it's done for s32 and s64.

In case you are afraid that this will delay patches and you are in a
hurry, you may define it locally (but in the very same way, so
somebody can make it generic). Nevertheless, I think this one can be
added directly to the generic header, it will definitely have more
than one user.

> ...
>
> > > > +       if (!data->cfg)
> > > > +               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];
> > >
> > > Not sure this is not a dead code since you are using ->probe_new().
> > >
> > Even I'm suspecting that and also removing id_table. But I'm not sure of
> > it so just left as it is.
>
> I²C ID table is good to have without direct use, but ->probe_new() is
> called if and only if there is a compatible string or ACPI ID match.
> In such case data->cfg may be NULL if and only if the corresponding
> table missed it, but this will be a bug anyway, so the above code will
> rather hide the bug. Hence, please remove it.



-- 
With Best Regards,
Andy Shevchenko

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

[Mugilraj Dhavachelvan]

On Wed, Aug 11, 2021 at 10:05:41AM +0300, Andy Shevchenko wrote:
> On Wed, Aug 11, 2021 at 10:03 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan
> > <dmugil2000@gmail.com> wrote:
> > > On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
> > > > <dmugil2000@gmail.com> wrote:
> >
> > ...
> >
> > > > > +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > > > > +       if (!(val & BIT(7)))
> > > > > +               data->tol *= -1;
> > > >
> > > > Shouldn't you simple use corresponding sign_extend*()?
> > > >
> > > I'm not able see any sign_extend for 16 bit. Is there any other way?
> >
> > So, then add it in bitops.h the same way it's done for s32 and s64.
> 
> In case you are afraid that this will delay patches and you are in a
> hurry, you may define it locally (but in the very same way, so
> somebody can make it generic). Nevertheless, I think this one can be
> added directly to the generic header, it will definitely have more
> than one user.
> 
It seems like we can use sign_extend32() for 16 and 8 bit as well.
> > ...
> >
> > > > > +       if (!data->cfg)
> > > > > +               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];
> > > >
> > > > Not sure this is not a dead code since you are using ->probe_new().
> > > >
> > > Even I'm suspecting that and also removing id_table. But I'm not sure of
> > > it so just left as it is.
> >
> > I²C ID table is good to have without direct use, but ->probe_new() is
> > called if and only if there is a compatible string or ACPI ID match.
> > In such case data->cfg may be NULL if and only if the corresponding
> > table missed it, but this will be a bug anyway, so the above code will
> > rather hide the bug. Hence, please remove it.
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Lars-Peter Clausen]

On 8/10/21 2:49 PM, Andy Shevchenko wrote:
>
>> +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
>> +       if (!(val & BIT(7)))
>> +               data->tol *= -1;
> Shouldn't you simple use corresponding sign_extend*()?
The data is encoded a sign-magnitude. sign_extend() works for two's 
complement numbers.

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 8/10/21 2:49 PM, Andy Shevchenko wrote:
> >
> >> +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> >> +       if (!(val & BIT(7)))
> >> +               data->tol *= -1;
> > Shouldn't you simple use corresponding sign_extend*()?
> The data is encoded a sign-magnitude. sign_extend() works for two's
> complement numbers.

Good catch!
I'm wondering if it's a good idea to have a sign_magnitude_to_int()
helper or so?

-- 
With Best Regards,
Andy Shevchenko

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

[Mugilraj Dhavachelvan]

On Wed, Aug 11, 2021 at 07:06:43PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> > On 8/10/21 2:49 PM, Andy Shevchenko wrote:
> > >
> > >> +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > >> +       if (!(val & BIT(7)))
> > >> +               data->tol *= -1;
> > > Shouldn't you simple use corresponding sign_extend*()?
> > The data is encoded a sign-magnitude. sign_extend() works for two's
> > complement numbers.
> 
> Good catch!
> I'm wondering if it's a good idea to have a sign_magnitude_to_int()
> helper or so?
>
So, What should I do now?

> -- 
> With Best Regards,
> Andy Shevchenko

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

[Mugilraj Dhavachelvan]

On Thu, Aug 12, 2021 at 10:23:41PM +0530, Mugilraj Dhavachelvan wrote:
> On Wed, Aug 11, 2021 at 07:06:43PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > On 8/10/21 2:49 PM, Andy Shevchenko wrote:
> > > >
> > > >> +       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
> > > >> +       if (!(val & BIT(7)))
> > > >> +               data->tol *= -1;
> > > > Shouldn't you simple use corresponding sign_extend*()?
> > > The data is encoded a sign-magnitude. sign_extend() works for two's
> > > complement numbers.
> > 
> > Good catch!
> > I'm wondering if it's a good idea to have a sign_magnitude_to_int()
> > helper or so?
> >
> So, What should I do now?
> 
Will send v3 with other changes and add this change in v4. Hope it's
okay.
> > -- 
> > With Best Regards,
> > Andy Shevchenko