[PATCH v3 0/6] iio: potentiometer: Add support for DS3502

[PATCH v3 0/6] iio: potentiometer: Add support for DS3502

[Jagath Jog J]

Add dt-bindings and support for Maxim DS3502 into existing ds1803 driver.
DS3502 is a 7 bit Nonvolatile Digital Potentiometer.

Changes since v2:
1. Addressed Andy Shevchenko comments.
2. Adding device name in Kconfig file.
3. Spliting up of patch into 3 patchs.
4. Adding channel info into ds1803_cfg in separate patch.
5. Dropping the use of enum in firmware data instead using previous
   pointer method for accessing device specific data.
6. Separate patch for using firmware provided data instead of 
   id->driver_data.
7. Adding DS3502 support in separate patch.

Changes since v1:
1. Fixes the alignment to match the open parenthesis in separate patch.
2. Adding available functionality for ds1803 driver in separate patch.
3. Moving maxim_potentiometer members into ds1803_cfg structure.
4. Droping of the INFO_ENABLE channel type.
5. Firmware entry with data is used instead of id->driver_data to
   to retrieve the chip specific data.

Jagath Jog J (6):
  iio: potentiometer: Alignment to match the open parenthesis
  iio: potentiometer: Add available functionality
  iio: potentiometer: Add channel information in device data
  iio: potentiometer: Change to firmware provided data
  iio: potentiometer: Add support for Maxim DS3502
  dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 drivers/iio/potentiometer/Kconfig             |   6 +-
 drivers/iio/potentiometer/ds1803.c            | 136 +++++++++++++-----
 3 files changed, 103 insertions(+), 41 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] iio: potentiometer: Alignment to match the open parenthesis

[Jagath Jog J]

Fix following checkpatch.pl check by removing blank space.
CHECK: Alignment should match open parenthesis.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/potentiometer/ds1803.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index 20b45407eaac..3c542a50ece6 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -55,8 +55,8 @@ static const struct iio_chan_spec ds1803_channels[] = {
 };
 
 static int ds1803_read_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int *val, int *val2, long mask)
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
 {
 	struct ds1803_data *data = iio_priv(indio_dev);
 	int pot = chan->channel;
@@ -66,7 +66,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = i2c_master_recv(data->client, result,
-				indio_dev->num_channels);
+				      indio_dev->num_channels);
 		if (ret < 0)
 			return ret;
 
@@ -83,8 +83,8 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
 }
 
 static int ds1803_write_raw(struct iio_dev *indio_dev,
-			     struct iio_chan_spec const *chan,
-			     int val, int val2, long mask)
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
 {
 	struct ds1803_data *data = iio_priv(indio_dev);
 	int pot = chan->channel;
@@ -109,8 +109,7 @@ static const struct iio_info ds1803_info = {
 	.write_raw = ds1803_write_raw,
 };
 
-static int ds1803_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
 	struct ds1803_data *data;
-- 
2.17.1

[PATCH v3 2/6] iio: potentiometer: Add available functionality

[Jagath Jog J]

Adding available functionality for DS1803 driver which
will show the minimum, step and maximum values that the
driver can accepts through sysfs entry.
Now using the max value present in avail array instead of chip
type specific macro to make the driver flexible to add other
type of potentiometer with different max position value.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/potentiometer/ds1803.c | 50 ++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index 3c542a50ece6..d1e00dda337a 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -16,7 +16,6 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 
-#define DS1803_MAX_POS		255
 #define DS1803_WRITE(chan)	(0xa8 | ((chan) + 1))
 
 enum ds1803_type {
@@ -26,27 +25,23 @@ enum ds1803_type {
 };
 
 struct ds1803_cfg {
+	int avail[3];
 	int kohms;
 };
 
-static const struct ds1803_cfg ds1803_cfg[] = {
-	[DS1803_010] = { .kohms =  10, },
-	[DS1803_050] = { .kohms =  50, },
-	[DS1803_100] = { .kohms = 100, },
-};
-
 struct ds1803_data {
 	struct i2c_client *client;
 	const struct ds1803_cfg *cfg;
 };
 
-#define DS1803_CHANNEL(ch) {					\
-	.type = IIO_RESISTANCE,					\
-	.indexed = 1,						\
-	.output = 1,						\
-	.channel = (ch),					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+#define DS1803_CHANNEL(ch) {						\
+	.type = IIO_RESISTANCE,						\
+	.indexed = 1,							\
+	.output = 1,							\
+	.channel = (ch),						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),   \
 }
 
 static const struct iio_chan_spec ds1803_channels[] = {
@@ -54,6 +49,12 @@ static const struct iio_chan_spec ds1803_channels[] = {
 	DS1803_CHANNEL(1),
 };
 
+static const struct ds1803_cfg ds1803_cfg[] = {
+	[DS1803_010] = { .avail = { 0, 1, 255 }, .kohms =  10, },
+	[DS1803_050] = { .avail = { 0, 1, 255 }, .kohms =  50, },
+	[DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, },
+};
+
 static int ds1803_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -75,7 +76,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1000 * data->cfg->kohms;
-		*val2 = DS1803_MAX_POS;
+		*val2 = data->cfg->avail[2]; /* Max wiper position */
 		return IIO_VAL_FRACTIONAL;
 	}
 
