[PATCH 0/3] Add support for LTC2499 ADC

[PATCH 0/3] Add support for LTC2499 ADC

[Ciprian Regus]

The LTC2499 is a 16-channel (eight differential), 24-bit,
ADC with Easy Drive technology and a 2-wire, I2C interface.

This adds support for the LTC2499 ADC by extending the LTC2497
driver.

Note: This fix is required to be applied first:
https://patchwork.kernel.org/project/linux-iio/patch/20220815091647.1523532-1-dzagorui@cisco.com

Ciprian Regus (3):
  dt-bindings: iio: adc: Add docs for LTC2499
  drivers: iio: adc: LTC2499 support
  drivers: iio: adc: Rename the LTC249x iio device

 .../bindings/iio/adc/lltc,ltc2497.yaml        |  6 +++-
 MAINTAINERS                                   |  1 +
 drivers/iio/adc/ltc2496.c                     |  9 ++++-
 drivers/iio/adc/ltc2497-core.c                |  4 +--
 drivers/iio/adc/ltc2497.c                     | 36 ++++++++++++++++---
 drivers/iio/adc/ltc2497.h                     | 14 +++++++-
 6 files changed, 60 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499

[Ciprian Regus]

Update the bindings documentation for ltc2497 to include the ltc2499.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++-
 MAINTAINERS                                                 | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
index c1772b568cd1..7bb30eafc543 100644
--- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
@@ -13,10 +13,14 @@ description: |
   16bit ADC supporting up to 16 single ended or 8 differential inputs.
   I2C interface.
 
+  https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
+
 properties:
   compatible:
-    const:
+    enum:
       lltc,ltc2497
+      lltc,ltc2499
 
   reg: true
   vref-supply: true
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..3c847619ceb1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1327,6 +1327,7 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523
 F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
 F:	Documentation/devicetree/bindings/iio/*/adi,*
+F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml
 F:	drivers/iio/*/ad*
 F:	drivers/iio/adc/ltc249*
-- 
2.30.2

[PATCH 2/3] drivers: iio: adc: LTC2499 support

[Ciprian Regus]

The LTC2499 is a 16-channel (eight differential), 24-bit,
ADC with Easy Drive technology and a 2-wire, I2C interface.

Implement support for the LTC2499 ADC by extending the LTC2497
driver. A new chip_info struct is added to differentiate between
chip types and resolutions when reading data from the device.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
 drivers/iio/adc/ltc2496.c      |  8 +++++++-
 drivers/iio/adc/ltc2497-core.c |  2 +-
 drivers/iio/adc/ltc2497.c      | 34 +++++++++++++++++++++++++++++-----
 drivers/iio/adc/ltc2497.h      | 13 ++++++++++++-
 4 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
index dfb3bb5997e5..98338104c24a 100644
--- a/drivers/iio/adc/ltc2496.c
+++ b/drivers/iio/adc/ltc2496.c
@@ -14,6 +14,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/mod_devicetable.h>
 
 #include "ltc2497.h"
@@ -74,6 +75,7 @@ static int ltc2496_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, indio_dev);
 	st->spi = spi;
 	st->common_ddata.result_and_measure = ltc2496_result_and_measure;
+	st->common_ddata.chip_info = device_get_match_data(dev);
 
 	return ltc2497core_probe(dev, indio_dev);
 }
@@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi)
 	ltc2497core_remove(indio_dev);
 }
 
+static struct chip_info ltc2496_info = {
+	.resolution = 16,
+};
+
 static const struct of_device_id ltc2496_of_match[] = {
-	{ .compatible = "lltc,ltc2496", },
+	{ .compatible = "lltc,ltc2496", .data = (void *)&ltc2496_info },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ltc2496_of_match);
diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
index 2a485c8a1940..b2752399402c 100644
--- a/drivers/iio/adc/ltc2497-core.c
+++ b/drivers/iio/adc/ltc2497-core.c
@@ -95,7 +95,7 @@ static int ltc2497core_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		*val = ret / 1000;
-		*val2 = 17;
+		*val2 = ddata->chip_info->resolution + 1;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 
diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
index f7c786f37ceb..bb5e0d4301e2 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -7,10 +7,14 @@
  * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
  */
 
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/mod_devicetable.h>
 
 #include "ltc2497.h"
@@ -19,6 +23,8 @@ struct ltc2497_driverdata {
 	/* this must be the first member */
 	struct ltc2497core_driverdata common_ddata;
 	struct i2c_client *client;
+	u32 recv_size;
+	u32 sub_lsb;
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
 	 * transfer buffers to live in their own cache lines.
@@ -34,13 +40,14 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 	int ret;
 
 	if (val) {
-		ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
+		ret = i2c_master_recv(st->client, (char *)&st->buf, st->recv_size);
 		if (ret < 0) {
 			dev_err(&st->client->dev, "i2c_master_recv failed\n");
 			return ret;
 		}
 
-		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
+		*val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
+			BIT(ddata->chip_info->resolution + 1);
 	}
 
 	ret = i2c_smbus_write_byte(st->client,
@@ -54,6 +61,7 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 static int ltc2497_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	u32 resolution;
 	struct iio_dev *indio_dev;
 	struct ltc2497_driverdata *st;
 	struct device *dev = &client->dev;
@@ -70,6 +78,11 @@ static int ltc2497_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, indio_dev);
 	st->client = client;
 	st->common_ddata.result_and_measure = ltc2497_result_and_measure;
+	st->common_ddata.chip_info = device_get_match_data(dev);
+
+	resolution = st->common_ddata.chip_info->resolution;
+	st->sub_lsb = 31 - (resolution + 1);
+	st->recv_size = resolution / BITS_PER_BYTE + 1;
 
 	return ltc2497core_probe(dev, indio_dev);
 }
