2022-09-01 12:45:06

by Ciprian Regus

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

Update the bindings documentation for ltc2497 to include the ltc2499.

Signed-off-by: Ciprian Regus <[email protected]>
---
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

2022-09-01 12:45:50

by Ciprian Regus

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

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 <[email protected]>
---
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

2022-09-01 12:46:00

by Ciprian Regus

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

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

Signed-off-by: Ciprian Regus <[email protected]>
---
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

2022-09-01 13:42:52

by Krzysztof Kozlowski

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

On 01/09/2022 15:16, Ciprian Regus wrote:
> Update the bindings documentation for ltc2497 to include the ltc2499.
>
> Signed-off-by: Ciprian Regus <[email protected]>
> ---

Acked-by: Krzysztof Kozlowski <[email protected]>


Best regards,
Krzysztof

2022-09-01 13:54:39

by Krzysztof Kozlowski

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

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 <[email protected]>

(...)

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

2022-09-01 20:42:26

by kernel test robot

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

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/[email protected]/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 <[email protected]>

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

2022-09-01 20:52:00

by kernel test robot

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

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/[email protected]/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 <[email protected]>

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

2022-09-02 11:39:41

by Andy Shevchenko

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

On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
<[email protected]> wrote:
> On Thu, 1 Sep 2022 16:23:09 +0300
> Krzysztof Kozlowski <[email protected]> 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

2022-09-04 15:31:57

by Jonathan Cameron

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

On Thu, 1 Sep 2022 15:16:56 +0300
Ciprian Regus <[email protected]> wrote:

> Update the bindings documentation for ltc2497 to include the ltc2499.
>
> Signed-off-by: Ciprian Regus <[email protected]>
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*

2022-09-04 15:43:47

by Jonathan Cameron

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

On Fri, 2 Sep 2022 14:37:03 +0300
Andy Shevchenko <[email protected]> wrote:

> On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
> <[email protected]> wrote:
> > On Thu, 1 Sep 2022 16:23:09 +0300
> > Krzysztof Kozlowski <[email protected]> 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

>

2022-09-04 15:44:35

by Jonathan Cameron

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

On Thu, 1 Sep 2022 15:16:57 +0300
Ciprian Regus <[email protected]> wrote:

> Update the MAINTAINERS file to include the path for the LTC2496
> devicetree bindings documentation.
>
> Signed-off-by: Ciprian Regus <[email protected]>
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*

2022-09-04 15:46:38

by Jonathan Cameron

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

On Thu, 1 Sep 2022 15:16:59 +0300
Ciprian Regus <[email protected]> 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 <[email protected]>

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,
> +};
> +

2022-09-04 15:49:39

by Jonathan Cameron

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

On Fri, 2 Sep 2022 04:00:05 +0800
kernel test robot <[email protected]> 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/[email protected]/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 <[email protected]>
>
> 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
>

2022-09-05 09:23:03

by Krzysztof Kozlowski

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

On 04/09/2022 16:55, Jonathan Cameron wrote:
> On Fri, 2 Sep 2022 14:37:03 +0300
> Andy Shevchenko <[email protected]> wrote:
>
>> On Fri, Sep 2, 2022 at 2:06 PM Jonathan Cameron
>> <[email protected]> wrote:
>>> On Thu, 1 Sep 2022 16:23:09 +0300
>>> Krzysztof Kozlowski <[email protected]> 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