@@ -88,13 +89,14 @@ static int ds1803_write_raw(struct iio_dev *indio_dev,
 {
 	struct ds1803_data *data = iio_priv(indio_dev);
 	int pot = chan->channel;
+	int max_pos = data->cfg->avail[2];
 
 	if (val2 != 0)
 		return -EINVAL;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		if (val > DS1803_MAX_POS || val < 0)
+		if (val > max_pos || val < 0)
 			return -EINVAL;
 		break;
 	default:
@@ -104,9 +106,25 @@ static int ds1803_write_raw(struct iio_dev *indio_dev,
 	return i2c_smbus_write_byte_data(data->client, DS1803_WRITE(pot), val);
 }
 
+static int ds1803_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length, long mask)
+{
+	struct ds1803_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*vals = data->cfg->avail;
+		*length = ARRAY_SIZE(data->cfg->avail);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+	}
+	return -EINVAL;
+}
+
 static const struct iio_info ds1803_info = {
 	.read_raw = ds1803_read_raw,
 	.write_raw = ds1803_write_raw,
+	.read_avail = ds1803_read_avail,
 };
 
 static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *id)
-- 
2.17.1

[PATCH v3 3/6] iio: potentiometer: Add channel information in device data

[Jagath Jog J]

Adding each device wiper count and channel information into
device private data.
Utilizing addr member of struct iio_chan_spec to get the
wiper register address so that the value can be read or write
to the same address.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/potentiometer/ds1803.c | 33 ++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index d1e00dda337a..0ef3acb13c79 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -16,7 +16,8 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 
-#define DS1803_WRITE(chan)	(0xa8 | ((chan) + 1))
+#define DS1803_WIPER_0         0xA9
+#define DS1803_WIPER_1         0xAA
 
 enum ds1803_type {
 	DS1803_010,
@@ -25,8 +26,11 @@ enum ds1803_type {
 };
 
 struct ds1803_cfg {
+	int wipers;
 	int avail[3];
 	int kohms;
+	const struct iio_chan_spec *channels;
+	u8 num_channels;
 };
 
 struct ds1803_data {
@@ -34,25 +38,32 @@ struct ds1803_data {
 	const struct ds1803_cfg *cfg;
 };
 
-#define DS1803_CHANNEL(ch) {						\
+#define DS1803_CHANNEL(ch, addr) {					\
 	.type = IIO_RESISTANCE,						\
 	.indexed = 1,							\
 	.output = 1,							\
 	.channel = (ch),						\
+	.address = (addr),						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),   \
 }
 
 static const struct iio_chan_spec ds1803_channels[] = {
-	DS1803_CHANNEL(0),
-	DS1803_CHANNEL(1),
+	DS1803_CHANNEL(0, DS1803_WIPER_0),
+	DS1803_CHANNEL(1, DS1803_WIPER_1),
 };
 
 static const struct ds1803_cfg ds1803_cfg[] = {
-	[DS1803_010] = { .avail = { 0, 1, 255 }, .kohms =  10, },
-	[DS1803_050] = { .avail = { 0, 1, 255 }, .kohms =  50, },
-	[DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, },
+	[DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
+			 .channels = ds1803_channels,
+			 .num_channels = ARRAY_SIZE(ds1803_channels) },
+	[DS1803_050] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  50,
+			 .channels = ds1803_channels,
+			 .num_channels = ARRAY_SIZE(ds1803_channels) },
+	[DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
+			 .channels = ds1803_channels,
+			 .num_channels = ARRAY_SIZE(ds1803_channels) },
 };
 
 static int ds1803_read_raw(struct iio_dev *indio_dev,
@@ -88,7 +99,7 @@ static int ds1803_write_raw(struct iio_dev *indio_dev,
 			    int val, int val2, long mask)
 {
 	struct ds1803_data *data = iio_priv(indio_dev);
-	int pot = chan->channel;
+	u8 addr = chan->address;
 	int max_pos = data->cfg->avail[2];
 
 	if (val2 != 0)
@@ -103,7 +114,7 @@ static int ds1803_write_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
-	return i2c_smbus_write_byte_data(data->client, DS1803_WRITE(pot), val);
+	return i2c_smbus_write_byte_data(data->client, addr, val);
 }
 
 static int ds1803_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
@@ -144,8 +155,8 @@ static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *i
 	data->cfg = &ds1803_cfg[id->driver_data];
 
 	indio_dev->info = &ds1803_info;
-	indio_dev->channels = ds1803_channels;
-	indio_dev->num_channels = ARRAY_SIZE(ds1803_channels);
+	indio_dev->channels = data->cfg->channels;
+	indio_dev->num_channels = data->cfg->num_channels;
 	indio_dev->name = client->name;
 
 	return devm_iio_device_register(dev, indio_dev);
-- 
2.17.1

Re: [PATCH v3 3/6] iio: potentiometer: Add channel information in device data

[Andy Shevchenko]

On Fri, Feb 18, 2022 at 5:20 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Adding each device wiper count and channel information into
> device private data.
> Utilizing addr member of struct iio_chan_spec to get the
> wiper register address so that the value can be read or write
> to the same address.

Looks much better!

...

>  static const struct ds1803_cfg ds1803_cfg[] = {
> -       [DS1803_010] = { .avail = { 0, 1, 255 }, .kohms =  10, },
> -       [DS1803_050] = { .avail = { 0, 1, 255 }, .kohms =  50, },
> -       [DS1803_100] = { .avail = { 0, 1, 255 }, .kohms = 100, },
> +       [DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
> +                        .channels = ds1803_channels,
> +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> +       [DS1803_050] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  50,
> +                        .channels = ds1803_channels,
> +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> +       [DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
> +                        .channels = ds1803_channels,
> +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
>  };

Now, you may see you touch still the lines that are not changed, so
consider in the _previous_ patch to define each entry like the
following:

  [DS1803_...] = {
    .avail = {...},
    .kohms = ...,
  },

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/6] iio: potentiometer: Change to firmware provided data

[Jagath Jog J]

Using firmware provided data to get the device specific
structure, if not available fall back to id->driver_data.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/potentiometer/ds1803.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index 0ef3acb13c79..ca28ad147402 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -152,7 +152,9 @@ static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *i
 
 	data = iio_priv(indio_dev);
 	data->client = client;
-	data->cfg = &ds1803_cfg[id->driver_data];
+	data->cfg = device_get_match_data(dev);
+	if (!data->cfg)
+		data->cfg = &ds1803_cfg[id->driver_data];
 
 	indio_dev->info = &ds1803_info;
 	indio_dev->channels = data->cfg->channels;
-- 
2.17.1

Re: [PATCH v3 4/6] iio: potentiometer: Change to firmware provided data

[Andy Shevchenko]

On Fri, Feb 18, 2022 at 5:20 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Using firmware provided data to get the device specific
> structure, if not available fall back to id->driver_data.

You probably will need to add

#include <linux/property.h>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/6] iio: potentiometer: Add support for Maxim DS3502

[Jagath Jog J]

The DS3502 is a 7-bit, nonvolatile digital potentiometer featuring
an output voltage range of up to 15.5V.
DS3502 support is implemented into existing DS1803 driver.
Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/potentiometer/Kconfig  |  6 ++--
 drivers/iio/potentiometer/ds1803.c | 46 ++++++++++++++++++++++++------
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 832df8da2bc6..79c524640196 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -27,11 +27,11 @@ config AD5272
 	  module will be called ad5272.
 
 config DS1803
-	tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
+	tristate "Maxim Integrated DS1803 and similar Digital Potentiometer driver"
 	depends on I2C
 	help
-	  Say yes here to build support for the Maxim Integrated DS1803
-	  digital potentiometer chip.
+	  Say yes here to build support for the Maxim Integrated Devices DS1803 and
+	  DS3502 digital potentiometer chip.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ds1803.
diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index ca28ad147402..ca903e9c2816 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -1,12 +1,15 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Maxim Integrated DS1803 digital potentiometer driver
+ * Maxim Integrated DS1803 and similar digital potentiometer driver
  * Copyright (c) 2016 Slawomir Stepien
+ * Copyright (c) 2022 Jagath Jog J
  *
  * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf
+ * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
  *
  * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
  * ds1803	2	256		10, 50, 100		0101xxx
+ * ds3502	1	128		10			01010xx
  */
 
 #include <linux/err.h>
@@ -18,11 +21,13 @@
 
 #define DS1803_WIPER_0         0xA9
 #define DS1803_WIPER_1         0xAA
+#define DS3502_WR_IVR          0x00
 
 enum ds1803_type {
 	DS1803_010,
 	DS1803_050,
 	DS1803_100,
+	DS3502,
 };
 
 struct ds1803_cfg {
@@ -36,6 +41,7 @@ struct ds1803_cfg {
 struct ds1803_data {
 	struct i2c_client *client;
 	const struct ds1803_cfg *cfg;
+	enum ds1803_type chip_type;
 };
 
 #define DS1803_CHANNEL(ch, addr) {					\
@@ -54,6 +60,10 @@ static const struct iio_chan_spec ds1803_channels[] = {
 	DS1803_CHANNEL(1, DS1803_WIPER_1),
 };
 
+static const struct iio_chan_spec ds3502_channels[] = {
+	DS1803_CHANNEL(0, DS3502_WR_IVR),
+};
+
 static const struct ds1803_cfg ds1803_cfg[] = {
 	[DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
 			 .channels = ds1803_channels,
@@ -64,6 +74,9 @@ static const struct ds1803_cfg ds1803_cfg[] = {
 	[DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
 			 .channels = ds1803_channels,
 			 .num_channels = ARRAY_SIZE(ds1803_channels) },
+	[DS3502] =     { .wipers = 1, .avail = { 0, 1, 127 }, .kohms =  10,
+			 .channels = ds3502_channels,
+			 .num_channels = ARRAY_SIZE(ds3502_channels) },
 };
 
 static int ds1803_read_raw(struct iio_dev *indio_dev,
@@ -77,13 +90,26 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_master_recv(data->client, result,
-				      indio_dev->num_channels);
-		if (ret < 0)
-			return ret;
-
-		*val = result[pot];
-		return IIO_VAL_INT;
+		switch (data->chip_type) {
+		case DS1803_010:
+		case DS1803_050:
+		case DS1803_100:
+			ret = i2c_master_recv(data->client, result,
+					      indio_dev->num_channels);
+			if (ret < 0)
+				return ret;
+			*val = result[pot];
+			return IIO_VAL_INT;
+		case DS3502:
+			ret = i2c_smbus_read_byte_data(data->client,
+						       chan->address);
+			if (ret < 0)
+				return ret;
+			*val = ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1000 * data->cfg->kohms;
@@ -156,6 +182,7 @@ static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *i
 	if (!data->cfg)
 		data->cfg = &ds1803_cfg[id->driver_data];
 
+	data->chip_type = id->driver_data;
 	indio_dev->info = &ds1803_info;
 	indio_dev->channels = data->cfg->channels;
 	indio_dev->num_channels = data->cfg->num_channels;
@@ -168,6 +195,7 @@ static const struct of_device_id ds1803_dt_ids[] = {
 	{ .compatible = "maxim,ds1803-010", .data = &ds1803_cfg[DS1803_010] },
 	{ .compatible = "maxim,ds1803-050", .data = &ds1803_cfg[DS1803_050] },
 	{ .compatible = "maxim,ds1803-100", .data = &ds1803_cfg[DS1803_100] },
+	{ .compatible = "maxim,ds3502", .data = &ds1803_cfg[DS3502] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ds1803_dt_ids);
@@ -176,6 +204,7 @@ static const struct i2c_device_id ds1803_id[] = {
 	{ "ds1803-010", DS1803_010 },
 	{ "ds1803-050", DS1803_050 },
 	{ "ds1803-100", DS1803_100 },
+	{ "ds3502", DS3502 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ds1803_id);
@@ -192,5 +221,6 @@ static struct i2c_driver ds1803_driver = {
 module_i2c_driver(ds1803_driver);
 
 MODULE_AUTHOR("Slawomir Stepien <sst@poczta.fm>");
+MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
 MODULE_DESCRIPTION("DS1803 digital potentiometer");
 MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v3 5/6] iio: potentiometer: Add support for Maxim DS3502

[Jonathan Cameron]

On Fri, 18 Feb 2022 09:50:37 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> The DS3502 is a 7-bit, nonvolatile digital potentiometer featuring
> an output voltage range of up to 15.5V.
> DS3502 support is implemented into existing DS1803 driver.
> Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

Hi Jagath,

One suggestion from my review of v2.  Instead of putting the enum
for type in the per chip type cfg structure, use a function pointer
and break out the device specific bits of the read_raw() into
separate functions accessed via that function pointer.

Thanks,

Jonathan

> ---
>  drivers/iio/potentiometer/Kconfig  |  6 ++--
>  drivers/iio/potentiometer/ds1803.c | 46 ++++++++++++++++++++++++------
>  2 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 832df8da2bc6..79c524640196 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -27,11 +27,11 @@ config AD5272
>  	  module will be called ad5272.
>  
>  config DS1803
> -	tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> +	tristate "Maxim Integrated DS1803 and similar Digital Potentiometer driver"
>  	depends on I2C
>  	help
> -	  Say yes here to build support for the Maxim Integrated DS1803
> -	  digital potentiometer chip.
> +	  Say yes here to build support for the Maxim Integrated Devices DS1803 and
> +	  DS3502 digital potentiometer chip.
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ds1803.
> diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
> index ca28ad147402..ca903e9c2816 100644
> --- a/drivers/iio/potentiometer/ds1803.c
> +++ b/drivers/iio/potentiometer/ds1803.c
> @@ -1,12 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Maxim Integrated DS1803 digital potentiometer driver
> + * Maxim Integrated DS1803 and similar digital potentiometer driver
>   * Copyright (c) 2016 Slawomir Stepien
> + * Copyright (c) 2022 Jagath Jog J
>   *
>   * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1803.pdf
> + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
>   *
>   * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
>   * ds1803	2	256		10, 50, 100		0101xxx
> + * ds3502	1	128		10			01010xx
>   */
>  
>  #include <linux/err.h>
> @@ -18,11 +21,13 @@
>  
>  #define DS1803_WIPER_0         0xA9
>  #define DS1803_WIPER_1         0xAA
> +#define DS3502_WR_IVR          0x00
>  
>  enum ds1803_type {
>  	DS1803_010,
>  	DS1803_050,
>  	DS1803_100,
> +	DS3502,
>  };
>  
>  struct ds1803_cfg {
> @@ -36,6 +41,7 @@ struct ds1803_cfg {
>  struct ds1803_data {
>  	struct i2c_client *client;
>  	const struct ds1803_cfg *cfg;
> +	enum ds1803_type chip_type;

Hmm. I'm not that keen on this backwards reference (effectively).
Consider using a read() function pointer in here instead.

>  };
>  
>  #define DS1803_CHANNEL(ch, addr) {					\
> @@ -54,6 +60,10 @@ static const struct iio_chan_spec ds1803_channels[] = {
>  	DS1803_CHANNEL(1, DS1803_WIPER_1),
>  };
>  
> +static const struct iio_chan_spec ds3502_channels[] = {
> +	DS1803_CHANNEL(0, DS3502_WR_IVR),
> +};
> +
>  static const struct ds1803_cfg ds1803_cfg[] = {
>  	[DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
>  			 .channels = ds1803_channels,
> @@ -64,6 +74,9 @@ static const struct ds1803_cfg ds1803_cfg[] = {
>  	[DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
>  			 .channels = ds1803_channels,
>  			 .num_channels = ARRAY_SIZE(ds1803_channels) },
> +	[DS3502] =     { .wipers = 1, .avail = { 0, 1, 127 }, .kohms =  10,
> +			 .channels = ds3502_channels,
> +			 .num_channels = ARRAY_SIZE(ds3502_channels) },
>  };
>  
>  static int ds1803_read_raw(struct iio_dev *indio_dev,
> @@ -77,13 +90,26 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = i2c_master_recv(data->client, result,
> -				      indio_dev->num_channels);
> -		if (ret < 0)
> -			return ret;
> -
> -		*val = result[pot];
> -		return IIO_VAL_INT;
> +		switch (data->chip_type) {
> +		case DS1803_010:
> +		case DS1803_050:
> +		case DS1803_100:

This block can become
			ret = data->read(indio_dev, chan, val);
or something along those lines and you can remove the switch statement
as it's a simple lookup.

> +			ret = i2c_master_recv(data->client, result,
> +					      indio_dev->num_channels);
> +			if (ret < 0)
> +				return ret;
> +			*val = result[pot];
> +			return IIO_VAL_INT;
> +		case DS3502:
> +			ret = i2c_smbus_read_byte_data(data->client,
> +						       chan->address);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 1000 * data->cfg->kohms;
> @@ -156,6 +182,7 @@ static int ds1803_probe(struct i2c_client *client, const struct i2c_device_id *i
>  	if (!data->cfg)
>  		data->cfg = &ds1803_cfg[id->driver_data];
>  
> +	data->chip_type = id->driver_data;
>  	indio_dev->info = &ds1803_info;
>  	indio_dev->channels = data->cfg->channels;
>  	indio_dev->num_channels = data->cfg->num_channels;
> @@ -168,6 +195,7 @@ static const struct of_device_id ds1803_dt_ids[] = {
>  	{ .compatible = "maxim,ds1803-010", .data = &ds1803_cfg[DS1803_010] },
>  	{ .compatible = "maxim,ds1803-050", .data = &ds1803_cfg[DS1803_050] },
>  	{ .compatible = "maxim,ds1803-100", .data = &ds1803_cfg[DS1803_100] },
> +	{ .compatible = "maxim,ds3502", .data = &ds1803_cfg[DS3502] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ds1803_dt_ids);
> @@ -176,6 +204,7 @@ static const struct i2c_device_id ds1803_id[] = {
>  	{ "ds1803-010", DS1803_010 },
>  	{ "ds1803-050", DS1803_050 },
>  	{ "ds1803-100", DS1803_100 },
> +	{ "ds3502", DS3502 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, ds1803_id);
> @@ -192,5 +221,6 @@ static struct i2c_driver ds1803_driver = {
>  module_i2c_driver(ds1803_driver);
>  
>  MODULE_AUTHOR("Slawomir Stepien <sst@poczta.fm>");
> +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
>  MODULE_DESCRIPTION("DS1803 digital potentiometer");
>  MODULE_LICENSE("GPL v2");

Re: [PATCH v3 5/6] iio: potentiometer: Add support for Maxim DS3502

[Andy Shevchenko]

On Fri, Feb 18, 2022 at 5:20 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> The DS3502 is a 7-bit, nonvolatile digital potentiometer featuring
> an output voltage range of up to 15.5V.
> DS3502 support is implemented into existing DS1803 driver.


> Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

Please, make sure the Datasheet will be in the tag block (tag block
doesn't allow blank lines).

1. summary
2. blank line
3. long description (may contain blank lines)
4. blank line
5. tag block (may not contain blank lines)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 6/6] dt-bindings: iio: potentiometer: Add Maxim DS3502 in trivial-devices

[Jagath Jog J]

Maxim DS3502 is a 7 bit nonvolatile digital potentiometer.
Add DS3502 binding into trivial-devices.yaml.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 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 091792ba993e..b6187603317a 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -157,6 +157,8 @@ properties:
           - maxim,ds1803-050
             # 100 kOhm digital potentiometer with I2C interface
           - maxim,ds1803-100
+            # 10 kOhm digital potentiometer with I2C interface
+          - maxim,ds3502
             # Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs
           - maxim,max1237
             # Temperature Sensor, I2C interface
-- 
2.17.1