@@ -83,15 +96,26 @@ static int ltc2497_remove(struct i2c_client *client)
 	return 0;
 }
 
+static struct chip_info ltc2497_info[] = {
+	[TYPE_LTC2497] = {
+		.resolution = 16,
+	},
+	[TYPE_LTC2499] = {
+		.resolution = 24,
+	}
+};
+
 static const struct i2c_device_id ltc2497_id[] = {
-	{ "ltc2497", 0 },
+	{ "ltc2497", TYPE_LTC2497 },
+	{ "ltc2499", TYPE_LTC2499 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ltc2497_id);
 
 static const struct of_device_id ltc2497_of_match[] = {
-	{ .compatible = "lltc,ltc2497", },
-	{},
+	{ .compatible = "lltc,ltc2497", .data = (void *)&ltc2497_info[TYPE_LTC2497] },
+	{ .compatible = "lltc,ltc2499", .data = (void *)&ltc2497_info[TYPE_LTC2499] },
+	{}
 };
 MODULE_DEVICE_TABLE(of, ltc2497_of_match);
 
diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
index d0b42dd6b8ad..f4d939cfd48b 100644
--- a/drivers/iio/adc/ltc2497.h
+++ b/drivers/iio/adc/ltc2497.h
@@ -4,9 +4,20 @@
 #define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
 #define LTC2497_CONVERSION_TIME_MS	150ULL
 
+enum chip_type {
+	TYPE_LTC2496,
+	TYPE_LTC2497,
+	TYPE_LTC2499
+};
+
+struct chip_info {
+	u32 resolution;
+};
+
 struct ltc2497core_driverdata {
 	struct regulator *ref;
-	ktime_t	time_prev;
+	ktime_t time_prev;
+	const struct chip_info *chip_info;
 	u8 addr_prev;
 	int (*result_and_measure)(struct ltc2497core_driverdata *ddata,
 				  u8 address, int *val);
-- 
2.30.2

[PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Ciprian Regus]

Set the iio device's name based on the chip used.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
 drivers/iio/adc/ltc2496.c      | 1 +
 drivers/iio/adc/ltc2497-core.c | 2 +-
 drivers/iio/adc/ltc2497.c      | 2 ++
 drivers/iio/adc/ltc2497.h      | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
index 98338104c24a..86470f49e8ca 100644
--- a/drivers/iio/adc/ltc2496.c
+++ b/drivers/iio/adc/ltc2496.c
@@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi)
 
 static struct chip_info ltc2496_info = {
 	.resolution = 16,
+	.name = "ltc2496"
 };
 
 static const struct of_device_id ltc2496_of_match[] = {
diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
index b2752399402c..6dd9ab601904 100644
--- a/drivers/iio/adc/ltc2497-core.c
+++ b/drivers/iio/adc/ltc2497-core.c
@@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
 	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
 	int ret;
 
-	indio_dev->name = dev_name(dev);
+	indio_dev->name = ddata->chip_info->name;
 	indio_dev->info = &ltc2497core_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ltc2497core_channel;
diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
index bb5e0d4301e2..a0aad71c8130 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client)
 static struct chip_info ltc2497_info[] = {
 	[TYPE_LTC2497] = {
 		.resolution = 16,
+		.name = "ltc2497"
 	},
 	[TYPE_LTC2499] = {
 		.resolution = 24,
+		.name = "ltc2499"
 	}
 };
 
diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
index f4d939cfd48b..0e86e38248ee 100644
--- a/drivers/iio/adc/ltc2497.h
+++ b/drivers/iio/adc/ltc2497.h
@@ -12,6 +12,7 @@ enum chip_type {
 
 struct chip_info {
 	u32 resolution;
+	char *name;
 };
 
 struct ltc2497core_driverdata {
-- 
2.30.2

Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Lars-Peter Clausen]

On 8/22/22 14:51, Ciprian Regus wrote:
> Set the iio device's name based on the chip used.

While the change is correct it breaks the ABI. This needs a bit of a 
better explanation what is being done, why, what are the potential 
problems, why do we want to do it anyway.

>
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> ---
>   drivers/iio/adc/ltc2496.c      | 1 +
>   drivers/iio/adc/ltc2497-core.c | 2 +-
>   drivers/iio/adc/ltc2497.c      | 2 ++
>   drivers/iio/adc/ltc2497.h      | 1 +
>   4 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> index 98338104c24a..86470f49e8ca 100644
> --- a/drivers/iio/adc/ltc2496.c
> +++ b/drivers/iio/adc/ltc2496.c
> @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi)
>   
>   static struct chip_info ltc2496_info = {
>   	.resolution = 16,
> +	.name = "ltc2496"
>   };
>   
>   static const struct of_device_id ltc2496_of_match[] = {
> diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
> index b2752399402c..6dd9ab601904 100644
> --- a/drivers/iio/adc/ltc2497-core.c
> +++ b/drivers/iio/adc/ltc2497-core.c
> @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
>   	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
>   	int ret;
>   
> -	indio_dev->name = dev_name(dev);
> +	indio_dev->name = ddata->chip_info->name;
>   	indio_dev->info = &ltc2497core_info;
>   	indio_dev->modes = INDIO_DIRECT_MODE;
>   	indio_dev->channels = ltc2497core_channel;
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> index bb5e0d4301e2..a0aad71c8130 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client)
>   static struct chip_info ltc2497_info[] = {
>   	[TYPE_LTC2497] = {
>   		.resolution = 16,
> +		.name = "ltc2497"
>   	},
>   	[TYPE_LTC2499] = {
>   		.resolution = 24,
> +		.name = "ltc2499"
>   	}
>   };
>   
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index f4d939cfd48b..0e86e38248ee 100644
> --- a/drivers/iio/adc/ltc2497.h
> +++ b/drivers/iio/adc/ltc2497.h
> @@ -12,6 +12,7 @@ enum chip_type {
>   
>   struct chip_info {
>   	u32 resolution;
> +	char *name;
>   };
>   
>   struct ltc2497core_driverdata {

Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Jonathan Cameron]

On Mon, 22 Aug 2022 16:08:12 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 8/22/22 14:51, Ciprian Regus wrote:
> > Set the iio device's name based on the chip used.  
> 
> While the change is correct it breaks the ABI. This needs a bit of a 
> better explanation what is being done, why, what are the potential 
> problems, why do we want to do it anyway.

+ it's a fix, so if we are doing this we need to enable backporting
by moving to first patch in series (obviously just for the
already supported parts).  

Jonathan

> 
> >
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> > ---
> >   drivers/iio/adc/ltc2496.c      | 1 +
> >   drivers/iio/adc/ltc2497-core.c | 2 +-
> >   drivers/iio/adc/ltc2497.c      | 2 ++
> >   drivers/iio/adc/ltc2497.h      | 1 +
> >   4 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> > index 98338104c24a..86470f49e8ca 100644
> > --- a/drivers/iio/adc/ltc2496.c
> > +++ b/drivers/iio/adc/ltc2496.c
> > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi)
> >   
> >   static struct chip_info ltc2496_info = {
> >   	.resolution = 16,
> > +	.name = "ltc2496"
> >   };
> >   
> >   static const struct of_device_id ltc2496_of_match[] = {
> > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
> > index b2752399402c..6dd9ab601904 100644
> > --- a/drivers/iio/adc/ltc2497-core.c
> > +++ b/drivers/iio/adc/ltc2497-core.c
> > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev)
> >   	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
> >   	int ret;
> >   
> > -	indio_dev->name = dev_name(dev);
> > +	indio_dev->name = ddata->chip_info->name;
> >   	indio_dev->info = &ltc2497core_info;
> >   	indio_dev->modes = INDIO_DIRECT_MODE;
> >   	indio_dev->channels = ltc2497core_channel;
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > index bb5e0d4301e2..a0aad71c8130 100644
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client)
> >   static struct chip_info ltc2497_info[] = {
> >   	[TYPE_LTC2497] = {
> >   		.resolution = 16,
> > +		.name = "ltc2497"
> >   	},
> >   	[TYPE_LTC2499] = {
> >   		.resolution = 24,
> > +		.name = "ltc2499"
> >   	}
> >   };
> >   
> > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> > index f4d939cfd48b..0e86e38248ee 100644
> > --- a/drivers/iio/adc/ltc2497.h
> > +++ b/drivers/iio/adc/ltc2497.h
> > @@ -12,6 +12,7 @@ enum chip_type {
> >   
> >   struct chip_info {
> >   	u32 resolution;
> > +	char *name;
> >   };
> >   
> >   struct ltc2497core_driverdata {  
> 
> 

Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499

[Jonathan Cameron]

On Mon, 22 Aug 2022 15:51:04 +0300
Ciprian Regus <ciprian.regus@analog.com> wrote:

> Update the bindings documentation for ltc2497 to include the ltc2499.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
Looks fine to me.  Maybe a separate patch to add the ltc2496 binding
given the driver is included in this MAINTAINERS entry?
A random comment inline.

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++-
>  MAINTAINERS                                                 | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> index c1772b568cd1..7bb30eafc543 100644
> --- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> @@ -13,10 +13,14 @@ description: |
>    16bit ADC supporting up to 16 single ended or 8 differential inputs.
>    I2C interface.
>  
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
> +
>  properties:
>    compatible:
> -    const:
> +    enum:
>        lltc,ltc2497
> +      lltc,ltc2499
>  
>    reg: true
>    vref-supply: true
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d7f64dc0efe..3c847619ceb1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1327,6 +1327,7 @@ W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523
>  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
>  F:	Documentation/devicetree/bindings/iio/*/adi,*
> +F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml

Unrelated question, but why is this here given the wild cards above match?

>  F:	drivers/iio/*/ad*
>  F:	drivers/iio/adc/ltc249*

Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Mon, 22 Aug 2022 15:51:05 +0300
Ciprian Regus <ciprian.regus@analog.com> wrote:

> The LTC2499 is a 16-channel (eight differential), 24-bit,
> ADC with Easy Drive technology and a 2-wire, I2C interface.
> 
> Implement support for the LTC2499 ADC by extending the LTC2497
> driver. A new chip_info struct is added to differentiate between
> chip types and resolutions when reading data from the device.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
Hi Ciprian,

Various comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ltc2496.c      |  8 +++++++-
>  drivers/iio/adc/ltc2497-core.c |  2 +-
>  drivers/iio/adc/ltc2497.c      | 34 +++++++++++++++++++++++++++++-----
>  drivers/iio/adc/ltc2497.h      | 13 ++++++++++++-
>  4 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> index dfb3bb5997e5..98338104c24a 100644
> --- a/drivers/iio/adc/ltc2496.c
> +++ b/drivers/iio/adc/ltc2496.c
> @@ -14,6 +14,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/driver.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
why?

>  #include <linux/mod_devicetable.h>
>  
>  #include "ltc2497.h"
> @@ -74,6 +75,7 @@ static int ltc2496_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, indio_dev);
>  	st->spi = spi;
>  	st->common_ddata.result_and_measure = ltc2496_result_and_measure;
> +	st->common_ddata.chip_info = device_get_match_data(dev);
>  
>  	return ltc2497core_probe(dev, indio_dev);
>  }
> @@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi)
>  	ltc2497core_remove(indio_dev);
>  }
>  
> +static struct chip_info ltc2496_info = {
> +	.resolution = 16,
> +};
> +
>  static const struct of_device_id ltc2496_of_match[] = {
> -	{ .compatible = "lltc,ltc2496", },
> +	{ .compatible = "lltc,ltc2496", .data = (void *)&ltc2496_info },

as below. Take the chip_info structure const and drop the cast.

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ltc2496_of_match);
> diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
> index 2a485c8a1940..b2752399402c 100644
> --- a/drivers/iio/adc/ltc2497-core.c
> +++ b/drivers/iio/adc/ltc2497-core.c
> @@ -95,7 +95,7 @@ static int ltc2497core_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		*val = ret / 1000;
> -		*val2 = 17;
> +		*val2 = ddata->chip_info->resolution + 1;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> index f7c786f37ceb..bb5e0d4301e2 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -7,10 +7,14 @@
>   * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>

