* [PATCH 0/3] Add support for LTC2499 ADC
@ 2022-08-22 12:51 Ciprian Regus
2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Ciprian Regus @ 2022-08-22 12:51 UTC (permalink / raw)
To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree,
linux-kernel
Cc: u.kleine-koenig, Ciprian Regus
The LTC2499 is a 16-channel (eight differential), 24-bit,
ADC with Easy Drive technology and a 2-wire, I2C interface.
This adds support for the LTC2499 ADC by extending the LTC2497
driver.
Note: This fix is required to be applied first:
https://patchwork.kernel.org/project/linux-iio/patch/20220815091647.1523532-1-dzagorui@cisco.com
Ciprian Regus (3):
dt-bindings: iio: adc: Add docs for LTC2499
drivers: iio: adc: LTC2499 support
drivers: iio: adc: Rename the LTC249x iio device
.../bindings/iio/adc/lltc,ltc2497.yaml | 6 +++-
MAINTAINERS | 1 +
drivers/iio/adc/ltc2496.c | 9 ++++-
drivers/iio/adc/ltc2497-core.c | 4 +--
drivers/iio/adc/ltc2497.c | 36 ++++++++++++++++---
drivers/iio/adc/ltc2497.h | 14 +++++++-
6 files changed, 60 insertions(+), 10 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 2022-08-22 12:51 [PATCH 0/3] Add support for LTC2499 ADC Ciprian Regus @ 2022-08-22 12:51 ` Ciprian Regus 2022-08-22 19:24 ` Jonathan Cameron 2022-08-22 19:40 ` Rob Herring 2022-08-22 12:51 ` [PATCH 2/3] drivers: iio: adc: LTC2499 support Ciprian Regus 2022-08-22 12:51 ` [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device Ciprian Regus 2 siblings, 2 replies; 17+ messages in thread From: Ciprian Regus @ 2022-08-22 12:51 UTC (permalink / raw) To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Cc: u.kleine-koenig, Ciprian Regus Update the bindings documentation for ltc2497 to include the ltc2499. Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> --- Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++- MAINTAINERS | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml index c1772b568cd1..7bb30eafc543 100644 --- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml @@ -13,10 +13,14 @@ description: | 16bit ADC supporting up to 16 single ended or 8 differential inputs. I2C interface. + https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf + properties: compatible: - const: + enum: lltc,ltc2497 + lltc,ltc2499 reg: true vref-supply: true diff --git a/MAINTAINERS b/MAINTAINERS index 9d7f64dc0efe..3c847619ceb1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1327,6 +1327,7 @@ W: https://ez.analog.com/linux-software-drivers F: Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523 F: Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350 F: Documentation/devicetree/bindings/iio/*/adi,* +F: Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml F: Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml F: drivers/iio/*/ad* F: drivers/iio/adc/ltc249* -- 2.30.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus @ 2022-08-22 19:24 ` Jonathan Cameron 2022-08-22 19:39 ` Jonathan Cameron 2022-08-22 19:40 ` Rob Herring 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2022-08-22 19:24 UTC (permalink / raw) To: Ciprian Regus Cc: krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel, u.kleine-koenig On Mon, 22 Aug 2022 15:51:04 +0300 Ciprian Regus <ciprian.regus@analog.com> wrote: > Update the bindings documentation for ltc2497 to include the ltc2499. > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> Looks fine to me. Maybe a separate patch to add the ltc2496 binding given the driver is included in this MAINTAINERS entry? A random comment inline. Jonathan > --- > Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++- > MAINTAINERS | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > index c1772b568cd1..7bb30eafc543 100644 > --- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > @@ -13,10 +13,14 @@ description: | > 16bit ADC supporting up to 16 single ended or 8 differential inputs. > I2C interface. > > + https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > + > properties: > compatible: > - const: > + enum: > lltc,ltc2497 > + lltc,ltc2499 > > reg: true > vref-supply: true > diff --git a/MAINTAINERS b/MAINTAINERS > index 9d7f64dc0efe..3c847619ceb1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1327,6 +1327,7 @@ W: https://ez.analog.com/linux-software-drivers > F: Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523 > F: Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350 > F: Documentation/devicetree/bindings/iio/*/adi,* > +F: Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > F: Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml Unrelated question, but why is this here given the wild cards above match? > F: drivers/iio/*/ad* > F: drivers/iio/adc/ltc249* ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 2022-08-22 19:24 ` Jonathan Cameron @ 2022-08-22 19:39 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2022-08-22 19:39 UTC (permalink / raw) To: Ciprian Regus Cc: krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel, u.kleine-koenig On Mon, 22 Aug 2022 20:24:35 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 22 Aug 2022 15:51:04 +0300 > Ciprian Regus <ciprian.regus@analog.com> wrote: > > > Update the bindings documentation for ltc2497 to include the ltc2499. > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > Looks fine to me. Maybe a separate patch to add the ltc2496 binding > given the driver is included in this MAINTAINERS entry? > A random comment inline. > > Jonathan > > > --- > > Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++- > > MAINTAINERS | 1 + > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > > index c1772b568cd1..7bb30eafc543 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > > @@ -13,10 +13,14 @@ description: | > > 16bit ADC supporting up to 16 single ended or 8 differential inputs. > > I2C interface. > > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/2497fb.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > > + > > properties: > > compatible: > > - const: > > + enum: > > lltc,ltc2497 > > + lltc,ltc2499 Oops. Benefit of Rob's build bot later. enum: - lltc,ltc2497 - lltc,ltc2499 Just goes to show how wonderful automatic checking is :) Jonathan > > > > reg: true > > vref-supply: true > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 9d7f64dc0efe..3c847619ceb1 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1327,6 +1327,7 @@ W: https://ez.analog.com/linux-software-drivers > > F: Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523 > > F: Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350 > > F: Documentation/devicetree/bindings/iio/*/adi,* > > +F: Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml > > F: Documentation/devicetree/bindings/iio/dac/adi,ad5758.yaml > > Unrelated question, but why is this here given the wild cards above match? > > > F: drivers/iio/*/ad* > > F: drivers/iio/adc/ltc249* ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus 2022-08-22 19:24 ` Jonathan Cameron @ 2022-08-22 19:40 ` Rob Herring 1 sibling, 0 replies; 17+ messages in thread From: Rob Herring @ 2022-08-22 19:40 UTC (permalink / raw) To: Ciprian Regus Cc: devicetree, linux-iio, jic23, linux-kernel, krzysztof.kozlowski+dt, u.kleine-koenig, robh+dt On Mon, 22 Aug 2022 15:51:04 +0300, Ciprian Regus wrote: > Update the bindings documentation for ltc2497 to include the ltc2499. > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > --- > Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml | 6 +++++- > MAINTAINERS | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array' from schema $id: http://json-schema.org/draft-07/schema# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: properties:compatible:enum: 'lltc,ltc2497 lltc,ltc2499' is not of type 'array' from schema $id: http://devicetree.org/meta-schemas/string-array.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.yaml: ignoring, error in schema: properties: compatible: enum Documentation/devicetree/bindings/iio/adc/lltc,ltc2497.example.dtb:0:0: /example-0/i2c/adc@76: failed to match any schema with compatible: ['lltc,ltc2497'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-22 12:51 [PATCH 0/3] Add support for LTC2499 ADC Ciprian Regus 2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus @ 2022-08-22 12:51 ` Ciprian Regus 2022-08-22 19:37 ` Jonathan Cameron 2022-08-22 12:51 ` [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device Ciprian Regus 2 siblings, 1 reply; 17+ messages in thread From: Ciprian Regus @ 2022-08-22 12:51 UTC (permalink / raw) To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Cc: u.kleine-koenig, Ciprian Regus The LTC2499 is a 16-channel (eight differential), 24-bit, ADC with Easy Drive technology and a 2-wire, I2C interface. Implement support for the LTC2499 ADC by extending the LTC2497 driver. A new chip_info struct is added to differentiate between chip types and resolutions when reading data from the device. Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> --- drivers/iio/adc/ltc2496.c | 8 +++++++- drivers/iio/adc/ltc2497-core.c | 2 +- drivers/iio/adc/ltc2497.c | 34 +++++++++++++++++++++++++++++----- drivers/iio/adc/ltc2497.h | 13 ++++++++++++- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c index dfb3bb5997e5..98338104c24a 100644 --- a/drivers/iio/adc/ltc2496.c +++ b/drivers/iio/adc/ltc2496.c @@ -14,6 +14,7 @@ #include <linux/iio/iio.h> #include <linux/iio/driver.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/mod_devicetable.h> #include "ltc2497.h" @@ -74,6 +75,7 @@ static int ltc2496_probe(struct spi_device *spi) spi_set_drvdata(spi, indio_dev); st->spi = spi; st->common_ddata.result_and_measure = ltc2496_result_and_measure; + st->common_ddata.chip_info = device_get_match_data(dev); return ltc2497core_probe(dev, indio_dev); } @@ -85,8 +87,12 @@ static void ltc2496_remove(struct spi_device *spi) ltc2497core_remove(indio_dev); } +static struct chip_info ltc2496_info = { + .resolution = 16, +}; + static const struct of_device_id ltc2496_of_match[] = { - { .compatible = "lltc,ltc2496", }, + { .compatible = "lltc,ltc2496", .data = (void *)<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); -- 2.30.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-22 12:51 ` [PATCH 2/3] drivers: iio: adc: LTC2499 support Ciprian Regus @ 2022-08-22 19:37 ` Jonathan Cameron 2022-08-23 15:21 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2022-08-22 19:37 UTC (permalink / raw) To: Ciprian Regus Cc: krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel, u.kleine-koenig 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); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-22 19:37 ` Jonathan Cameron @ 2022-08-23 15:21 ` Andy Shevchenko 2022-08-24 12:15 ` Jonathan Cameron 2022-08-29 6:30 ` Regus, Ciprian 0 siblings, 2 replies; 17+ messages in thread From: Andy Shevchenko @ 2022-08-23 15:21 UTC (permalink / raw) To: Jonathan Cameron Cc: Ciprian Regus, Krzysztof Kozlowski, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List, Uwe Kleine-König On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 22 Aug 2022 15:51:05 +0300 > Ciprian Regus <ciprian.regus@analog.com> wrote: In reply to Jonathan's comments to answer his question and add more comments from me. ... > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2499fe.pdf > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> Tag block mustn't have the blank line(s). ... > > #include <linux/iio/iio.h> > > #include <linux/iio/driver.h> > > #include <linux/module.h> > > +#include <linux/property.h> > why? device_get_match_data() requires it. But why not sort them? > > #include <linux/mod_devicetable.h> ... > > - *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17); > > Old code here is less than ideal, should be reading into 3 bytes then using > the be24 accesors. Please fix whilst here. You will need multiple paths here > depending on size. > > > + *val = (be32_to_cpu(st->buf) >> st->sub_lsb) - > > + BIT(ddata->chip_info->resolution + 1); Shouldn't this use some kind of sign_extend()? Also with a temporary variable for chip info this line can be single. struct ... *ci = ddata->chip_info; ...BIT(ci->resolution + 1) ... > > + u32 resolution; Keep this in a way that the "longer lines go first". ... > > + resolution = st->common_ddata.chip_info->resolution; > > + st->sub_lsb = 31 - (resolution + 1); > > + st->recv_size = resolution / BITS_PER_BYTE + 1; BITS_TO_BYTES() ... > > static const struct i2c_device_id ltc2497_id[] = { > > - { "ltc2497", 0 }, > > + { "ltc2497", TYPE_LTC2497 }, > > + { "ltc2499", TYPE_LTC2499 }, Use pointers here like you have done for the OF table. > > { } > > }; ... > > +enum chip_type { > > + TYPE_LTC2496, > > + TYPE_LTC2497, > > + TYPE_LTC2499 Keep trailing comma. > > +}; -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-23 15:21 ` Andy Shevchenko @ 2022-08-24 12:15 ` Jonathan Cameron 2022-08-29 6:30 ` Regus, Ciprian 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2022-08-24 12:15 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Ciprian Regus, Krzysztof Kozlowski, Rob Herring, linux-iio, devicetree, Linux Kernel Mailing List, Uwe Kleine-König > ... > > > > #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! ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-23 15:21 ` Andy Shevchenko 2022-08-24 12:15 ` Jonathan Cameron @ 2022-08-29 6:30 ` Regus, Ciprian 2022-08-29 16:47 ` Jonathan Cameron 1 sibling, 1 reply; 17+ messages in thread From: Regus, Ciprian @ 2022-08-29 6:30 UTC (permalink / raw) To: Andy Shevchenko Cc: Krzysztof Kozlowski, Linux Kernel Mailing List, Rob Herring, linux-iio, devicetree, Jonathan Cameron, Uwe Kleine-König 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support 2022-08-29 6:30 ` Regus, Ciprian @ 2022-08-29 16:47 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2022-08-29 16:47 UTC (permalink / raw) To: Regus, Ciprian Cc: Andy Shevchenko, Krzysztof Kozlowski, Linux Kernel Mailing List, Rob Herring, linux-iio, devicetree, Uwe Kleine-König 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-22 12:51 [PATCH 0/3] Add support for LTC2499 ADC Ciprian Regus 2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus 2022-08-22 12:51 ` [PATCH 2/3] drivers: iio: adc: LTC2499 support Ciprian Regus @ 2022-08-22 12:51 ` Ciprian Regus 2022-08-22 14:08 ` Lars-Peter Clausen 2022-08-23 11:09 ` Krzysztof Kozlowski 2 siblings, 2 replies; 17+ messages in thread From: Ciprian Regus @ 2022-08-22 12:51 UTC (permalink / raw) To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Cc: u.kleine-koenig, Ciprian Regus Set the iio device's name based on the chip used. Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> --- drivers/iio/adc/ltc2496.c | 1 + drivers/iio/adc/ltc2497-core.c | 2 +- drivers/iio/adc/ltc2497.c | 2 ++ drivers/iio/adc/ltc2497.h | 1 + 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c index 98338104c24a..86470f49e8ca 100644 --- a/drivers/iio/adc/ltc2496.c +++ b/drivers/iio/adc/ltc2496.c @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi) static struct chip_info ltc2496_info = { .resolution = 16, + .name = "ltc2496" }; static const struct of_device_id ltc2496_of_match[] = { diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c index b2752399402c..6dd9ab601904 100644 --- a/drivers/iio/adc/ltc2497-core.c +++ b/drivers/iio/adc/ltc2497-core.c @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev) struct ltc2497core_driverdata *ddata = iio_priv(indio_dev); int ret; - indio_dev->name = dev_name(dev); + indio_dev->name = ddata->chip_info->name; indio_dev->info = <c2497core_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = ltc2497core_channel; diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c index bb5e0d4301e2..a0aad71c8130 100644 --- a/drivers/iio/adc/ltc2497.c +++ b/drivers/iio/adc/ltc2497.c @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client) static struct chip_info ltc2497_info[] = { [TYPE_LTC2497] = { .resolution = 16, + .name = "ltc2497" }, [TYPE_LTC2499] = { .resolution = 24, + .name = "ltc2499" } }; diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h index f4d939cfd48b..0e86e38248ee 100644 --- a/drivers/iio/adc/ltc2497.h +++ b/drivers/iio/adc/ltc2497.h @@ -12,6 +12,7 @@ enum chip_type { struct chip_info { u32 resolution; + char *name; }; struct ltc2497core_driverdata { -- 2.30.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-22 12:51 ` [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device Ciprian Regus @ 2022-08-22 14:08 ` Lars-Peter Clausen 2022-08-22 19:17 ` Jonathan Cameron 2022-08-29 6:13 ` Regus, Ciprian 2022-08-23 11:09 ` Krzysztof Kozlowski 1 sibling, 2 replies; 17+ messages in thread From: Lars-Peter Clausen @ 2022-08-22 14:08 UTC (permalink / raw) To: Ciprian Regus, jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Cc: u.kleine-koenig On 8/22/22 14:51, Ciprian Regus wrote: > Set the iio device's name based on the chip used. While the change is correct it breaks the ABI. This needs a bit of a better explanation what is being done, why, what are the potential problems, why do we want to do it anyway. > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > --- > drivers/iio/adc/ltc2496.c | 1 + > drivers/iio/adc/ltc2497-core.c | 2 +- > drivers/iio/adc/ltc2497.c | 2 ++ > drivers/iio/adc/ltc2497.h | 1 + > 4 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c > index 98338104c24a..86470f49e8ca 100644 > --- a/drivers/iio/adc/ltc2496.c > +++ b/drivers/iio/adc/ltc2496.c > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi) > > static struct chip_info ltc2496_info = { > .resolution = 16, > + .name = "ltc2496" > }; > > static const struct of_device_id ltc2496_of_match[] = { > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c > index b2752399402c..6dd9ab601904 100644 > --- a/drivers/iio/adc/ltc2497-core.c > +++ b/drivers/iio/adc/ltc2497-core.c > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev) > struct ltc2497core_driverdata *ddata = iio_priv(indio_dev); > int ret; > > - indio_dev->name = dev_name(dev); > + indio_dev->name = ddata->chip_info->name; > indio_dev->info = <c2497core_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = ltc2497core_channel; > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c > index bb5e0d4301e2..a0aad71c8130 100644 > --- a/drivers/iio/adc/ltc2497.c > +++ b/drivers/iio/adc/ltc2497.c > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client) > static struct chip_info ltc2497_info[] = { > [TYPE_LTC2497] = { > .resolution = 16, > + .name = "ltc2497" > }, > [TYPE_LTC2499] = { > .resolution = 24, > + .name = "ltc2499" > } > }; > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > index f4d939cfd48b..0e86e38248ee 100644 > --- a/drivers/iio/adc/ltc2497.h > +++ b/drivers/iio/adc/ltc2497.h > @@ -12,6 +12,7 @@ enum chip_type { > > struct chip_info { > u32 resolution; > + char *name; > }; > > struct ltc2497core_driverdata { ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-22 14:08 ` Lars-Peter Clausen @ 2022-08-22 19:17 ` Jonathan Cameron 2022-08-29 6:13 ` Regus, Ciprian 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2022-08-22 19:17 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Ciprian Regus, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel, u.kleine-koenig On Mon, 22 Aug 2022 16:08:12 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 8/22/22 14:51, Ciprian Regus wrote: > > Set the iio device's name based on the chip used. > > While the change is correct it breaks the ABI. This needs a bit of a > better explanation what is being done, why, what are the potential > problems, why do we want to do it anyway. + it's a fix, so if we are doing this we need to enable backporting by moving to first patch in series (obviously just for the already supported parts). Jonathan > > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > > --- > > drivers/iio/adc/ltc2496.c | 1 + > > drivers/iio/adc/ltc2497-core.c | 2 +- > > drivers/iio/adc/ltc2497.c | 2 ++ > > drivers/iio/adc/ltc2497.h | 1 + > > 4 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c > > index 98338104c24a..86470f49e8ca 100644 > > --- a/drivers/iio/adc/ltc2496.c > > +++ b/drivers/iio/adc/ltc2496.c > > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi) > > > > static struct chip_info ltc2496_info = { > > .resolution = 16, > > + .name = "ltc2496" > > }; > > > > static const struct of_device_id ltc2496_of_match[] = { > > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c > > index b2752399402c..6dd9ab601904 100644 > > --- a/drivers/iio/adc/ltc2497-core.c > > +++ b/drivers/iio/adc/ltc2497-core.c > > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct iio_dev *indio_dev) > > struct ltc2497core_driverdata *ddata = iio_priv(indio_dev); > > int ret; > > > > - indio_dev->name = dev_name(dev); > > + indio_dev->name = ddata->chip_info->name; > > indio_dev->info = <c2497core_info; > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->channels = ltc2497core_channel; > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c > > index bb5e0d4301e2..a0aad71c8130 100644 > > --- a/drivers/iio/adc/ltc2497.c > > +++ b/drivers/iio/adc/ltc2497.c > > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client) > > static struct chip_info ltc2497_info[] = { > > [TYPE_LTC2497] = { > > .resolution = 16, > > + .name = "ltc2497" > > }, > > [TYPE_LTC2499] = { > > .resolution = 24, > > + .name = "ltc2499" > > } > > }; > > > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > > index f4d939cfd48b..0e86e38248ee 100644 > > --- a/drivers/iio/adc/ltc2497.h > > +++ b/drivers/iio/adc/ltc2497.h > > @@ -12,6 +12,7 @@ enum chip_type { > > > > struct chip_info { > > u32 resolution; > > + char *name; > > }; > > > > struct ltc2497core_driverdata { > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-22 14:08 ` Lars-Peter Clausen 2022-08-22 19:17 ` Jonathan Cameron @ 2022-08-29 6:13 ` Regus, Ciprian 2022-08-29 16:03 ` Jonathan Cameron 1 sibling, 1 reply; 17+ messages in thread From: Regus, Ciprian @ 2022-08-29 6:13 UTC (permalink / raw) To: Lars-Peter Clausen Cc: u.kleine-koenig, jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Hi Lars, Thanks for the review. I have one question before going further with this patch. > On 8/22/22 14:51, Ciprian Regus wrote: > > Set the iio device's name based on the chip used. > > While the change is correct it breaks the ABI. This needs a bit of a > better explanation what is being done, why, what are the potential > problems, why do we want to do it anyway. The reason for this change is to make it easier to identify the device (by having a generic name) when using an IIO client. The current name is based on the i2c_client's kobj name, which has the format "i2c_instance -i2c_address " (e.g. 1-0076). So, the renaming would allow for interacting with the device without knowing which bus it's connected to. Also, as far as I can see, the IIO ABI states that the device's name is "Typically a part number". For the reason given, would this change be considered an improvement, or is the ABI breaking too much of a drawback? (in which case I'll just drop this patch) Thanks, Ciprian > > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > > --- > > drivers/iio/adc/ltc2496.c | 1 + > > drivers/iio/adc/ltc2497-core.c | 2 +- > > drivers/iio/adc/ltc2497.c | 2 ++ > > drivers/iio/adc/ltc2497.h | 1 + > > 4 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c > > index 98338104c24a..86470f49e8ca 100644 > > --- a/drivers/iio/adc/ltc2496.c > > +++ b/drivers/iio/adc/ltc2496.c > > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi) > > > > static struct chip_info ltc2496_info = { > > .resolution = 16, > > + .name = "ltc2496" > > }; > > > > static const struct of_device_id ltc2496_of_match[] = { > > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c > > index b2752399402c..6dd9ab601904 100644 > > --- a/drivers/iio/adc/ltc2497-core.c > > +++ b/drivers/iio/adc/ltc2497-core.c > > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct > iio_dev *indio_dev) > > struct ltc2497core_driverdata *ddata = iio_priv(indio_dev); > > int ret; > > > > - indio_dev->name = dev_name(dev); > > + indio_dev->name = ddata->chip_info->name; > > indio_dev->info = <c2497core_info; > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->channels = ltc2497core_channel; > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c > > index bb5e0d4301e2..a0aad71c8130 100644 > > --- a/drivers/iio/adc/ltc2497.c > > +++ b/drivers/iio/adc/ltc2497.c > > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client) > > static struct chip_info ltc2497_info[] = { > > [TYPE_LTC2497] = { > > .resolution = 16, > > + .name = "ltc2497" > > }, > > [TYPE_LTC2499] = { > > .resolution = 24, > > + .name = "ltc2499" > > } > > }; > > > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > > index f4d939cfd48b..0e86e38248ee 100644 > > --- a/drivers/iio/adc/ltc2497.h > > +++ b/drivers/iio/adc/ltc2497.h > > @@ -12,6 +12,7 @@ enum chip_type { > > > > struct chip_info { > > u32 resolution; > > + char *name; > > }; > > > > struct ltc2497core_driverdata { > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-29 6:13 ` Regus, Ciprian @ 2022-08-29 16:03 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2022-08-29 16:03 UTC (permalink / raw) To: Regus, Ciprian Cc: Lars-Peter Clausen, u.kleine-koenig, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel On Mon, 29 Aug 2022 06:13:52 +0000 "Regus, Ciprian" <Ciprian.Regus@analog.com> wrote: > Hi Lars, > > Thanks for the review. I have one question before going further > with this patch. > > > On 8/22/22 14:51, Ciprian Regus wrote: > > > Set the iio device's name based on the chip used. > > > > While the change is correct it breaks the ABI. This needs a bit of a > > better explanation what is being done, why, what are the potential > > problems, why do we want to do it anyway. > > The reason for this change is to make it easier to identify the device > (by having a generic name) when using an IIO client. The current name is > based on the i2c_client's kobj name, which has the format > "i2c_instance -i2c_address " (e.g. 1-0076). So, the renaming would allow > for interacting with the device without knowing which bus it's connected > to. > > Also, as far as I can see, the IIO ABI states that the device's name is > "Typically a part number". > > For the reason given, would this change be considered an > improvement, or is the ABI breaking too much of a drawback? > (in which case I'll just drop this patch) Hi Ciprian There are a set of drivers that do it similarly to this (amongst other things because I wasn't paying attention a few years back). Generally we've considered them unfixable unfortunately because it's common to use name to find the device - so userspace breakage is very likely. If we are introducing new device support thought here can't be a platform already relying on the old naming. So weird though it seems, can you fix this just for the newly supported models and leave the old ones 'broken'? (with a big note on why!) Perhaps do that by leaving .name = NULL for the older parts and doing a check on that to select behaviour. Thanks, Jonathan > > Thanks, > Ciprian > > > > > > > > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com> > > > --- > > > drivers/iio/adc/ltc2496.c | 1 + > > > drivers/iio/adc/ltc2497-core.c | 2 +- > > > drivers/iio/adc/ltc2497.c | 2 ++ > > > drivers/iio/adc/ltc2497.h | 1 + > > > 4 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c > > > index 98338104c24a..86470f49e8ca 100644 > > > --- a/drivers/iio/adc/ltc2496.c > > > +++ b/drivers/iio/adc/ltc2496.c > > > @@ -89,6 +89,7 @@ static void ltc2496_remove(struct spi_device *spi) > > > > > > static struct chip_info ltc2496_info = { > > > .resolution = 16, > > > + .name = "ltc2496" > > > }; > > > > > > static const struct of_device_id ltc2496_of_match[] = { > > > diff --git a/drivers/iio/adc/ltc2497-core.c b/drivers/iio/adc/ltc2497-core.c > > > index b2752399402c..6dd9ab601904 100644 > > > --- a/drivers/iio/adc/ltc2497-core.c > > > +++ b/drivers/iio/adc/ltc2497-core.c > > > @@ -169,7 +169,7 @@ int ltc2497core_probe(struct device *dev, struct > > iio_dev *indio_dev) > > > struct ltc2497core_driverdata *ddata = iio_priv(indio_dev); > > > int ret; > > > > > > - indio_dev->name = dev_name(dev); > > > + indio_dev->name = ddata->chip_info->name; > > > indio_dev->info = <c2497core_info; > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > indio_dev->channels = ltc2497core_channel; > > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c > > > index bb5e0d4301e2..a0aad71c8130 100644 > > > --- a/drivers/iio/adc/ltc2497.c > > > +++ b/drivers/iio/adc/ltc2497.c > > > @@ -99,9 +99,11 @@ static int ltc2497_remove(struct i2c_client *client) > > > static struct chip_info ltc2497_info[] = { > > > [TYPE_LTC2497] = { > > > .resolution = 16, > > > + .name = "ltc2497" > > > }, > > > [TYPE_LTC2499] = { > > > .resolution = 24, > > > + .name = "ltc2499" > > > } > > > }; > > > > > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > > > index f4d939cfd48b..0e86e38248ee 100644 > > > --- a/drivers/iio/adc/ltc2497.h > > > +++ b/drivers/iio/adc/ltc2497.h > > > @@ -12,6 +12,7 @@ enum chip_type { > > > > > > struct chip_info { > > > u32 resolution; > > > + char *name; > > > }; > > > > > > struct ltc2497core_driverdata { > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device 2022-08-22 12:51 ` [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device Ciprian Regus 2022-08-22 14:08 ` Lars-Peter Clausen @ 2022-08-23 11:09 ` Krzysztof Kozlowski 1 sibling, 0 replies; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-08-23 11:09 UTC (permalink / raw) To: Ciprian Regus, jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree, linux-kernel Cc: u.kleine-koenig On 22/08/2022 15:51, Ciprian Regus wrote: (...) > > diff --git a/drivers/iio/adc/ltc2497.h b/drivers/iio/adc/ltc2497.h > index f4d939cfd48b..0e86e38248ee 100644 > --- a/drivers/iio/adc/ltc2497.h > +++ b/drivers/iio/adc/ltc2497.h > @@ -12,6 +12,7 @@ enum chip_type { > > struct chip_info { > u32 resolution; > + char *name; const char * Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-29 17:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-22 12:51 [PATCH 0/3] Add support for LTC2499 ADC Ciprian Regus 2022-08-22 12:51 ` [PATCH 1/3] dt-bindings: iio: adc: Add docs for LTC2499 Ciprian Regus 2022-08-22 19:24 ` Jonathan Cameron 2022-08-22 19:39 ` Jonathan Cameron 2022-08-22 19:40 ` Rob Herring 2022-08-22 12:51 ` [PATCH 2/3] drivers: iio: adc: LTC2499 support Ciprian Regus 2022-08-22 19:37 ` Jonathan Cameron 2022-08-23 15:21 ` Andy Shevchenko 2022-08-24 12:15 ` Jonathan Cameron 2022-08-29 6:30 ` Regus, Ciprian 2022-08-29 16:47 ` Jonathan Cameron 2022-08-22 12:51 ` [PATCH 3/3] drivers: iio: adc: Rename the LTC249x iio device Ciprian Regus 2022-08-22 14:08 ` Lars-Peter Clausen 2022-08-22 19:17 ` Jonathan Cameron 2022-08-29 6:13 ` Regus, Ciprian 2022-08-29 16:03 ` Jonathan Cameron 2022-08-23 11:09 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).