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

[PATCH v2 1/5] 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>
---
changes in v2:
 - added dashes in front of enum elements.
 .../devicetree/bindings/iio/adc/lltc,ltc2497.yaml         | 8 ++++++--
 MAINTAINERS                                               | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
index c1772b568cd1..875f394576c2 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:
-      lltc,ltc2497
+    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 v2 2/5] Add the LTC2496 MAINTAINERS entry

[Ciprian Regus]

Update the MAINTAINERS file to include the path for the LTC2496
devicetree bindings documentation.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
changes in v2:
 - new patch
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3c847619ceb1..1beb41a8b672 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,ltc2496.yaml
 F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml
 F:	drivers/iio/*/ad*
-- 
2.30.2

[PATCH v2 3/5] Remove duplicate matching entry

[Ciprian Regus]

Remove the specific entry for ad5758, since
Documentation/devicetree/bindings/iio/*/adi,* already matches
the path.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
changes in v2:
 - new patch
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1beb41a8b672..ca7fc57173c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1329,7 +1329,6 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
 F:	Documentation/devicetree/bindings/iio/*/adi,*
 F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2496.yaml
 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*
 F:	drivers/iio/amplifiers/hmc425a.c
-- 
2.30.2

[PATCH v2 4/5] 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>
---
changes in v2:
 - removed the bitfield.h and bitops.h includes, since they were not needed.
 - removed a blank line.
 - replaced the data buffer for the ltc2497_driverdata with a union.
 - depending on frame size, i2c transfers use either 3 or 4 byte buffers, instead
   of always using a __be32.
 - added a comment which explains the output data format and how does sign extension.
   happen.
 - added the const modifier for the chip_info structs.
 - renamed the chip_info struct to ltc2497_chip_info.
 - renamed the chip_type enum to ltc2497_chip_type
 - added probe fallback to using i2c_device_id in case OF fails.
 - used BITS_TO_BYTES() instead of dividing by 8.
 - used a tab instead of a space in a struct field declaration, which in v1 appeared as
   if the line was deleted and added back.
 - added back a trailing comma.
 - rearranged variable declaration lines so that longer ones would be first.
 - used pointers to a chip info struct in the i2c_device_id table, instead of enums. 
 drivers/iio/adc/ltc2496.c      |  8 ++++-
 drivers/iio/adc/ltc2497-core.c |  2 +-
 drivers/iio/adc/ltc2497.c      | 56 +++++++++++++++++++++++++++++++---
 drivers/iio/adc/ltc2497.h      | 11 +++++++
 4 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
index dfb3bb5997e5..bf89d5ae19af 100644
--- a/drivers/iio/adc/ltc2496.c
+++ b/drivers/iio/adc/ltc2496.c
@@ -15,6 +15,7 @@
 #include <linux/iio/driver.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/property.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 const struct ltc2497_chip_info ltc2496_info = {
+	.resolution = 16,
+};
+
 static const struct of_device_id ltc2496_of_match[] = {
-	{ .compatible = "lltc,ltc2496", },
+	{ .compatible = "lltc,ltc2496", .data = &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..2f660015f34b 100644
--- a/drivers/iio/adc/ltc2497.c
+++ b/drivers/iio/adc/ltc2497.c
@@ -12,6 +12,7 @@
 #include <linux/iio/driver.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/property.h>
 
 #include "ltc2497.h"
 
@@ -19,11 +20,16 @@ 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.
 	 */
-	__be32 buf __aligned(IIO_DMA_MINALIGN);
+	union {
+		__be32 d32;
+		u8 d8[3];
+	} data __aligned(IIO_DMA_MINALIGN);
 };
 
 static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
