| Message ID | 20220822125106.1106798-3-ciprian.regus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for LTC2499 ADC | expand |
On Mon, 22 Aug 2022 15:51:05 +0300 Ciprian Regus <ciprian.regus@analog.com> wrote: > The LTC2499 is a 16-channel (eight differential), 24-bit, > ADC with Easy Drive technology and a 2-wire, I2C interface. > > Implement support for the LTC2499 ADC by extending the LTC2497 > driver. A new chip_info struct is added to differentiate between > chip types and resolutions when reading data from the device. > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> Hi Ciprian, Various comments inline. Thanks, Jonathan > --- > drivers/iio/adc/ltc2496.c | 8 +++++++- > drivers/iio/adc/ltc2497-core.c | 2 +- > drivers/iio/adc/ltc2497.c | 34 +++++++++++++++++++++++++++++----- > drivers/iio/adc/ltc2497.h | 13 ++++++++++++- > 4 files changed, 49 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c > index dfb3bb5997e5..98338104c24a 100644 > --- a/drivers/iio/adc/ltc2496.c > +++ b/drivers/iio/adc/ltc2496.c > @@ -14,6 +14,7 @@ > #include <linux/iio/iio.h> > #include <linux/iio/driver.h> > #include <linux/module.h> > +#include <linux/property.h> why? > #include <linux/mod_devicetable.h> > > #include "ltc2497.h" > @@ -74,6 +75,7 @@ static int ltc2496_probe(struct spi_device *spi) > spi_set_drvdata(spi, indio_dev); > st->spi = spi; > st->common_ddata.result_and_measure = ltc2496_result_and_measure; > + st->common_ddata.chip_info = device_get_match_data(dev); > > return ltc2497core_probe(dev, indio_dev); > } > @@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi) > ltc2497core_remove(indio_dev); > } > > +static struct chip_info ltc2496_info = { > + .resolution = 16, > +}; > + > static const struct of_device_id ltc2496_of_match[] = { > - { .compatible = "lltc,ltc2496", }, > + { .compatible = "lltc,ltc2496", .data = (void *)<c2496_info }, as below. Take the chip_info structure const and drop the cast. > {}, > }; > MODULE_DEVICE_TABLE(of, ltc2496_of_match); > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c > index 2a485c8a1940..b2752399402c 100644 > --- a/drivers/iio/adc/ltc2497-core.c > +++ b/drivers/iio/adc/ltc2497-core.c > @@ -95,7 +95,7 @@ static int ltc2497core_read_raw(struct iio_dev *indio_dev, > return ret; > > *val = ret / 1000; > - *val2 = 17; > + *val2 = ddata->chip_info->resolution + 1; > > return IIO_VAL_FRACTIONAL_LOG2; > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c > index f7c786f37ceb..bb5e0d4301e2 100644 > --- a/drivers/iio/adc/ltc2497.c > +++ b/drivers/iio/adc/ltc2497.c > @@ -7,10 +7,14 @@ > * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf > */ > > +#include <linux/bitfield.h> > +#include <linux/bitops.h> Not immediately obvious why these need adding as part of this patch. If just general include improvement, separate patch please. > + Why blank line? > #include <linux/i2c.h> > #include <linux/iio/iio.h> > #include <linux/iio/driver.h> > #include <linux/module.h> > +#include <linux/property.h> May make sense, but not obvious why it's in this patch. If it's missing and should be present, please do it in a separate patch. > #include <linux/mod_devicetable.h> > > #include "ltc2497.h" > @@ -19,6 +23,8 @@ struct ltc2497_driverdata { > /* this must be the first member */ > struct ltc2497core_driverdata common_ddata; > struct i2c_client *client; > + u32 recv_size; > + u32 sub_lsb; > /* > * DMA (thus cache coherency maintenance) may require the > * transfer buffers to live in their own cache lines. > @@ -34,13 +40,14 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata, > int ret; > > if (val) { > - ret = i2c_master_recv(st->client, (char *)&st->buf, 3); > + ret = i2c_master_recv(st->client, (char *)&st->buf, st->recv_size); Not helpful yet, but there was a patch series under review form Jason Gerecke that changed the i2c transfer commands to use a u8. Which raises a question. buf is a __be32 which is rather odd given size of 3 in original code. If we have a case where it needs to have separate types for different transfers, use a union. > if (ret < 0) { > dev_err(&st->client->dev, "i2c_master_recv failed\n"); > return ret; > } > > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); Old code here is less than ideal, should be reading into 3 bytes then using the be24 accesors. Please fix whilst here. You will need multiple paths here depending on size. > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > + BIT(ddata->chip_info->resolution + 1); > } > > ret = i2c_smbus_write_byte(st->client, > @@ -54,6 +61,7 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata, > static int ltc2497_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > + u32 resolution; > struct iio_dev *indio_dev; > struct ltc2497_driverdata *st; > struct device *dev = &client->dev; > @@ -70,6 +78,11 @@ static int ltc2497_probe(struct i2c_client *client, > i2c_set_clientdata(client, indio_dev); > st->client = client; > st->common_ddata.result_and_measure = ltc2497_result_and_measure; > + st->common_ddata.chip_info = device_get_match_data(dev); > + > + resolution = st->common_ddata.chip_info->resolution; > + st->sub_lsb = 31 - (resolution + 1); > + st->recv_size = resolution / BITS_PER_BYTE + 1; > > return ltc2497core_probe(dev, indio_dev); > } > @@ -83,15 +96,26 @@ static int ltc2497_remove(struct i2c_client *client) > return 0; > } > > +static struct chip_info ltc2497_info[] = { Should be const - which is why you need the cast below. > + [TYPE_LTC2497] = { > + .resolution = 16, > + }, > + [TYPE_LTC2499] = { > + .resolution = 24, > + } > +}; > + > static const struct i2c_device_id ltc2497_id[] = { > - { "ltc2497", 0 }, > + { "ltc2497", TYPE_LTC2497 }, > + { "ltc2499", TYPE_LTC2499 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, ltc2497_id); > > static const struct of_device_id ltc2497_of_match[] = { > - { .compatible = "lltc,ltc2497", }, > - {}, > + { .compatible = "lltc,ltc2497", .data = (void *)<c2497_info[TYPE_LTC2497] }, > + { .compatible = "lltc,ltc2499", .data = (void *)<c2497_info[TYPE_LTC2499] }, Cast is a bad sign. Reason above. > + {} > }; > MODULE_DEVICE_TABLE(of, ltc2497_of_match); > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > index d0b42dd6b8ad..f4d939cfd48b 100644 > --- a/drivers/iio/adc/ltc2497.h > +++ b/drivers/iio/adc/ltc2497.h > @@ -4,9 +4,20 @@ > #define LTC2497_CONFIG_DEFAULT LTC2497_ENABLE > #define LTC2497_CONVERSION_TIME_MS 150ULL > > +enum chip_type { > + TYPE_LTC2496, > + TYPE_LTC2497, > + TYPE_LTC2499 > +}; > + > +struct chip_info { > + u32 resolution; > +}; > + > struct ltc2497core_driverdata { > struct regulator *ref; > - ktime_t time_prev; > + ktime_t time_prev; I'm staring at this and can't see the difference between the two lines. Closer inspection (i.e. messing with the stuff around it) tells me line you are removing has a tab when should be a space. Whilst trivial please call that out in patch description (or do it as a separate patch). > + const struct chip_info *chip_info; > u8 addr_prev; > int (*result_and_measure)(struct ltc2497core_driverdata *ddata, > u8 address, int *val);
On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 22 Aug 2022 15:51:05 +0300 > Ciprian Regus <ciprian.regus@analog.com> wrote: In reply to Jonathan's comments to answer his question and add more comments from me. ... > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> Tag block mustn't have the blank line(s). ... > > #include <linux/iio/iio.h> > > #include <linux/iio/driver.h> > > #include <linux/module.h> > > +#include <linux/property.h> > why? device_get_match_data() requires it. But why not sort them? > > #include <linux/mod_devicetable.h> ... > > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); > > Old code here is less than ideal, should be reading into 3 bytes then using > the be24 accesors. Please fix whilst here. You will need multiple paths here > depending on size. > > > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > > + BIT(ddata->chip_info->resolution + 1); Shouldn't this use some kind of sign_extend()? Also with a temporary variable for chip info this line can be single. struct ... *ci = ddata->chip_info; ...BIT(ci->resolution + 1) ... > > + u32 resolution; Keep this in a way that the "longer lines go first". ... > > + resolution = st->common_ddata.chip_info->resolution; > > + st->sub_lsb = 31 - (resolution + 1); > > + st->recv_size = resolution / BITS_PER_BYTE + 1; BITS_TO_BYTES() ... > > static const struct i2c_device_id ltc2497_id[] = { > > - { "ltc2497", 0 }, > > + { "ltc2497", TYPE_LTC2497 }, > > + { "ltc2499", TYPE_LTC2499 }, Use pointers here like you have done for the OF table. > > { } > > }; ... > > +enum chip_type { > > + TYPE_LTC2496, > > + TYPE_LTC2497, > > + TYPE_LTC2499 Keep trailing comma. > > +};
> ... > > > > #include <linux/iio/iio.h> > > > #include <linux/iio/driver.h> > > > #include <linux/module.h> > > > +#include <linux/property.h> > > why? > > device_get_match_data() requires it. Good point :) Not sure how I missed that!
In reply to one of Andy's questions. > On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> > wrote: > > On Mon, 22 Aug 2022 15:51:05 +0300 > > Ciprian Regus <ciprian.regus@analog.com> wrote: > > In reply to Jonathan's comments to answer his question and add more > comments from me. > > ... > > > > Datasheet: https://www.analog.com/media/en/technical- > documentation/data-sheets/2499fe.pdf > > > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > > Tag block mustn't have the blank line(s). > > ... > > > > #include <linux/iio/iio.h> > > > #include <linux/iio/driver.h> > > > #include <linux/module.h> > > > +#include <linux/property.h> > > why? > > device_get_match_data() requires it. > > But why not sort them? > > > > #include <linux/mod_devicetable.h> > > ... > > > > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); > > > > Old code here is less than ideal, should be reading into 3 bytes then using > > the be24 accesors. Please fix whilst here. You will need multiple paths here > > depending on size. > > > > > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > > > + BIT(ddata->chip_info->resolution + 1); > > Shouldn't this use some kind of sign_extend()? The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension, since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise. Regards, Ciprian > > Also with a temporary variable for chip info this line can be single. > > struct ... *ci = ddata->chip_info; > > ...BIT(ci->resolution + 1) > > ... > > > > + u32 resolution; > > Keep this in a way that the "longer lines go first". > > ... > > > > + resolution = st->common_ddata.chip_info->resolution; > > > + st->sub_lsb = 31 - (resolution + 1); > > > + st->recv_size = resolution / BITS_PER_BYTE + 1; > > BITS_TO_BYTES() > > ... > > > > static const struct i2c_device_id ltc2497_id[] = { > > > - { "ltc2497", 0 }, > > > + { "ltc2497", TYPE_LTC2497 }, > > > + { "ltc2499", TYPE_LTC2499 }, > > Use pointers here like you have done for the OF table. > > > > { } > > > }; > > ... > > > > +enum chip_type { > > > + TYPE_LTC2496, > > > + TYPE_LTC2497, > > > + TYPE_LTC2499 > > Keep trailing comma. > > > > +}; > > -- > With Best Regards, > Andy Shevchenko
On Mon, 29 Aug 2022 06:30:52 +0000 "Regus, Ciprian" <Ciprian.Regus@analog.com> wrote: > In reply to one of Andy's questions. > > > On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> > > wrote: > > > On Mon, 22 Aug 2022 15:51:05 +0300 > > > Ciprian Regus <ciprian.regus@analog.com> wrote: > > > > In reply to Jonathan's comments to answer his question and add more > > comments from me. > > > > ... > > > > > > Datasheet: https://www.analog.com/media/en/technical- > > documentation/data-sheets/2499fe.pdf > > > > > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > > > > Tag block mustn't have the blank line(s). > > > > ... > > > > > > #include <linux/iio/iio.h> > > > > #include <linux/iio/driver.h> > > > > #include <linux/module.h> > > > > +#include <linux/property.h> > > > why? > > > > device_get_match_data() requires it. > > > > But why not sort them? > > > > > > #include <linux/mod_devicetable.h> > > > > ... > > > > > > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); > > > > > > Old code here is less than ideal, should be reading into 3 bytes then using > > > the be24 accesors. Please fix whilst here. You will need multiple paths here > > > depending on size. > > > > > > > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > > > > + BIT(ddata->chip_info->resolution + 1); > > > > Shouldn't this use some kind of sign_extend()? > The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension, > since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise. This wins the award for one of the strangest formats I've yet seen. The format is normal 2s complement 24 bit, but with a bonus upper bit to allow representation of ever so slightly more than 24 bits. Specifically on the postive side the out of range value. 0x7fffff + 1 = 0x800000 (which would normally be considered wrapped around to most negative value. and on the negative side as well 0x800000 - 1 = 0x7fffff (normally most positive value). It does this by throwing in an additional bit for sign which only tells us something useful if in these two edge conditions (which are really an out of range error). That bit corresponds int his case to 0x1000000 and is set for postive values only. Thus our VS+ value becomes 0x01800000 and VS- value becomes 0x007fffff (extended to 32 bits for reasons that will become clear) Applying 2's complement subtraction of the magic value above. 0x01800000 - 0x01000000 = 0x00800000 (2^23) 0x007fffff - 0x01000000 = 0xff7fffff (-(2^23 + 1)) So it is indeed sign extended by the above. Perhaps a few comments to set people off along the right path to what is going on here would be useful? Jonathan > > Regards, > Ciprian > > > > Also with a temporary variable for chip info this line can be single. > > > > struct ... *ci = ddata->chip_info; > > > > ...BIT(ci->resolution + 1) > > > > ... > > > > > > + u32 resolution; > > > > Keep this in a way that the "longer lines go first". > > > > ... > > > > > > + resolution = st->common_ddata.chip_info->resolution; > > > > + st->sub_lsb = 31 - (resolution + 1); > > > > + st->recv_size = resolution / BITS_PER_BYTE + 1; > > > > BITS_TO_BYTES() > > > > ... > > > > > > static const struct i2c_device_id ltc2497_id[] = { > > > > - { "ltc2497", 0 }, > > > > + { "ltc2497", TYPE_LTC2497 }, > > > > + { "ltc2499", TYPE_LTC2499 }, > > > > Use pointers here like you have done for the OF table. > > > > > > { } > > > > }; > > > > ... > > > > > > +enum chip_type { > > > > + TYPE_LTC2496, > > > > + TYPE_LTC2497, > > > > + TYPE_LTC2499 > > > > Keep trailing comma. > > > > > > +}; > > > > -- > > With Best Regards, > > Andy Shevchenko
diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c index dfb3bb5997e5..98338104c24a 100644 --- a/drivers/iio/adc/ltc2496.c +++ b/drivers/iio/adc/ltc2496.c @@ -14,6 +14,7 @@ #include <linux/iio/iio.h> #include <linux/iio/driver.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/mod_devicetable.h> #include "ltc2497.h" @@ -74,6 +75,7 @@ static int ltc2496_probe(struct spi_device *spi) spi_set_drvdata(spi, indio_dev); st->spi = spi; st->common_ddata.result_and_measure = ltc2496_result_and_measure; + st->common_ddata.chip_info = device_get_match_data(dev); return ltc2497core_probe(dev, indio_dev); } @@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi) ltc2497core_remove(indio_dev); } +static struct chip_info ltc2496_info = { + .resolution = 16, +}; + static const struct of_device_id ltc2496_of_match[] = { - { .compatible = "lltc,ltc2496", }, + { .compatible = "lltc,ltc2496", .data = (void *)<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..bb5e0d4301e2 100644 --- a/drivers/iio/adc/ltc2497.c +++ b/drivers/iio/adc/ltc2497.c @@ -7,10 +7,14 @@ * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf */ +#include <linux/bitfield.h> +#include <linux/bitops.h> + #include <linux/i2c.h> #include <linux/iio/iio.h> #include <linux/iio/driver.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/mod_devicetable.h> #include "ltc2497.h" @@ -19,6 +23,8 @@ struct ltc2497_driverdata { /* this must be the first member */ struct ltc2497core_driverdata common_ddata; struct i2c_client *client; + u32 recv_size; + u32 sub_lsb; /* * DMA (thus cache coherency maintenance) may require the * transfer buffers to live in their own cache lines. @@ -34,13 +40,14 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata, int ret; if (val) { - ret = i2c_master_recv(st->client, (char *)&st->buf, 3); + ret = i2c_master_recv(st->client, (char *)&st->buf, st->recv_size); if (ret < 0) { dev_err(&st->client->dev, "i2c_master_recv failed\n"); return ret; } - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - + BIT(ddata->chip_info->resolution + 1); } ret = i2c_smbus_write_byte(st->client, @@ -54,6 +61,7 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata, static int ltc2497_probe(struct i2c_client *client, const struct i2c_device_id *id) { + u32 resolution; struct iio_dev *indio_dev; struct ltc2497_driverdata *st; struct device *dev = &client->dev; @@ -70,6 +78,11 @@ static int ltc2497_probe(struct i2c_client *client, i2c_set_clientdata(client, indio_dev); st->client = client; st->common_ddata.result_and_measure = ltc2497_result_and_measure; + st->common_ddata.chip_info = device_get_match_data(dev); + + resolution = st->common_ddata.chip_info->resolution; + st->sub_lsb = 31 - (resolution + 1); + st->recv_size = resolution / BITS_PER_BYTE + 1; return ltc2497core_probe(dev, indio_dev); } @@ -83,15 +96,26 @@ static int ltc2497_remove(struct i2c_client *client) return 0; } +static struct chip_info ltc2497_info[] = { + [TYPE_LTC2497] = { + .resolution = 16, + }, + [TYPE_LTC2499] = { + .resolution = 24, + } +}; + static const struct i2c_device_id ltc2497_id[] = { - { "ltc2497", 0 }, + { "ltc2497", TYPE_LTC2497 }, + { "ltc2499", TYPE_LTC2499 }, { } }; MODULE_DEVICE_TABLE(i2c, ltc2497_id); static const struct of_device_id ltc2497_of_match[] = { - { .compatible = "lltc,ltc2497", }, - {}, + { .compatible = "lltc,ltc2497", .data = (void *)<c2497_info[TYPE_LTC2497] }, + { .compatible = "lltc,ltc2499", .data = (void *)<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..f4d939cfd48b 100644 --- a/drivers/iio/adc/ltc2497.h +++ b/drivers/iio/adc/ltc2497.h @@ -4,9 +4,20 @@ #define LTC2497_CONFIG_DEFAULT LTC2497_ENABLE #define LTC2497_CONVERSION_TIME_MS 150ULL +enum chip_type { + TYPE_LTC2496, + TYPE_LTC2497, + TYPE_LTC2499 +}; + +struct chip_info { + u32 resolution; +}; + struct ltc2497core_driverdata { struct regulator *ref; - ktime_t time_prev; + ktime_t time_prev; + const struct chip_info *chip_info; u8 addr_prev; int (*result_and_measure)(struct ltc2497core_driverdata *ddata, u8 address, int *val);
The LTC2499 is a 16-channel (eight differential), 24-bit, ADC with Easy Drive technology and a 2-wire, I2C interface. Implement support for the LTC2499 ADC by extending the LTC2497 driver. A new chip_info struct is added to differentiate between chip types and resolutions when reading data from the device. Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> --- drivers/iio/adc/ltc2496.c | 8 +++++++- drivers/iio/adc/ltc2497-core.c | 2 +- drivers/iio/adc/ltc2497.c | 34 +++++++++++++++++++++++++++++----- drivers/iio/adc/ltc2497.h | 13 ++++++++++++- 4 files changed, 49 insertions(+), 8 deletions(-)