Not immediately obvious why these need adding as part of this patch.
If just general include improvement, separate patch please.

> +
Why blank line?
>  #include <linux/i2c.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/driver.h>
>  #include <linux/module.h>
> +#include <linux/property.h>

May make sense, but not obvious why it's in this patch.
If it's missing and should be present, please do it in a separate patch.

>  #include <linux/mod_devicetable.h>
>  
>  #include "ltc2497.h"
> @@ -19,6 +23,8 @@ struct ltc2497_driverdata {
>  	/* this must be the first member */
>  	struct ltc2497core_driverdata common_ddata;
>  	struct i2c_client *client;
> +	u32 recv_size;
> +	u32 sub_lsb;

>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
>  	 * transfer buffers to live in their own cache lines.
> @@ -34,13 +40,14 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  	int ret;
>  
>  	if (val) {
> -		ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
> +		ret = i2c_master_recv(st->client, (char *)&st->buf, st->recv_size);
Not helpful yet, but there was a patch series under review form Jason Gerecke that
changed the i2c transfer commands to use a u8.  

Which raises a question. buf is a __be32 which is rather odd given size of 3 in original
code.  If we have a case where it needs to have separate types for different transfers, use
a union.

>  		if (ret < 0) {
>  			dev_err(&st->client->dev, "i2c_master_recv failed\n");
>  			return ret;
>  		}
>  
> -		*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);

Old code here is less than ideal, should be reading into 3 bytes then using
the be24 accesors. Please fix whilst here.  You will need multiple paths here
depending on size.

> +		*val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
> +			BIT(ddata->chip_info->resolution + 1);
>  	}
>  
>  	ret = i2c_smbus_write_byte(st->client,
> @@ -54,6 +61,7 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  static int ltc2497_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> +	u32 resolution;
>  	struct iio_dev *indio_dev;
>  	struct ltc2497_driverdata *st;
>  	struct device *dev = &client->dev;
> @@ -70,6 +78,11 @@ static int ltc2497_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, indio_dev);
>  	st->client = client;
>  	st->common_ddata.result_and_measure = ltc2497_result_and_measure;
> +	st->common_ddata.chip_info = device_get_match_data(dev);
> +
> +	resolution = st->common_ddata.chip_info->resolution;
> +	st->sub_lsb = 31 - (resolution + 1);
> +	st->recv_size = resolution / BITS_PER_BYTE + 1;
>  
>  	return ltc2497core_probe(dev, indio_dev);
>  }
> @@ -83,15 +96,26 @@ static int ltc2497_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static struct chip_info ltc2497_info[] = {

Should be const - which is why you need the cast below.

> +	[TYPE_LTC2497] = {
> +		.resolution = 16,
> +	},
> +	[TYPE_LTC2499] = {
> +		.resolution = 24,
> +	}
> +};
> +
>  static const struct i2c_device_id ltc2497_id[] = {
> -	{ "ltc2497", 0 },
> +	{ "ltc2497", TYPE_LTC2497 },
> +	{ "ltc2499", TYPE_LTC2499 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltc2497_id);
>  
>  static const struct of_device_id ltc2497_of_match[] = {
> -	{ .compatible = "lltc,ltc2497", },
> -	{},
> +	{ .compatible = "lltc,ltc2497", .data = (void *)&ltc2497_info[TYPE_LTC2497] },
> +	{ .compatible = "lltc,ltc2499", .data = (void *)&ltc2497_info[TYPE_LTC2499] },
Cast is a bad sign.  Reason above.

> +	{}
>  };
>  MODULE_DEVICE_TABLE(of, ltc2497_of_match);
>  
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index d0b42dd6b8ad..f4d939cfd48b 100644
> --- a/drivers/iio/adc/ltc2497.h
> +++ b/drivers/iio/adc/ltc2497.h
> @@ -4,9 +4,20 @@
>  #define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
>  #define LTC2497_CONVERSION_TIME_MS	150ULL
>  
> +enum chip_type {
> +	TYPE_LTC2496,
> +	TYPE_LTC2497,
> +	TYPE_LTC2499
> +};
> +
> +struct chip_info {
> +	u32 resolution;
> +};
> +
>  struct ltc2497core_driverdata {
>  	struct regulator *ref;
> -	ktime_t	time_prev;
> +	ktime_t time_prev;
I'm staring at this and can't see the difference between the two lines.
Closer inspection (i.e. messing with the stuff around it) tells me line
you are removing has a tab when should be a space.  Whilst trivial please
call that out in patch description (or do it as a separate patch).

> +	const struct chip_info *chip_info;
>  	u8 addr_prev;
>  	int (*result_and_measure)(struct ltc2497core_driverdata *ddata,
>  				  u8 address, int *val);

Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499

[Jonathan Cameron]

On Mon, 22 Aug 2022 20:24:35 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 22 Aug 2022 15:51:04 +0300
> Ciprian Regus <ciprian.regus@analog.com> wrote:
> 
> > Update the bindings documentation for ltc2497 to include the ltc2499.
> > 
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>  
> Looks fine to me.  Maybe a separate patch to add the ltc2496 binding
> given the driver is included in this MAINTAINERS entry?
> A random comment inline.
> 
> Jonathan
> 
> > ---
> >  Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++-
> >  MAINTAINERS                                                 | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> > index c1772b568cd1..7bb30eafc543 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> > @@ -13,10 +13,14 @@ description: |
> >    16bit ADC supporting up to 16 single ended or 8 differential inputs.
> >    I2C interface.
> >  
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
> > +
> >  properties:
> >    compatible:
> > -    const:
> > +    enum:
> >        lltc,ltc2497
> > +      lltc,ltc2499

Oops. Benefit of Rob's build bot later.

       enum:
         - lltc,ltc2497
         - lltc,ltc2499

Just goes to show how wonderful automatic checking is :)

Jonathan

> >  
> >    reg: true
> >    vref-supply: true
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9d7f64dc0efe..3c847619ceb1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1327,6 +1327,7 @@ W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523
> >  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
> >  F:	Documentation/devicetree/bindings/iio/*/adi,*
> > +F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> >  F:	Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml  
> 
> Unrelated question, but why is this here given the wild cards above match?
> 
> >  F:	drivers/iio/*/ad*
> >  F:	drivers/iio/adc/ltc249*  

Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499

[Rob Herring]

On Mon, 22 Aug 2022 15:51:04 +0300, Ciprian Regus wrote:
> Update the bindings documentation for ltc2497 to include the ltc2499.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++-
>  MAINTAINERS                                                 | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: ignoring, error in schema: properties: compatible: enum
Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.example.dtb:0:0: /example-0/i2c/adc@76: failed to match any schema with compatible: ['lltc,ltc2497']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Krzysztof Kozlowski]