@@ -34,13 +40,29 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 	int ret;
 
 	if (val) {
-		ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
+		if (st->recv_size == 3)
+			ret = i2c_master_recv(st->client, (char *)&st->data.d8, st->recv_size);
+		else
+			ret = i2c_master_recv(st->client, (char *)&st->data.d32, 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);
+		/*
+		 * The data format is 16/24 bit 2s complement, but with an upper sign bit on the
+		 * resolution + 1 position, which is set for positive values only. Given this
+		 * bit's value, subtracting BIT(resolution + 1) from the ADC's result is
+		 * equivalent to a sign extension.
+		 */
+		if (st->recv_size == 3) {
+			*val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
+				- BIT(ddata->chip_info->resolution + 1);
+		} else {
+			*val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
+				- BIT(ddata->chip_info->resolution + 1);
+		}
 	}
 
 	ret = i2c_smbus_write_byte(st->client,
@@ -54,9 +76,11 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
 static int ltc2497_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	const struct ltc2497_chip_info *chip_info;
 	struct iio_dev *indio_dev;
 	struct ltc2497_driverdata *st;
 	struct device *dev = &client->dev;
+	u32 resolution;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
 				     I2C_FUNC_SMBUS_WRITE_BYTE))
@@ -71,6 +95,15 @@ static int ltc2497_probe(struct i2c_client *client,
 	st->client = client;
 	st->common_ddata.result_and_measure = ltc2497_result_and_measure;
 
+	chip_info = device_get_match_data(dev);
+	if (!chip_info)
+		chip_info = (const struct ltc2497_chip_info *)id->driver_data;
+	st->common_ddata.chip_info = chip_info;
+
+	resolution = chip_info->resolution;
+	st->sub_lsb = 31 - (resolution + 1);
+	st->recv_size = BITS_TO_BYTES(resolution) + 1;
+
 	return ltc2497core_probe(dev, indio_dev);
 }
 
@@ -83,14 +116,27 @@ static int ltc2497_remove(struct i2c_client *client)
 	return 0;
 }
 
