| Message ID | 20220901121700.1325733-4-ciprian.regus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/5] dt-bindings: iio: adc: Add docs for LTC2499 | expand |
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)<c2497_info[TYPE_LTC2497] }, > + { "ltc2499", (kernel_ulong_t)<c2497_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 = <c2497_info[TYPE_LTC2497] }, > + { .compatible = "lltc,ltc2499", .data = <c2497_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
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
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
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)<c2497_info[TYPE_LTC2497] }, > > + { "ltc2499", (kernel_ulong_t)<c2497_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 = <c2497_info[TYPE_LTC2497] }, > > + { .compatible = "lltc,ltc2499", .data = <c2497_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
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).
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 >
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 = <c2496_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)<c2497_info[TYPE_LTC2497] }, > + { "ltc2499", (kernel_ulong_t)<c2497_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 = <c2497_info[TYPE_LTC2497] }, > + { .compatible = "lltc,ltc2499", .data = <c2497_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, > +}; > +
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 >
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
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 = <c2496_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)<c2497_info[TYPE_LTC2497] }, + { "ltc2499", (kernel_ulong_t)<c2497_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 = <c2497_info[TYPE_LTC2497] }, + { .compatible = "lltc,ltc2499", .data = <c2497_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);
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(-)