On 22/08/2022 15:51, Ciprian Regus wrote:

(...)

>  
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index f4d939cfd48b..0e86e38248ee 100644
> --- a/drivers/iio/adc/ltc2497.h
> +++ b/drivers/iio/adc/ltc2497.h
> @@ -12,6 +12,7 @@ enum chip_type {
>  
>  struct chip_info {
>  	u32 resolution;
> +	char *name;

const char *


Best regards,
Krzysztof

Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 22 Aug 2022 15:51:05 +0300
> Ciprian Regus <ciprian.regus@analog.com> wrote:

In reply to Jonathan's comments to answer his question and add more
comments from me.

...

> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
> >
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>

Tag block mustn't have the blank line(s).

...

> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/driver.h>
> >  #include <linux/module.h>
> > +#include <linux/property.h>
> why?

device_get_match_data() requires it.

But why not sort them?

> >  #include <linux/mod_devicetable.h>

...

> > -             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
>
> Old code here is less than ideal, should be reading into 3 bytes then using
> the be24 accesors. Please fix whilst here.  You will need multiple paths here
> depending on size.
>
> > +             *val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
> > +                     BIT(ddata->chip_info->resolution + 1);

Shouldn't this use some kind of sign_extend()?

Also with a temporary variable for chip info this line can be single.

   struct ... *ci = ddata->chip_info;

   ...BIT(ci->resolution + 1)

...

> > +     u32 resolution;

Keep this in a way that the "longer lines go first".

...

> > +     resolution = st->common_ddata.chip_info->resolution;
> > +     st->sub_lsb = 31 - (resolution + 1);
> > +     st->recv_size = resolution / BITS_PER_BYTE + 1;

BITS_TO_BYTES()

...

> >  static const struct i2c_device_id ltc2497_id[] = {
> > -     { "ltc2497", 0 },
> > +     { "ltc2497", TYPE_LTC2497 },
> > +     { "ltc2499", TYPE_LTC2499 },

Use pointers here like you have done for the OF table.

> >       { }
> >  };

...

> > +enum chip_type {
> > +     TYPE_LTC2496,
> > +     TYPE_LTC2497,
> > +     TYPE_LTC2499

Keep trailing comma.

> > +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

> ...
> 
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/driver.h>
> > >  #include <linux/module.h>
> > > +#include <linux/property.h>  
> > why?  
> 
> device_get_match_data() requires it.

Good point :)  Not sure how I missed that!

RE: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Regus, Ciprian]

Hi Lars,

Thanks for the review. I have one question before going further
with this patch.

> On 8/22/22 14:51, Ciprian Regus wrote:
> > Set the iio device's name based on the chip used.
> 
> While the change is correct it breaks the ABI. This needs a bit of a
> better explanation what is being done, why, what are the potential
> problems, why do we want to do it anyway.

The reason for this change is to make it easier to identify the device
(by having a generic name) when using an IIO client. The current name is
based on the i2c_client's kobj name, which has the format
"i2c_instance -i2c_address " (e.g. 1-0076). So, the renaming would allow
for interacting with the device without knowing which bus it's connected
to.

Also, as far as I can see, the IIO ABI states that the device's name is
"Typically a part number".

For the reason given, would this change be considered an
improvement, or is the ABI breaking too much of a drawback?
(in which case I'll just drop this patch)

Thanks,
Ciprian
> 
> >
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> > ---
> >   drivers/iio/adc/ltc2496.c      | 1 +
> >   drivers/iio/adc/ltc2497-core.c | 2 +-
> >   drivers/iio/adc/ltc2497.c      | 2 ++
> >   drivers/iio/adc/ltc2497.h      | 1 +
> >   4 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> > index 98338104c24a..86470f49e8ca 100644
> > --- a/drivers/iio/adc/ltc2496.c
> > +++ b/drivers/iio/adc/ltc2496.c
> > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi)
> >
> >   static struct chip_info ltc2496_info = {
> >   	.resolution = 16,
> > +	.name = "ltc2496"
> >   };
> >
> >   static const struct of_device_id ltc2496_of_match[] = {
> > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
> > index b2752399402c..6dd9ab601904 100644
> > --- a/drivers/iio/adc/ltc2497-core.c
> > +++ b/drivers/iio/adc/ltc2497-core.c
> > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct
> iio_dev *indio_dev)
> >   	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
> >   	int ret;
> >
> > -	indio_dev->name = dev_name(dev);
> > +	indio_dev->name = ddata->chip_info->name;
> >   	indio_dev->info = &ltc2497core_info;
> >   	indio_dev->modes = INDIO_DIRECT_MODE;
> >   	indio_dev->channels = ltc2497core_channel;
> > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > index bb5e0d4301e2..a0aad71c8130 100644
> > --- a/drivers/iio/adc/ltc2497.c
> > +++ b/drivers/iio/adc/ltc2497.c
> > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client)
> >   static struct chip_info ltc2497_info[] = {
> >   	[TYPE_LTC2497] = {
> >   		.resolution = 16,
> > +		.name = "ltc2497"
> >   	},
> >   	[TYPE_LTC2499] = {
> >   		.resolution = 24,
> > +		.name = "ltc2499"
> >   	}
> >   };
> >
> > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> > index f4d939cfd48b..0e86e38248ee 100644
> > --- a/drivers/iio/adc/ltc2497.h
> > +++ b/drivers/iio/adc/ltc2497.h
> > @@ -12,6 +12,7 @@ enum chip_type {
> >
> >   struct chip_info {
> >   	u32 resolution;
> > +	char *name;
> >   };
> >
> >   struct ltc2497core_driverdata {
> 

RE: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Regus, Ciprian]

In reply to one of Andy's questions.

> On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org>
> wrote:
> > On Mon, 22 Aug 2022 15:51:05 +0300
> > Ciprian Regus <ciprian.regus@analog.com> wrote:
> 
> In reply to Jonathan's comments to answer his question and add more
> comments from me.
> 
> ...
> 
> > > Datasheet: https://www.analog.com/media/en/technical-
> documentation/data-sheets/2499fe.pdf
> > >
> > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> 
> Tag block mustn't have the blank line(s).
> 
> ...
> 
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/driver.h>
> > >  #include <linux/module.h>
> > > +#include <linux/property.h>
> > why?
> 
> device_get_match_data() requires it.
> 
> But why not sort them?
> 
> > >  #include <linux/mod_devicetable.h>
> 
> ...
> 
> > > -             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> >
> > Old code here is less than ideal, should be reading into 3 bytes then using
> > the be24 accesors. Please fix whilst here.  You will need multiple paths here
> > depending on size.
> >
> > > +             *val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
> > > +                     BIT(ddata->chip_info->resolution + 1);
> 
> Shouldn't this use some kind of sign_extend()?
The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension,
since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise.

Regards,
Ciprian
>
> Also with a temporary variable for chip info this line can be single.
> 
>    struct ... *ci = ddata->chip_info;
> 
>    ...BIT(ci->resolution + 1)
> 
> ...
> 
> > > +     u32 resolution;
> 
> Keep this in a way that the "longer lines go first".
> 
> ...
> 
> > > +     resolution = st->common_ddata.chip_info->resolution;
> > > +     st->sub_lsb = 31 - (resolution + 1);
> > > +     st->recv_size = resolution / BITS_PER_BYTE + 1;
> 
> BITS_TO_BYTES()
> 
> ...
> 
> > >  static const struct i2c_device_id ltc2497_id[] = {
> > > -     { "ltc2497", 0 },
> > > +     { "ltc2497", TYPE_LTC2497 },
> > > +     { "ltc2499", TYPE_LTC2499 },
> 
> Use pointers here like you have done for the OF table.
> 
> > >       { }
> > >  };
> 
> ...
> 
> > > +enum chip_type {
> > > +     TYPE_LTC2496,
> > > +     TYPE_LTC2497,
> > > +     TYPE_LTC2499
> 
> Keep trailing comma.
> 
> > > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device

[Jonathan Cameron]

On Mon, 29 Aug 2022 06:13:52 +0000
"Regus, Ciprian" <Ciprian.Regus@analog.com> wrote:

> Hi Lars,
> 
> Thanks for the review. I have one question before going further
> with this patch.
> 
> > On 8/22/22 14:51, Ciprian Regus wrote:  
> > > Set the iio device's name based on the chip used.  
> > 
> > While the change is correct it breaks the ABI. This needs a bit of a
> > better explanation what is being done, why, what are the potential
> > problems, why do we want to do it anyway.  
> 
> The reason for this change is to make it easier to identify the device
> (by having a generic name) when using an IIO client. The current name is
> based on the i2c_client's kobj name, which has the format
> "i2c_instance -i2c_address " (e.g. 1-0076). So, the renaming would allow
> for interacting with the device without knowing which bus it's connected
> to.
> 
> Also, as far as I can see, the IIO ABI states that the device's name is
> "Typically a part number".
> 
> For the reason given, would this change be considered an
> improvement, or is the ABI breaking too much of a drawback?
> (in which case I'll just drop this patch)

Hi Ciprian

There are a set of drivers that do it similarly to this (amongst other
things because I wasn't paying attention a few years back).

Generally we've considered them unfixable unfortunately because it's
common to use name to find the device - so userspace breakage is
very likely.

If we are introducing new device support thought here can't be a platform
already relying on the old naming.  So weird though it seems, can you fix
this just for the newly supported models and leave the old ones 'broken'?
(with a big note on why!)  Perhaps do that by leaving .name = NULL for the
older parts and doing a check on that to select behaviour.

Thanks,

Jonathan

> 
> Thanks,
> Ciprian
> >   
> > >
> > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> > > ---
> > >   drivers/iio/adc/ltc2496.c      | 1 +
> > >   drivers/iio/adc/ltc2497-core.c | 2 +-
> > >   drivers/iio/adc/ltc2497.c      | 2 ++
> > >   drivers/iio/adc/ltc2497.h      | 1 +
> > >   4 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> > > index 98338104c24a..86470f49e8ca 100644
> > > --- a/drivers/iio/adc/ltc2496.c
> > > +++ b/drivers/iio/adc/ltc2496.c
> > > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi)
> > >
> > >   static struct chip_info ltc2496_info = {
> > >   	.resolution = 16,
> > > +	.name = "ltc2496"
> > >   };
> > >
> > >   static const struct of_device_id ltc2496_of_match[] = {
> > > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c
> > > index b2752399402c..6dd9ab601904 100644
> > > --- a/drivers/iio/adc/ltc2497-core.c
> > > +++ b/drivers/iio/adc/ltc2497-core.c
> > > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct  
> > iio_dev *indio_dev)  
> > >   	struct ltc2497core_driverdata *ddata = iio_priv(indio_dev);
> > >   	int ret;
> > >
> > > -	indio_dev->name = dev_name(dev);
> > > +	indio_dev->name = ddata->chip_info->name;
> > >   	indio_dev->info = &ltc2497core_info;
> > >   	indio_dev->modes = INDIO_DIRECT_MODE;
> > >   	indio_dev->channels = ltc2497core_channel;
> > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> > > index bb5e0d4301e2..a0aad71c8130 100644
> > > --- a/drivers/iio/adc/ltc2497.c
> > > +++ b/drivers/iio/adc/ltc2497.c
> > > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client)
> > >   static struct chip_info ltc2497_info[] = {
> > >   	[TYPE_LTC2497] = {
> > >   		.resolution = 16,
> > > +		.name = "ltc2497"
> > >   	},
> > >   	[TYPE_LTC2499] = {
> > >   		.resolution = 24,
> > > +		.name = "ltc2499"
> > >   	}
> > >   };
> > >
> > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> > > index f4d939cfd48b..0e86e38248ee 100644
> > > --- a/drivers/iio/adc/ltc2497.h
> > > +++ b/drivers/iio/adc/ltc2497.h
> > > @@ -12,6 +12,7 @@ enum chip_type {
> > >
> > >   struct chip_info {
> > >   	u32 resolution;
> > > +	char *name;
> > >   };
> > >
> > >   struct ltc2497core_driverdata {  
> >   
> 

Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Mon, 29 Aug 2022 06:30:52 +0000
"Regus, Ciprian" <Ciprian.Regus@analog.com> wrote:

> In reply to one of Andy's questions.
> 
> > On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org>
> > wrote:  
> > > On Mon, 22 Aug 2022 15:51:05 +0300
> > > Ciprian Regus <ciprian.regus@analog.com> wrote:  
> > 
> > In reply to Jonathan's comments to answer his question and add more
> > comments from me.
> > 
> > ...
> >   
> > > > Datasheet: https://www.analog.com/media/en/technical-  
> > documentation/data-sheets/2499fe.pdf  
> > > >
> > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>  
> > 
> > Tag block mustn't have the blank line(s).
> > 
> > ...
> >   
> > > >  #include <linux/iio/iio.h>
> > > >  #include <linux/iio/driver.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/property.h>  
> > > why?  
> > 
> > device_get_match_data() requires it.
> > 
> > But why not sort them?
> >   
> > > >  #include <linux/mod_devicetable.h>  
> > 
> > ...
> >   
> > > > -             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);  
> > >
> > > Old code here is less than ideal, should be reading into 3 bytes then using
> > > the be24 accesors. Please fix whilst here.  You will need multiple paths here
> > > depending on size.
> > >  
> > > > +             *val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
> > > > +                     BIT(ddata->chip_info->resolution + 1);  
> > 
> > Shouldn't this use some kind of sign_extend()?  
> The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension,
> since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise.

This wins the award for one of the strangest formats I've yet seen.
The format is normal 2s complement 24 bit, but with a bonus upper bit to allow representation of
ever so slightly more than 24 bits.  Specifically on the postive side the out of range value.
0x7fffff + 1 = 0x800000 (which would normally be considered wrapped around to most negative value.

and on the negative side as well
0x800000 - 1 = 0x7fffff (normally most positive value).
It does this by throwing in an additional bit for sign which only tells us something useful
if in these two edge conditions (which are really an out of range error).
That bit corresponds int his case to
0x1000000 and is set for postive values only.

Thus our VS+ value becomes 0x01800000
and VS- value becomes      0x007fffff 
(extended to 32 bits for reasons that will become clear)
Applying 2's complement subtraction of the magic value above.

0x01800000 - 0x01000000 = 0x00800000   (2^23)
0x007fffff - 0x01000000 = 0xff7fffff   (-(2^23 + 1))

So it is indeed sign extended by the above.

Perhaps a few comments to set people off along the right path to what is
going on here would be useful?

Jonathan


> 
> Regards,
> Ciprian
> >
> > Also with a temporary variable for chip info this line can be single.
> > 
> >    struct ... *ci = ddata->chip_info;
> > 
> >    ...BIT(ci->resolution + 1)
> > 
> > ...
> >   
> > > > +     u32 resolution;  
> > 
> > Keep this in a way that the "longer lines go first".
> > 
> > ...
> >   
> > > > +     resolution = st->common_ddata.chip_info->resolution;
> > > > +     st->sub_lsb = 31 - (resolution + 1);
> > > > +     st->recv_size = resolution / BITS_PER_BYTE + 1;  
> > 
> > BITS_TO_BYTES()
> > 
> > ...
> >   
> > > >  static const struct i2c_device_id ltc2497_id[] = {
> > > > -     { "ltc2497", 0 },
> > > > +     { "ltc2497", TYPE_LTC2497 },
> > > > +     { "ltc2499", TYPE_LTC2499 },  
> > 
> > Use pointers here like you have done for the OF table.
> >   
> > > >       { }
> > > >  };  
> > 
> > ...
> >   
> > > > +enum chip_type {
> > > > +     TYPE_LTC2496,
> > > > +     TYPE_LTC2497,
> > > > +     TYPE_LTC2499  
> > 
> > Keep trailing comma.
> >   
> > > > +};  
> > 
> > --
> > With Best Regards,
> > Andy Shevchenko