+static const struct ltc2497_chip_info ltc2497_info[] = {
+	[TYPE_LTC2497] = {
+		.resolution = 16,
+		.name = NULL,
+	},
+	[TYPE_LTC2499] = {
+		.resolution = 24,
+		.name = "ltc2499",
+	},
+};
+
 static const struct i2c_device_id ltc2497_id[] = {
-	{ "ltc2497", 0 },
+	{ "ltc2497", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },
+	{ "ltc2499", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ltc2497_id);
 
 static const struct of_device_id ltc2497_of_match[] = {
-	{ .compatible = "lltc,ltc2497", },
+	{ .compatible = "lltc,ltc2497", .data = &ltc2497_info[TYPE_LTC2497] },
+	{ .compatible = "lltc,ltc2499", .data = &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..95f6a5f4d4a6 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 ltc2497_chip_type {
+	TYPE_LTC2496,
+	TYPE_LTC2497,
+	TYPE_LTC2499,
+};
+
+struct ltc2497_chip_info {
+	u32 resolution;
+};
+
 struct ltc2497core_driverdata {
 	struct regulator *ref;
 	ktime_t	time_prev;
+	const struct ltc2497_chip_info	*chip_info;
 	u8 addr_prev;
 	int (*result_and_measure)(struct ltc2497core_driverdata *ddata,
 				  u8 address, int *val);
-- 
2.30.2

[PATCH v2 5/5] drivers: iio: adc: Rename the LTC2499 iio device

[Ciprian Regus]

Set the iio device's name based on the chip used for the
LTC2499 only. The most common way for IIO clients to interact
with a device is to address it based on it's name. By using
the dev_name() function, the name will be set based on a
i2c_client's kobj name, which has the format i2c_instance-i2c_address
(1-0076 for example). This is not ideal, since it makes a
requirement for userspace to have knowledge about the hardware
connections of the device.

The name field is set to NULL for the LTC2497 and LTC2496, so
that the old name can kept as it is, since changing it will
result in an ABI breakage.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
changes in v2:
 - updated the patch title (LTC249x -> LTC2499), since the name change only
   affects the LTC2499.
 - updated the commit description to better explain what is being done.
 - only changed the iio_dev's name for the LTC2499.
 - added a comment to explain difference in naming.
 - added the const qualifier to the name field.
 drivers/iio/adc/ltc2496.c      |  1 +
 drivers/iio/adc/ltc2497-core.c | 10 +++++++++-
 drivers/iio/adc/ltc2497.h      |  1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
index bf89d5ae19af..2593fa4322eb 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 const struct ltc2497_chip_info ltc2496_info = {
 	.resolution = 16,
+	.name = NULL,
 };
 
 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..f52d37af4d1f 100644
--- a/drivers/iio/adc/ltc2497-core.c
+++ b/drivers/iio/adc/ltc2497-core.c
@@ -169,7 +169,15 @@ 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);
+	/*
+	 * Keep using dev_name() for the iio_dev's name on some of the parts,
+	 * since updating it would result in a ABI breakage.
+	 */
+	if (ddata->chip_info->name)
+		indio_dev->name = ddata->chip_info->name;
+	else
+		indio_dev->name = dev_name(dev);
+
 	indio_dev->info = &ltc2497core_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ltc2497core_channel;
diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
index 95f6a5f4d4a6..fd3dfd491060 100644
--- a/drivers/iio/adc/ltc2497.h
+++ b/drivers/iio/adc/ltc2497.h
@@ -12,6 +12,7 @@ enum ltc2497_chip_type {
 
 struct ltc2497_chip_info {
 	u32 resolution;
+	const char *name;
 };
 
 struct ltc2497core_driverdata {
-- 
2.30.2

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Krzysztof Kozlowski]

On 01/09/2022 15:16, Ciprian Regus 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

Missing blank line. Use standard Git tools for handling your patches or
be sure you produce the same result when using some custom process.

> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>

(...)

> +};
> +
>  static const struct i2c_device_id ltc2497_id[] = {
> -	{ "ltc2497", 0 },
> +	{ "ltc2497", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },
> +	{ "ltc2499", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },

So they are the same, aren't they?

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltc2497_id);
>  
>  static const struct of_device_id ltc2497_of_match[] = {
> -	{ .compatible = "lltc,ltc2497", },
> +	{ .compatible = "lltc,ltc2497", .data = &ltc2497_info[TYPE_LTC2497] },
> +	{ .compatible = "lltc,ltc2499", .data = &ltc2497_info[TYPE_LTC2499] },

I think this should be split into two patches for easier review - one
working on driver data for existing variant and second for adding new
variant 2499.

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index d0b42dd6b8ad..95f6a5f4d4a6 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 ltc2497_chip_type {
> +	TYPE_LTC2496,

Why having here 2496 and not using it?

> +	TYPE_LTC2497,
> +	TYPE_LTC2499,
> +};
> +
Krzysztof

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

[Krzysztof Kozlowski]

On 01/09/2022 15:16, Ciprian Regus wrote:
> Update the bindings documentation for ltc2497 to include the ltc2499.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[kernel test robot]

Hi Ciprian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: hexagon-randconfig-r003-20220901 (https://download.01.org/0day-ci/archive/20220902/202209020359.vCYzjn4X-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/08ff9ae09bfde86fc512e13a4ea2af894e4aa442
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
        git checkout 08ff9ae09bfde86fc512e13a4ea2af894e4aa442
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/iio/adc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iio/adc/ltc2497.c:60:12: error: call to undeclared function 'get_unaligned_be24'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           *val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
                                   ^
   drivers/iio/adc/ltc2497.c:122:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
                   .name = NULL,
                    ^
   drivers/iio/adc/ltc2497.c:126:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
                   .name = "ltc2499",
                    ^
   3 errors generated.


vim +/get_unaligned_be24 +60 drivers/iio/adc/ltc2497.c

    34	
    35	static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
    36					      u8 address, int *val)
    37	{
    38		struct ltc2497_driverdata *st =
    39			container_of(ddata, struct ltc2497_driverdata, common_ddata);
    40		int ret;
    41	
    42		if (val) {
    43			if (st->recv_size == 3)
    44				ret = i2c_master_recv(st->client, (char *)&st->data.d8, st->recv_size);
    45			else
    46				ret = i2c_master_recv(st->client, (char *)&st->data.d32, st->recv_size);
    47	
    48			if (ret < 0) {
    49				dev_err(&st->client->dev, "i2c_master_recv failed\n");
    50				return ret;
    51			}
    52	
    53			/*
    54			 * The data format is 16/24 bit 2s complement, but with an upper sign bit on the
    55			 * resolution + 1 position, which is set for positive values only. Given this
    56			 * bit's value, subtracting BIT(resolution + 1) from the ADC's result is
    57			 * equivalent to a sign extension.
    58			 */
    59			if (st->recv_size == 3) {
  > 60				*val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
    61					- BIT(ddata->chip_info->resolution + 1);
    62			} else {
    63				*val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
    64					- BIT(ddata->chip_info->resolution + 1);
    65			}
    66		}
    67	
    68		ret = i2c_smbus_write_byte(st->client,
    69					   LTC2497_ENABLE | address);
    70		if (ret)
    71			dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
    72				ERR_PTR(ret));
    73		return ret;
    74	}
    75	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[kernel test robot]

Hi Ciprian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-a015 (https://download.01.org/0day-ci/archive/20220902/202209020413.akDnDcLc-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/08ff9ae09bfde86fc512e13a4ea2af894e4aa442
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
        git checkout 08ff9ae09bfde86fc512e13a4ea2af894e4aa442
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/adc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iio/adc/ltc2497.c:60:12: error: implicit declaration of function 'get_unaligned_be24' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                           *val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
                                   ^
   drivers/iio/adc/ltc2497.c:122:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
                   .name = NULL,
                    ^
   drivers/iio/adc/ltc2497.c:126:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
                   .name = "ltc2499",
                    ^
   3 errors generated.


vim +/get_unaligned_be24 +60 drivers/iio/adc/ltc2497.c

    34	
    35	static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
    36					      u8 address, int *val)
    37	{
    38		struct ltc2497_driverdata *st =
    39			container_of(ddata, struct ltc2497_driverdata, common_ddata);
    40		int ret;
    41	
    42		if (val) {
    43			if (st->recv_size == 3)
    44				ret = i2c_master_recv(st->client, (char *)&st->data.d8, st->recv_size);
    45			else
    46				ret = i2c_master_recv(st->client, (char *)&st->data.d32, st->recv_size);
    47	
    48			if (ret < 0) {
    49				dev_err(&st->client->dev, "i2c_master_recv failed\n");
    50				return ret;
    51			}
    52	
    53			/*
    54			 * The data format is 16/24 bit 2s complement, but with an upper sign bit on the
    55			 * resolution + 1 position, which is set for positive values only. Given this
    56			 * bit's value, subtracting BIT(resolution + 1) from the ADC's result is
    57			 * equivalent to a sign extension.
    58			 */
    59			if (st->recv_size == 3) {
  > 60				*val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
    61					- BIT(ddata->chip_info->resolution + 1);
    62			} else {
    63				*val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
    64					- BIT(ddata->chip_info->resolution + 1);
    65			}
    66		}
    67	
    68		ret = i2c_smbus_write_byte(st->client,
    69					   LTC2497_ENABLE | address);
    70		if (ret)
    71			dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
    72				ERR_PTR(ret));
    73		return ret;
    74	}
    75	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Thu, 1 Sep 2022 16:23:09 +0300
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 01/09/2022 15:16, Ciprian Regus 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  
> 
> Missing blank line. Use standard Git tools for handling your patches or
> be sure you produce the same result when using some custom process.

My understanding is Datasheet is a standard tag as part of the main tag block.
There should not be a blank line between that and the Sign off.

+CC Andy who can probably point to a reference for that...

> 
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>  
> 
> (...)
> 
> > +};
> > +
> >  static const struct i2c_device_id ltc2497_id[] = {
> > -	{ "ltc2497", 0 },
> > +	{ "ltc2497", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },
> > +	{ "ltc2499", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },  
> 
> So they are the same, aren't they?
> 
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, ltc2497_id);
> >  
> >  static const struct of_device_id ltc2497_of_match[] = {
> > -	{ .compatible = "lltc,ltc2497", },
> > +	{ .compatible = "lltc,ltc2497", .data = &ltc2497_info[TYPE_LTC2497] },
> > +	{ .compatible = "lltc,ltc2499", .data = &ltc2497_info[TYPE_LTC2499] },  
> 
> I think this should be split into two patches for easier review - one
> working on driver data for existing variant and second for adding new
> variant 2499.
> 
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> > index d0b42dd6b8ad..95f6a5f4d4a6 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 ltc2497_chip_type {
> > +	TYPE_LTC2496,  
> 
> Why having here 2496 and not using it?
> 
> > +	TYPE_LTC2497,
> > +	TYPE_LTC2499,
> > +};
> > +  
> Krzysztof

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Thu, 1 Sep 2022 16:23:09 +0300
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > On 01/09/2022 15:16, Ciprian Regus wrote:

...

> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf
> >
> > Missing blank line. Use standard Git tools for handling your patches or
> > be sure you produce the same result when using some custom process.
>
> My understanding is Datasheet is a standard tag as part of the main tag block.
> There should not be a blank line between that and the Sign off.
>
> +CC Andy who can probably point to a reference for that...

Yes, the idea to have a Datasheet as a formal tag. Which is, by the
way, somehow established practice (since ca.2020).

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Thu, 1 Sep 2022 15:16:56 +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>
Please use --cover-letter to add a cover letter to series with more
than 1 or 2 patches. It makes automation + commenting on the whole
series much easier + provides a place to briefly say what the overall
theme joining the patches in the series together is!

The MAINTAINERS and yaml changes seem unrelated so should probably be in
separate patches.

Thanks,

Jonathan

> ---
> changes in v2:
>  - added dashes in front of enum elements.
>  .../devicetree/bindings/iio/adc/lltc,ltc2497.yaml         | 8 ++++++--
>  MAINTAINERS                                               | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
> index c1772b568cd1..875f394576c2 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:
> -      lltc,ltc2497
> +    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*

Re: [PATCH v2 2/5] Add the LTC2496 MAINTAINERS entry

[Jonathan Cameron]

On Thu, 1 Sep 2022 15:16:57 +0300
Ciprian Regus <ciprian.regus@analog.com> wrote:

> Update the MAINTAINERS file to include the path for the LTC2496
> devicetree bindings documentation.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
Hi Ciprian,

Thanks for tidying this up.

Pulling the addition of the line for the ltc2497 into this patch as well
would make more sense than the current split between patches 1 and 2.

Obviously with updates to the patch description to mention that it is
covering both.

Thanks,

Jonathan

> ---
> changes in v2:
>  - new patch
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3c847619ceb1..1beb41a8b672 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,ltc2496.yaml
>  F:	Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml
>  F:	drivers/iio/*/ad*

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Fri, 2 Sep 2022 14:37:03 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Thu, 1 Sep 2022 16:23:09 +0300
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:  
> > > On 01/09/2022 15:16, Ciprian Regus wrote:  
> 
> ...
> 
> > > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf  
> > >
> > > Missing blank line. Use standard Git tools for handling your patches or
> > > be sure you produce the same result when using some custom process.  
> >
> > My understanding is Datasheet is a standard tag as part of the main tag block.
> > There should not be a blank line between that and the Sign off.
> >
> > +CC Andy who can probably point to a reference for that...  
> 
> Yes, the idea to have a Datasheet as a formal tag. Which is, by the
> way, somehow established practice (since ca.2020).

We should probably add it to the docs so we have somewhere to point at
beyond fairly common practice.

Hohum.  Anyone want to take that on with associated possible bikeshedding?

Jonathan

> 

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Thu, 1 Sep 2022 15:16:59 +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>

A few small comments inline, but looks pretty good to me.

Jonathan

> ---
> changes in v2:
>  - removed the bitfield.h and bitops.h includes, since they were not needed.
>  - removed a blank line.
>  - replaced the data buffer for the ltc2497_driverdata with a union.
>  - depending on frame size, i2c transfers use either 3 or 4 byte buffers, instead
>    of always using a __be32.
>  - added a comment which explains the output data format and how does sign extension.
>    happen.
>  - added the const modifier for the chip_info structs.
>  - renamed the chip_info struct to ltc2497_chip_info.
>  - renamed the chip_type enum to ltc2497_chip_type
>  - added probe fallback to using i2c_device_id in case OF fails.
>  - used BITS_TO_BYTES() instead of dividing by 8.
>  - used a tab instead of a space in a struct field declaration, which in v1 appeared as
>    if the line was deleted and added back.
>  - added back a trailing comma.
>  - rearranged variable declaration lines so that longer ones would be first.
>  - used pointers to a chip info struct in the i2c_device_id table, instead of enums. 
>  drivers/iio/adc/ltc2496.c      |  8 ++++-
>  drivers/iio/adc/ltc2497-core.c |  2 +-
>  drivers/iio/adc/ltc2497.c      | 56 +++++++++++++++++++++++++++++++---
>  drivers/iio/adc/ltc2497.h      | 11 +++++++
>  4 files changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> index dfb3bb5997e5..bf89d5ae19af 100644
> --- a/drivers/iio/adc/ltc2496.c
> +++ b/drivers/iio/adc/ltc2496.c
> @@ -15,6 +15,7 @@
>  #include <linux/iio/driver.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/property.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);

Hmm. This driver doesn't provide the other i2c registration path
(i2c_device_id table) so this is fine.  Adding that table can be a problem
for whoever needs it sometime in the future (I'm fine with patches to add
it though if anyone wants to write one!)
>  
>  	return ltc2497core_probe(dev, indio_dev);
>  }
> @@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi)
>  	ltc2497core_remove(indio_dev);
>  }
>  
> +static const struct ltc2497_chip_info ltc2496_info = {
> +	.resolution = 16,
> +};
> +
>  static const struct of_device_id ltc2496_of_match[] = {
> -	{ .compatible = "lltc,ltc2496", },
> +	{ .compatible = "lltc,ltc2496", .data = &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..2f660015f34b 100644
> --- a/drivers/iio/adc/ltc2497.c
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -12,6 +12,7 @@
>  #include <linux/iio/driver.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/property.h>
>  
>  #include "ltc2497.h"
>  
> @@ -19,11 +20,16 @@ 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.
>  	 */
> -	__be32 buf __aligned(IIO_DMA_MINALIGN);
> +	union {
> +		__be32 d32;
> +		u8 d8[3];
> +	} data __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
> @@ -34,13 +40,29 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>  	int ret;
>  
>  	if (val) {
> -		ret = i2c_master_recv(st->client, (char *)&st->buf, 3);
> +		if (st->recv_size == 3)
> +			ret = i2c_master_recv(st->client, (char *)&st->data.d8, st->recv_size);
> +		else
> +			ret = i2c_master_recv(st->client, (char *)&st->data.d32, 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);
> +		/*
> +		 * The data format is 16/24 bit 2s complement, but with an upper sign bit on the
> +		 * resolution + 1 position, which is set for positive values only. Given this
> +		 * bit's value, subtracting BIT(resolution + 1) from the ADC's result is
> +		 * equivalent to a sign extension.

Description looks good to me.  Thanks for adding.

> +		 */
> +		if (st->recv_size == 3) {
> +			*val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
> +				- BIT(ddata->chip_info->resolution + 1);
> +		} else {
> +			*val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
> +				- BIT(ddata->chip_info->resolution + 1);
> +		}
>  	}
>  

>  
> +static const struct ltc2497_chip_info ltc2497_info[] = {
> +	[TYPE_LTC2497] = {
> +		.resolution = 16,
> +		.name = NULL,
> +	},
> +	[TYPE_LTC2499] = {
> +		.resolution = 24,
> +		.name = "ltc2499",
> +	},
> +};
> +
>  static const struct i2c_device_id ltc2497_id[] = {
> -	{ "ltc2497", 0 },
> +	{ "ltc2497", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },
> +	{ "ltc2499", (kernel_ulong_t)&ltc2497_info[TYPE_LTC2497] },

TYPE_LTC2499

Guess you haven't tested this particular path but should be easy to
force it by not setting the of_device_id table pointer (or you could
use a board file but that's more trouble than ti's worth).

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltc2497_id);
>  
>  static const struct of_device_id ltc2497_of_match[] = {
> -	{ .compatible = "lltc,ltc2497", },
> +	{ .compatible = "lltc,ltc2497", .data = &ltc2497_info[TYPE_LTC2497] },
> +	{ .compatible = "lltc,ltc2499", .data = &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..95f6a5f4d4a6 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 ltc2497_chip_type {
> +	TYPE_LTC2496,

Hmm. this is a little interesting given there is no
such entry in the info table so that table will get pushed out
to have an empty first entry.  Maybe push this chip_type definition
down into the relevant c file and drop the LTC2496 entry (which will
then seem fine as that .c file doesn't cover that part.

> +	TYPE_LTC2497,
> +	TYPE_LTC2499,
> +};
> +

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Fri, 2 Sep 2022 04:00:05 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Ciprian,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on jic23-iio/togreg]
> [also build test ERROR on robh/for-next linus/master v6.0-rc3 next-20220901]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: hexagon-randconfig-r003-20220901 (https://download.01.org/0day-ci/archive/20220902/202209020359.vCYzjn4X-lkp@intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/08ff9ae09bfde86fc512e13a4ea2af894e4aa442
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Ciprian-Regus/dt-bindings-iio-adc-Add-docs-for-LTC2499/20220901-202115
>         git checkout 08ff9ae09bfde86fc512e13a4ea2af894e4aa442
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/iio/adc/
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/iio/adc/ltc2497.c:60:12: error: call to undeclared function 'get_unaligned_be24'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]  
>                            *val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
>                                    ^
>    drivers/iio/adc/ltc2497.c:122:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
>                    .name = NULL,
>                     ^
>    drivers/iio/adc/ltc2497.c:126:4: error: field designator 'name' does not refer to any field in type 'const struct ltc2497_chip_info'
>                    .name = "ltc2499",
>                     ^
>    3 errors generated.
> 
Ah. The power of automation proves itself again.  I missed that you'd
not added 

#include <asm/unaligned.h>
and that the .name field is introduced in a later patch.

Jonathan

> 
> vim +/get_unaligned_be24 +60 drivers/iio/adc/ltc2497.c
> 
>     34	
>     35	static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
>     36					      u8 address, int *val)
>     37	{
>     38		struct ltc2497_driverdata *st =
>     39			container_of(ddata, struct ltc2497_driverdata, common_ddata);
>     40		int ret;
>     41	
>     42		if (val) {
>     43			if (st->recv_size == 3)
>     44				ret = i2c_master_recv(st->client, (char *)&st->data.d8, st->recv_size);
>     45			else
>     46				ret = i2c_master_recv(st->client, (char *)&st->data.d32, st->recv_size);
>     47	
>     48			if (ret < 0) {
>     49				dev_err(&st->client->dev, "i2c_master_recv failed\n");
>     50				return ret;
>     51			}
>     52	
>     53			/*
>     54			 * The data format is 16/24 bit 2s complement, but with an upper sign bit on the
>     55			 * resolution + 1 position, which is set for positive values only. Given this
>     56			 * bit's value, subtracting BIT(resolution + 1) from the ADC's result is
>     57			 * equivalent to a sign extension.
>     58			 */
>     59			if (st->recv_size == 3) {
>   > 60				*val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)  
>     61					- BIT(ddata->chip_info->resolution + 1);
>     62			} else {
>     63				*val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
>     64					- BIT(ddata->chip_info->resolution + 1);
>     65			}
>     66		}
>     67	
>     68		ret = i2c_smbus_write_byte(st->client,
>     69					   LTC2497_ENABLE | address);
>     70		if (ret)
>     71			dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
>     72				ERR_PTR(ret));
>     73		return ret;
>     74	}
>     75	
> 

Re: [PATCH v2 5/5] drivers: iio: adc: Rename the LTC2499 iio device

[Jonathan Cameron]

On Thu, 1 Sep 2022 15:17:00 +0300
Ciprian Regus <ciprian.regus@analog.com> wrote:

> Set the iio device's name based on the chip used for the
> LTC2499 only. The most common way for IIO clients to interact
> with a device is to address it based on it's name. By using
> the dev_name() function, the name will be set based on a
> i2c_client's kobj name, which has the format i2c_instance-i2c_address
> (1-0076 for example). This is not ideal, since it makes a
> requirement for userspace to have knowledge about the hardware
> connections of the device.
> 
> The name field is set to NULL for the LTC2497 and LTC2496, so
> that the old name can kept as it is, since changing it will
> result in an ABI breakage.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
Other than the issue with the split between patches 4 and 5 that
the robot found this looks good to me.

Jonathan

> ---
> changes in v2:
>  - updated the patch title (LTC249x -> LTC2499), since the name change only
>    affects the LTC2499.
>  - updated the commit description to better explain what is being done.
>  - only changed the iio_dev's name for the LTC2499.
>  - added a comment to explain difference in naming.
>  - added the const qualifier to the name field.
>  drivers/iio/adc/ltc2496.c      |  1 +
>  drivers/iio/adc/ltc2497-core.c | 10 +++++++++-
>  drivers/iio/adc/ltc2497.h      |  1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> index bf89d5ae19af..2593fa4322eb 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 const struct ltc2497_chip_info ltc2496_info = {
>  	.resolution = 16,
> +	.name = NULL,
>  };
>  
>  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..f52d37af4d1f 100644
> --- a/drivers/iio/adc/ltc2497-core.c
> +++ b/drivers/iio/adc/ltc2497-core.c
> @@ -169,7 +169,15 @@ 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);
> +	/*
> +	 * Keep using dev_name() for the iio_dev's name on some of the parts,
> +	 * since updating it would result in a ABI breakage.
> +	 */
> +	if (ddata->chip_info->name)
> +		indio_dev->name = ddata->chip_info->name;
> +	else
> +		indio_dev->name = dev_name(dev);
> +
>  	indio_dev->info = &ltc2497core_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = ltc2497core_channel;
> diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h
> index 95f6a5f4d4a6..fd3dfd491060 100644
> --- a/drivers/iio/adc/ltc2497.h
> +++ b/drivers/iio/adc/ltc2497.h
> @@ -12,6 +12,7 @@ enum ltc2497_chip_type {
>  
>  struct ltc2497_chip_info {
>  	u32 resolution;
> +	const char *name;
>  };
>  
>  struct ltc2497core_driverdata {

Re: [PATCH v2 4/5] drivers: iio: adc: LTC2499 support

[Krzysztof Kozlowski]

On 04/09/2022 16:55, Jonathan Cameron wrote:
> On Fri, 2 Sep 2022 14:37:03 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
>> <Jonathan.Cameron@huawei.com> wrote:
>>> On Thu, 1 Sep 2022 16:23:09 +0300
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:  
>>>> On 01/09/2022 15:16, Ciprian Regus wrote:  
>>
>> ...
>>
>>>>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf  
>>>>
>>>> Missing blank line. Use standard Git tools for handling your patches or
>>>> be sure you produce the same result when using some custom process.  
>>>
>>> My understanding is Datasheet is a standard tag as part of the main tag block.
>>> There should not be a blank line between that and the Sign off.
>>>
>>> +CC Andy who can probably point to a reference for that...  
>>
>> Yes, the idea to have a Datasheet as a formal tag. Which is, by the
>> way, somehow established practice (since ca.2020).
> 
> We should probably add it to the docs so we have somewhere to point at
> beyond fairly common practice.
> 
> Hohum.  Anyone want to take that on with associated possible bikeshedding?

Sorry for the noise then - I grepped, nothing popped up. It's not
appearing in the checkpatch patterns, although indeed appears in the
history, so it is fine.

Thanks for clarification.


Best regards,
Krzysztof