* [PATCH 0/3] Support more parts in LTC2983 @ 2022-10-14 12:37 Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once Cosmin Tanislav ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-14 12:37 UTC (permalink / raw) Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav Add support for the following parts: * LTC2984 * LTC2986 * LTM2985 The LTC2984 is a variant of the LTC2983 with EEPROM. The LTC2986 is a variant of the LTC2983 with only 10 channels, EEPROM and support for active analog temperature sensors. The LTM2985 is software-compatible with the LTC2986. Also, remove excessive allocations on resume. Cosmin Tanislav (3): iio: temperature: ltc2983: allocate iio channels once dt-bindings: iio: temperature: ltc2983: support more parts iio: temperature: ltc2983: support more parts .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++- drivers/iio/temperature/ltc2983.c | 195 ++++++++++++++++-- 2 files changed, 240 insertions(+), 18 deletions(-) -- 2.37.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once 2022-10-14 12:37 [PATCH 0/3] Support more parts in LTC2983 Cosmin Tanislav @ 2022-10-14 12:37 ` Cosmin Tanislav 2022-10-14 14:11 ` Jonathan Cameron 2022-10-14 12:37 ` [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 3/3] " Cosmin Tanislav 2 siblings, 1 reply; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-14 12:37 UTC (permalink / raw) Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav From: Cosmin Tanislav <cosmin.tanislav@analog.com> Currently, every time the device wakes up from sleep, the iio_chan array is reallocated, leaking the previous one until the device is removed (basically never). Move the allocation to the probe function to avoid this. Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> --- drivers/iio/temperature/ltc2983.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c index b652d2b39bcf..a60ccf183687 100644 --- a/drivers/iio/temperature/ltc2983.c +++ b/drivers/iio/temperature/ltc2983.c @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) return ret; } - st->iio_chan = devm_kzalloc(&st->spi->dev, - st->iio_channels * sizeof(*st->iio_chan), - GFP_KERNEL); - - if (!st->iio_chan) - return -ENOMEM; - ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG, LTC2983_NOTCH_FREQ_MASK, LTC2983_NOTCH_FREQ(st->filter_notch_freq)); @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi) gpiod_set_value_cansleep(gpio, 0); } + st->iio_chan = devm_kzalloc(&spi->dev, + st->iio_channels * sizeof(*st->iio_chan), + GFP_KERNEL); + if (!st->iio_chan) + return -ENOMEM; + ret = ltc2983_setup(st, true); if (ret) return ret; -- 2.37.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once 2022-10-14 12:37 ` [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once Cosmin Tanislav @ 2022-10-14 14:11 ` Jonathan Cameron 2022-10-14 15:18 ` Jonathan Cameron 0 siblings, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-10-14 14:11 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Fri, 14 Oct 2022 15:37:22 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Currently, every time the device wakes up from sleep, the > iio_chan array is reallocated, leaking the previous one > until the device is removed (basically never). > > Move the allocation to the probe function to avoid this. > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> Hi Cosmin, Please give a fixes tag for this one as we'll definitely want to backport it. Reply to this patch is fine as b4 will pick it up like any other tag. > --- > drivers/iio/temperature/ltc2983.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c > index b652d2b39bcf..a60ccf183687 100644 > --- a/drivers/iio/temperature/ltc2983.c > +++ b/drivers/iio/temperature/ltc2983.c > @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) > return ret; > } > > - st->iio_chan = devm_kzalloc(&st->spi->dev, > - st->iio_channels * sizeof(*st->iio_chan), > - GFP_KERNEL); > - > - if (!st->iio_chan) > - return -ENOMEM; > - > ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG, > LTC2983_NOTCH_FREQ_MASK, > LTC2983_NOTCH_FREQ(st->filter_notch_freq)); > @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi) > gpiod_set_value_cansleep(gpio, 0); > } > > + st->iio_chan = devm_kzalloc(&spi->dev, > + st->iio_channels * sizeof(*st->iio_chan), > + GFP_KERNEL); > + if (!st->iio_chan) > + return -ENOMEM; > + > ret = ltc2983_setup(st, true); > if (ret) > return ret; ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once 2022-10-14 14:11 ` Jonathan Cameron @ 2022-10-14 15:18 ` Jonathan Cameron 2022-10-15 16:35 ` Jonathan Cameron 0 siblings, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-10-14 15:18 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Fri, 14 Oct 2022 15:11:47 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Fri, 14 Oct 2022 15:37:22 +0300 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > > > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > > > Currently, every time the device wakes up from sleep, the > > iio_chan array is reallocated, leaking the previous one > > until the device is removed (basically never). > > > > Move the allocation to the probe function to avoid this. > > > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > Hi Cosmin, > > Please give a fixes tag for this one as we'll definitely want to > backport it. > > Reply to this patch is fine as b4 will pick it up like any other tag. Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983") (from direct mail) > > > --- > > drivers/iio/temperature/ltc2983.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c > > index b652d2b39bcf..a60ccf183687 100644 > > --- a/drivers/iio/temperature/ltc2983.c > > +++ b/drivers/iio/temperature/ltc2983.c > > @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) > > return ret; > > } > > > > - st->iio_chan = devm_kzalloc(&st->spi->dev, > > - st->iio_channels * sizeof(*st->iio_chan), > > - GFP_KERNEL); > > - > > - if (!st->iio_chan) > > - return -ENOMEM; > > - > > ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG, > > LTC2983_NOTCH_FREQ_MASK, > > LTC2983_NOTCH_FREQ(st->filter_notch_freq)); > > @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi) > > gpiod_set_value_cansleep(gpio, 0); > > } > > > > + st->iio_chan = devm_kzalloc(&spi->dev, > > + st->iio_channels * sizeof(*st->iio_chan), > > + GFP_KERNEL); > > + if (!st->iio_chan) > > + return -ENOMEM; > > + > > ret = ltc2983_setup(st, true); > > if (ret) > > return ret; > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once 2022-10-14 15:18 ` Jonathan Cameron @ 2022-10-15 16:35 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2022-10-15 16:35 UTC (permalink / raw) To: Jonathan Cameron Cc: Cosmin Tanislav, Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Fri, 14 Oct 2022 16:18:24 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Fri, 14 Oct 2022 15:11:47 +0100 > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > On Fri, 14 Oct 2022 15:37:22 +0300 > > Cosmin Tanislav <demonsingur@gmail.com> wrote: > > > > > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > > > > > Currently, every time the device wakes up from sleep, the > > > iio_chan array is reallocated, leaking the previous one > > > until the device is removed (basically never). > > > > > > Move the allocation to the probe function to avoid this. > > > > > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Hi Cosmin, > > > > Please give a fixes tag for this one as we'll definitely want to > > backport it. > > > > Reply to this patch is fine as b4 will pick it up like any other tag. > Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983") > > (from direct mail) > Applied to the fixes-togreg branch of iio.git. Thanks, Jonathan > > > > > --- > > > drivers/iio/temperature/ltc2983.c | 13 ++++++------- > > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c > > > index b652d2b39bcf..a60ccf183687 100644 > > > --- a/drivers/iio/temperature/ltc2983.c > > > +++ b/drivers/iio/temperature/ltc2983.c > > > @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) > > > return ret; > > > } > > > > > > - st->iio_chan = devm_kzalloc(&st->spi->dev, > > > - st->iio_channels * sizeof(*st->iio_chan), > > > - GFP_KERNEL); > > > - > > > - if (!st->iio_chan) > > > - return -ENOMEM; > > > - > > > ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG, > > > LTC2983_NOTCH_FREQ_MASK, > > > LTC2983_NOTCH_FREQ(st->filter_notch_freq)); > > > @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi) > > > gpiod_set_value_cansleep(gpio, 0); > > > } > > > > > > + st->iio_chan = devm_kzalloc(&spi->dev, > > > + st->iio_channels * sizeof(*st->iio_chan), > > > + GFP_KERNEL); > > > + if (!st->iio_chan) > > > + return -ENOMEM; > > > + > > > ret = ltc2983_setup(st, true); > > > if (ret) > > > return ret; > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-14 12:37 [PATCH 0/3] Support more parts in LTC2983 Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once Cosmin Tanislav @ 2022-10-14 12:37 ` Cosmin Tanislav 2022-10-14 15:37 ` Jonathan Cameron 2022-10-17 1:59 ` Krzysztof Kozlowski 2022-10-14 12:37 ` [PATCH 3/3] " Cosmin Tanislav 2 siblings, 2 replies; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-14 12:37 UTC (permalink / raw) Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav From: Cosmin Tanislav <cosmin.tanislav@analog.com> Add support for the following parts: * LTC2984 * LTC2986 * LTM2985 The LTC2984 is a variant of the LTC2983 with EEPROM. The LTC2986 is a variant of the LTC2983 with only 10 channels, EEPROM and support for active analog temperature sensors. The LTM2985 is software-compatible with the LTC2986. Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> --- .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml index 722781aa4697..c33ab524fb64 100644 --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml @@ -4,19 +4,27 @@ $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Analog Devices LTC2983 Multi-sensor Temperature system +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system maintainers: - Nuno Sá <nuno.sa@analog.com> description: | - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital + Temperature Measurement Systems + https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf properties: compatible: enum: - adi,ltc2983 + - adi,ltc2984 + - adi,ltc2986 + - adi,ltm2985 reg: maxItems: 1 @@ -26,7 +34,7 @@ properties: adi,mux-delay-config-us: description: - The LTC2983 performs 2 or 3 internal conversion cycles per temperature + The device performs 2 or 3 internal conversion cycles per temperature result. Each conversion cycle is performed with different excitation and input multiplexer configurations. Prior to each conversion, these excitation circuits and input switch configurations are changed and an @@ -145,7 +153,7 @@ patternProperties: adi,three-conversion-cycles: description: Boolean property which set's three conversion cycles removing - parasitic resistance effects between the LTC2983 and the diode. + parasitic resistance effects between the device and the diode. type: boolean adi,average-on: @@ -353,6 +361,41 @@ patternProperties: description: Boolean property which set's the adc as single-ended. type: boolean + "^temp@": + type: object + description: + Represents a channel which is being used as an active analog temperature + sensor. + + properties: + adi,sensor-type: + description: + Identifies the sensor as an active analog temperature sensor. + $ref: /schemas/types.yaml#/definitions/uint32 + const: 31 + + adi,single-ended: + description: Boolean property which sets the sensor as single-ended. + type: boolean + + adi,custom-temp: + description: + This is a table, where each entry should be a pair of + voltage(mv)-temperature(K). The entries must be given in nv and uK + so that, the original values must be multiplied by 1000000. For + more details look at table 71 and 72. + Note should be signed, but dtc doesn't currently maintain the + sign. + $ref: /schemas/types.yaml#/definitions/uint64-matrix + minItems: 3 + maxItems: 64 + items: + minItems: 2 + maxItems: 2 + + required: + - adi,custom-temp + "^rsense@": type: object description: @@ -382,6 +425,18 @@ required: - reg - interrupts +allOf: + - if: + properties: + compatible: + contains: + enum: + - adi,ltc2983 + - adi,ltc2984 + then: + patternProperties: + "^temp@": false + additionalProperties: false examples: -- 2.37.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-14 12:37 ` [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts Cosmin Tanislav @ 2022-10-14 15:37 ` Jonathan Cameron 2022-10-17 7:01 ` Cosmin Tanislav 2022-10-17 1:59 ` Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-10-14 15:37 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Fri, 14 Oct 2022 15:37:23 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Add support for the following parts: > * LTC2984 > * LTC2986 > * LTM2985 > > The LTC2984 is a variant of the LTC2983 with EEPROM. > The LTC2986 is a variant of the LTC2983 with only 10 channels, > EEPROM and support for active analog temperature sensors. If they 'work' but with fewer features, perhaps a fallback compatible? > The LTM2985 is software-compatible with the LTC2986. Fallback compatible? > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > --- > .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- > 1 file changed, 59 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > index 722781aa4697..c33ab524fb64 100644 > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > @@ -4,19 +4,27 @@ > $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Analog Devices LTC2983 Multi-sensor Temperature system > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system > > maintainers: > - Nuno Sá <nuno.sa@analog.com> > > description: | > - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System > + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital > + Temperature Measurement Systems > + > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > > properties: > compatible: > enum: > - adi,ltc2983 > + - adi,ltc2984 > + - adi,ltc2986 > + - adi,ltm2985 > > reg: > maxItems: 1 > @@ -26,7 +34,7 @@ properties: > > adi,mux-delay-config-us: > description: > - The LTC2983 performs 2 or 3 internal conversion cycles per temperature > + The device performs 2 or 3 internal conversion cycles per temperature Definitely a lesson here in avoiding device names in the descriptions! > result. Each conversion cycle is performed with different excitation and > input multiplexer configurations. Prior to each conversion, these > excitation circuits and input switch configurations are changed and an > @@ -145,7 +153,7 @@ patternProperties: > adi,three-conversion-cycles: > description: > Boolean property which set's three conversion cycles removing > - parasitic resistance effects between the LTC2983 and the diode. > + parasitic resistance effects between the device and the diode. > type: boolean > > adi,average-on: > @@ -353,6 +361,41 @@ patternProperties: > description: Boolean property which set's the adc as single-ended. > type: boolean > > + "^temp@": > + type: object > + description: > + Represents a channel which is being used as an active analog temperature > + sensor. > + > + properties: > + adi,sensor-type: > + description: > + Identifies the sensor as an active analog temperature sensor. > + $ref: /schemas/types.yaml#/definitions/uint32 > + const: 31 Ah. This is a bit odd as it's fixed for the channel type. However there is precedence in this binding so fair enough. > + > + adi,single-ended: > + description: Boolean property which sets the sensor as single-ended. > + type: boolean > + > + adi,custom-temp: > + description: > + This is a table, where each entry should be a pair of > + voltage(mv)-temperature(K). The entries must be given in nv and uK > + so that, the original values must be multiplied by 1000000. For > + more details look at table 71 and 72. > + Note should be signed, but dtc doesn't currently maintain the > + sign. > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > + minItems: 3 > + maxItems: 64 > + items: > + minItems: 2 > + maxItems: 2 > + > + required: > + - adi,custom-temp > + > "^rsense@": > type: object > description: > @@ -382,6 +425,18 @@ required: > - reg > - interrupts > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - adi,ltc2983 > + - adi,ltc2984 > + then: > + patternProperties: > + "^temp@": false > + > additionalProperties: false > > examples: ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-14 15:37 ` Jonathan Cameron @ 2022-10-17 7:01 ` Cosmin Tanislav 2022-10-17 10:22 ` Jonathan Cameron 0 siblings, 1 reply; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-17 7:01 UTC (permalink / raw) To: Jonathan Cameron Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 10/14/22 18:37, Jonathan Cameron wrote: > On Fri, 14 Oct 2022 15:37:23 +0300 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> >> >> Add support for the following parts: >> * LTC2984 >> * LTC2986 >> * LTM2985 >> >> The LTC2984 is a variant of the LTC2983 with EEPROM. >> The LTC2986 is a variant of the LTC2983 with only 10 channels, >> EEPROM and support for active analog temperature sensors. > > If they 'work' but with fewer features, perhaps a fallback > compatible? > 10 channels vs 20 channels. Using ltc2983 compatible as fallback would allow you to have 10 non-functional channels specified in the dts. >> The LTM2985 is software-compatible with the LTC2986. > > Fallback compatible? > >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >> --- >> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- >> 1 file changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> index 722781aa4697..c33ab524fb64 100644 >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> @@ -4,19 +4,27 @@ >> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> >> -title: Analog Devices LTC2983 Multi-sensor Temperature system >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system >> >> maintainers: >> - Nuno Sá <nuno.sa@analog.com> >> >> description: | >> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System >> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital >> + Temperature Measurement Systems >> + >> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf >> >> properties: >> compatible: >> enum: >> - adi,ltc2983 >> + - adi,ltc2984 >> + - adi,ltc2986 >> + - adi,ltm2985 >> >> reg: >> maxItems: 1 >> @@ -26,7 +34,7 @@ properties: >> >> adi,mux-delay-config-us: >> description: >> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature >> + The device performs 2 or 3 internal conversion cycles per temperature > > Definitely a lesson here in avoiding device names in the descriptions! > >> result. Each conversion cycle is performed with different excitation and >> input multiplexer configurations. Prior to each conversion, these >> excitation circuits and input switch configurations are changed and an >> @@ -145,7 +153,7 @@ patternProperties: >> adi,three-conversion-cycles: >> description: >> Boolean property which set's three conversion cycles removing >> - parasitic resistance effects between the LTC2983 and the diode. >> + parasitic resistance effects between the device and the diode. >> type: boolean >> >> adi,average-on: >> @@ -353,6 +361,41 @@ patternProperties: >> description: Boolean property which set's the adc as single-ended. >> type: boolean >> >> + "^temp@": >> + type: object >> + description: >> + Represents a channel which is being used as an active analog temperature >> + sensor. >> + >> + properties: >> + adi,sensor-type: >> + description: >> + Identifies the sensor as an active analog temperature sensor. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + const: 31 > > Ah. This is a bit odd as it's fixed for the channel type. However there > is precedence in this binding so fair enough. > I think it >> + >> + adi,single-ended: >> + description: Boolean property which sets the sensor as single-ended. >> + type: boolean >> + >> + adi,custom-temp: >> + description: >> + This is a table, where each entry should be a pair of >> + voltage(mv)-temperature(K). The entries must be given in nv and uK >> + so that, the original values must be multiplied by 1000000. For >> + more details look at table 71 and 72. >> + Note should be signed, but dtc doesn't currently maintain the >> + sign. >> + $ref: /schemas/types.yaml#/definitions/uint64-matrix >> + minItems: 3 >> + maxItems: 64 >> + items: >> + minItems: 2 >> + maxItems: 2 >> + >> + required: >> + - adi,custom-temp >> + >> "^rsense@": >> type: object >> description: >> @@ -382,6 +425,18 @@ required: >> - reg >> - interrupts >> >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - adi,ltc2983 >> + - adi,ltc2984 >> + then: >> + patternProperties: >> + "^temp@": false >> + >> additionalProperties: false >> >> examples: > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 7:01 ` Cosmin Tanislav @ 2022-10-17 10:22 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2022-10-17 10:22 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 17 Oct 2022 10:01:03 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > On 10/14/22 18:37, Jonathan Cameron wrote: > > On Fri, 14 Oct 2022 15:37:23 +0300 > > Cosmin Tanislav <demonsingur@gmail.com> wrote: > > > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> > >> > >> Add support for the following parts: > >> * LTC2984 > >> * LTC2986 > >> * LTM2985 > >> > >> The LTC2984 is a variant of the LTC2983 with EEPROM. > >> The LTC2986 is a variant of the LTC2983 with only 10 channels, > >> EEPROM and support for active analog temperature sensors. > > > > If they 'work' but with fewer features, perhaps a fallback > > compatible? > > > > 10 channels vs 20 channels. Using ltc2983 compatible as fallback > would allow you to have 10 non-functional channels specified in the > dts. Ah. That one probably doesn't make sense then. The 2984? > > >> The LTM2985 is software-compatible with the LTC2986. > > > > Fallback compatible? > > > >> > >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > >> --- > >> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- > >> 1 file changed, 59 insertions(+), 4 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> index 722781aa4697..c33ab524fb64 100644 > >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> @@ -4,19 +4,27 @@ > >> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > >> $schema: http://devicetree.org/meta-schemas/core.yaml# > >> > >> -title: Analog Devices LTC2983 Multi-sensor Temperature system > >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system > >> > >> maintainers: > >> - Nuno Sá <nuno.sa@analog.com> > >> > >> description: | > >> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System > >> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital > >> + Temperature Measurement Systems > >> + > >> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > >> > >> properties: > >> compatible: > >> enum: > >> - adi,ltc2983 > >> + - adi,ltc2984 > >> + - adi,ltc2986 > >> + - adi,ltm2985 > >> > >> reg: > >> maxItems: 1 > >> @@ -26,7 +34,7 @@ properties: > >> > >> adi,mux-delay-config-us: > >> description: > >> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature > >> + The device performs 2 or 3 internal conversion cycles per temperature > > > > Definitely a lesson here in avoiding device names in the descriptions! > > > >> result. Each conversion cycle is performed with different excitation and > >> input multiplexer configurations. Prior to each conversion, these > >> excitation circuits and input switch configurations are changed and an > >> @@ -145,7 +153,7 @@ patternProperties: > >> adi,three-conversion-cycles: > >> description: > >> Boolean property which set's three conversion cycles removing > >> - parasitic resistance effects between the LTC2983 and the diode. > >> + parasitic resistance effects between the device and the diode. > >> type: boolean > >> > >> adi,average-on: > >> @@ -353,6 +361,41 @@ patternProperties: > >> description: Boolean property which set's the adc as single-ended. > >> type: boolean > >> > >> + "^temp@": > >> + type: object > >> + description: > >> + Represents a channel which is being used as an active analog temperature > >> + sensor. > >> + > >> + properties: > >> + adi,sensor-type: > >> + description: > >> + Identifies the sensor as an active analog temperature sensor. > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + const: 31 > > > > Ah. This is a bit odd as it's fixed for the channel type. However there > > is precedence in this binding so fair enough. > > > > I think it > > >> + > >> + adi,single-ended: > >> + description: Boolean property which sets the sensor as single-ended. > >> + type: boolean > >> + > >> + adi,custom-temp: > >> + description: > >> + This is a table, where each entry should be a pair of > >> + voltage(mv)-temperature(K). The entries must be given in nv and uK > >> + so that, the original values must be multiplied by 1000000. For > >> + more details look at table 71 and 72. > >> + Note should be signed, but dtc doesn't currently maintain the > >> + sign. > >> + $ref: /schemas/types.yaml#/definitions/uint64-matrix > >> + minItems: 3 > >> + maxItems: 64 > >> + items: > >> + minItems: 2 > >> + maxItems: 2 > >> + > >> + required: > >> + - adi,custom-temp > >> + > >> "^rsense@": > >> type: object > >> description: > >> @@ -382,6 +425,18 @@ required: > >> - reg > >> - interrupts > >> > >> +allOf: > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - adi,ltc2983 > >> + - adi,ltc2984 > >> + then: > >> + patternProperties: > >> + "^temp@": false > >> + > >> additionalProperties: false > >> > >> examples: > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-14 12:37 ` [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts Cosmin Tanislav 2022-10-14 15:37 ` Jonathan Cameron @ 2022-10-17 1:59 ` Krzysztof Kozlowski 2022-10-17 6:53 ` Cosmin Tanislav 2022-10-17 9:38 ` Nuno Sá 1 sibling, 2 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2022-10-17 1:59 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 14/10/2022 08:37, Cosmin Tanislav wrote: > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Add support for the following parts: > * LTC2984 > * LTC2986 > * LTM2985 > > The LTC2984 is a variant of the LTC2983 with EEPROM. > The LTC2986 is a variant of the LTC2983 with only 10 channels, > EEPROM and support for active analog temperature sensors. > The LTM2985 is software-compatible with the LTC2986. > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > --- > .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- > 1 file changed, 59 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > index 722781aa4697..c33ab524fb64 100644 > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > @@ -4,19 +4,27 @@ > $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Analog Devices LTC2983 Multi-sensor Temperature system > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system > > maintainers: > - Nuno Sá <nuno.sa@analog.com> > > description: | > - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System > + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital > + Temperature Measurement Systems > + > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > > properties: > compatible: > enum: > - adi,ltc2983 > + - adi,ltc2984 > + - adi,ltc2986 > + - adi,ltm2985 > > reg: > maxItems: 1 > @@ -26,7 +34,7 @@ properties: > > adi,mux-delay-config-us: > description: > - The LTC2983 performs 2 or 3 internal conversion cycles per temperature > + The device performs 2 or 3 internal conversion cycles per temperature > result. Each conversion cycle is performed with different excitation and > input multiplexer configurations. Prior to each conversion, these > excitation circuits and input switch configurations are changed and an > @@ -145,7 +153,7 @@ patternProperties: > adi,three-conversion-cycles: > description: > Boolean property which set's three conversion cycles removing > - parasitic resistance effects between the LTC2983 and the diode. > + parasitic resistance effects between the device and the diode. > type: boolean > > adi,average-on: > @@ -353,6 +361,41 @@ patternProperties: > description: Boolean property which set's the adc as single-ended. > type: boolean > > + "^temp@": There is already a property for thermocouple. Isn't a thermocouple a temperature sensor? IOW, why new property is needed? > + type: object > + description: > + Represents a channel which is being used as an active analog temperature > + sensor. > + > + properties: > + adi,sensor-type: > + description: > + Identifies the sensor as an active analog temperature sensor. > + $ref: /schemas/types.yaml#/definitions/uint32 > + const: 31 > + > + adi,single-ended: > + description: Boolean property which sets the sensor as single-ended. Drop "Boolean property which sets" - it's obvious from the type. > + type: boolean > + > + adi,custom-temp: > + description: > + This is a table, where each entry should be a pair of "This is a table" - obvious from the type. > + voltage(mv)-temperature(K). The entries must be given in nv and uK mv-K or nv-uK? Confusing... > + so that, the original values must be multiplied by 1000000. For > + more details look at table 71 and 72. There is no table 71 in the bindings... It seems you pasted it from somewhere. > + Note should be signed, but dtc doesn't currently maintain the > + sign. What do you mean? "Maintain" as allow or keep when building FDT? What's the problem of using negative numbers here and why it should be part of bindings? > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > + minItems: 3 > + maxItems: 64 > + items: > + minItems: 2 > + maxItems: 2 Instead describe the items with "description" (and maybe constraints) like here: https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > + > + required: > + - adi,custom-temp > + > "^rsense@": > type: object > description: > @@ -382,6 +425,18 @@ required: > - reg > - interrupts > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - adi,ltc2983 > + - adi,ltc2984 > + then: > + patternProperties: > + "^temp@": false > + > additionalProperties: false > > examples: Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 1:59 ` Krzysztof Kozlowski @ 2022-10-17 6:53 ` Cosmin Tanislav 2022-10-17 10:26 ` Jonathan Cameron 2022-10-17 23:26 ` Krzysztof Kozlowski 2022-10-17 9:38 ` Nuno Sá 1 sibling, 2 replies; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-17 6:53 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 10/17/22 04:59, Krzysztof Kozlowski wrote: > On 14/10/2022 08:37, Cosmin Tanislav wrote: >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> >> >> Add support for the following parts: >> * LTC2984 >> * LTC2986 >> * LTM2985 >> >> The LTC2984 is a variant of the LTC2983 with EEPROM. >> The LTC2986 is a variant of the LTC2983 with only 10 channels, >> EEPROM and support for active analog temperature sensors. >> The LTM2985 is software-compatible with the LTC2986. >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >> --- >> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- >> 1 file changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> index 722781aa4697..c33ab524fb64 100644 >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >> @@ -4,19 +4,27 @@ >> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> >> -title: Analog Devices LTC2983 Multi-sensor Temperature system >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system >> >> maintainers: >> - Nuno Sá <nuno.sa@analog.com> >> >> description: | >> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System >> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital >> + Temperature Measurement Systems >> + >> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf >> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf >> >> properties: >> compatible: >> enum: >> - adi,ltc2983 >> + - adi,ltc2984 >> + - adi,ltc2986 >> + - adi,ltm2985 >> >> reg: >> maxItems: 1 >> @@ -26,7 +34,7 @@ properties: >> >> adi,mux-delay-config-us: >> description: >> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature >> + The device performs 2 or 3 internal conversion cycles per temperature >> result. Each conversion cycle is performed with different excitation and >> input multiplexer configurations. Prior to each conversion, these >> excitation circuits and input switch configurations are changed and an >> @@ -145,7 +153,7 @@ patternProperties: >> adi,three-conversion-cycles: >> description: >> Boolean property which set's three conversion cycles removing >> - parasitic resistance effects between the LTC2983 and the diode. >> + parasitic resistance effects between the device and the diode. >> type: boolean >> >> adi,average-on: >> @@ -353,6 +361,41 @@ patternProperties: >> description: Boolean property which set's the adc as single-ended. >> type: boolean >> >> + "^temp@": > > There is already a property for thermocouple. Isn't a thermocouple a > temperature sensor? IOW, why new property is needed? > This node is needed for active analog temperature sensors. It has fewer options than the thermocouple, as it only supports a table to map from voltage to temperature and specifying whether the measurement is differential or single-ended. If you did as much as glimpsed at the datasheet you would have understood. >> + type: object >> + description: >> + Represents a channel which is being used as an active analog temperature >> + sensor. >> + >> + properties: >> + adi,sensor-type: >> + description: >> + Identifies the sensor as an active analog temperature sensor. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + const: 31 >> + >> + adi,single-ended: >> + description: Boolean property which sets the sensor as single-ended. > > Drop "Boolean property which sets" - it's obvious from the type. > That's how the rest of the file is written. > > >> + type: boolean >> + >> + adi,custom-temp: >> + description: >> + This is a table, where each entry should be a pair of > > "This is a table" - obvious from the type. > That's how the rest of the file is written. >> + voltage(mv)-temperature(K). The entries must be given in nv and uK > > mv-K or nv-uK? Confusing... That's how the rest of the file is written. The chip uses mv-K, but the binding specifies nv-uK, the driver translates it into the appropriate unit. > >> + so that, the original values must be multiplied by 1000000. For >> + more details look at table 71 and 72. > > There is no table 71 in the bindings... It seems you pasted it from > somewhere. > It's pretty obvious that "Table" in a binding refers to the datasheet. But if you meant datasheet, not binding, you're looking at the wrong datasheet then. Also, that's how the rest of the file is written. >> + Note should be signed, but dtc doesn't currently maintain the >> + sign. > > What do you mean? "Maintain" as allow or keep when building FDT? What's > the problem of using negative numbers here and why it should be part of > bindings? > You're really clueless, I'll let you figure it out on your own. Also, that's how the rest of the file is written. >> + $ref: /schemas/types.yaml#/definitions/uint64-matrix >> + minItems: 3 >> + maxItems: 64 >> + items: >> + minItems: 2 >> + maxItems: 2 > > Instead describe the items with "description" (and maybe constraints) > like here: > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > That's how the rest of the file is written. If you really want to use something different, you can submit a patch later and fix the whole binding however you want. >> + >> + required: >> + - adi,custom-temp >> + >> "^rsense@": >> type: object >> description: >> @@ -382,6 +425,18 @@ required: >> - reg >> - interrupts >> >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - adi,ltc2983 >> + - adi,ltc2984 >> + then: >> + patternProperties: >> + "^temp@": false >> + >> additionalProperties: false >> >> examples: > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 6:53 ` Cosmin Tanislav @ 2022-10-17 10:26 ` Jonathan Cameron 2022-10-17 10:37 ` Jonathan Cameron 2022-10-17 23:26 ` Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-10-17 10:26 UTC (permalink / raw) To: Cosmin Tanislav Cc: Krzysztof Kozlowski, Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 17 Oct 2022 09:53:40 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > On 10/17/22 04:59, Krzysztof Kozlowski wrote: > > On 14/10/2022 08:37, Cosmin Tanislav wrote: > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> > >> > >> Add support for the following parts: > >> * LTC2984 > >> * LTC2986 > >> * LTM2985 > >> > >> The LTC2984 is a variant of the LTC2983 with EEPROM. > >> The LTC2986 is a variant of the LTC2983 with only 10 channels, > >> EEPROM and support for active analog temperature sensors. > >> The LTM2985 is software-compatible with the LTC2986. > >> > >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> Cosmin, Please take care to keep things polite. Krzysztof is reviewing an enormous number of bindings and is very helpful indeed. We should respect his feedback and respond appropriately. Sometimes what is 'obvious' to one person may need a clarifying comment or explanation for another. Jonathan > >> --- > >> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- > >> 1 file changed, 59 insertions(+), 4 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> index 722781aa4697..c33ab524fb64 100644 > >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > >> @@ -4,19 +4,27 @@ > >> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > >> $schema: http://devicetree.org/meta-schemas/core.yaml# > >> > >> -title: Analog Devices LTC2983 Multi-sensor Temperature system > >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system > >> > >> maintainers: > >> - Nuno Sá <nuno.sa@analog.com> > >> > >> description: | > >> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System > >> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital > >> + Temperature Measurement Systems > >> + > >> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > >> > >> properties: > >> compatible: > >> enum: > >> - adi,ltc2983 > >> + - adi,ltc2984 > >> + - adi,ltc2986 > >> + - adi,ltm2985 > >> > >> reg: > >> maxItems: 1 > >> @@ -26,7 +34,7 @@ properties: > >> > >> adi,mux-delay-config-us: > >> description: > >> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature > >> + The device performs 2 or 3 internal conversion cycles per temperature > >> result. Each conversion cycle is performed with different excitation and > >> input multiplexer configurations. Prior to each conversion, these > >> excitation circuits and input switch configurations are changed and an > >> @@ -145,7 +153,7 @@ patternProperties: > >> adi,three-conversion-cycles: > >> description: > >> Boolean property which set's three conversion cycles removing > >> - parasitic resistance effects between the LTC2983 and the diode. > >> + parasitic resistance effects between the device and the diode. > >> type: boolean > >> > >> adi,average-on: > >> @@ -353,6 +361,41 @@ patternProperties: > >> description: Boolean property which set's the adc as single-ended. > >> type: boolean > >> > >> + "^temp@": > > > > There is already a property for thermocouple. Isn't a thermocouple a > > temperature sensor? IOW, why new property is needed? > > > This node is needed for active analog temperature sensors. > It has fewer options than the thermocouple, as it only supports > a table to map from voltage to temperature and specifying whether > the measurement is differential or single-ended. > > If you did as much as glimpsed at the datasheet you would have > understood. > > >> + type: object > >> + description: > >> + Represents a channel which is being used as an active analog temperature > >> + sensor. > >> + > >> + properties: > >> + adi,sensor-type: > >> + description: > >> + Identifies the sensor as an active analog temperature sensor. > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + const: 31 > >> + > >> + adi,single-ended: > >> + description: Boolean property which sets the sensor as single-ended. > > > > Drop "Boolean property which sets" - it's obvious from the type. > > > > That's how the rest of the file is written. > > > > > > >> + type: boolean > >> + > >> + adi,custom-temp: > >> + description: > >> + This is a table, where each entry should be a pair of > > > > "This is a table" - obvious from the type. > > > > That's how the rest of the file is written. > > >> + voltage(mv)-temperature(K). The entries must be given in nv and uK > > > > mv-K or nv-uK? Confusing... > > That's how the rest of the file is written. > > The chip uses mv-K, but the binding specifies nv-uK, the driver > translates it into the appropriate unit. > > > > >> + so that, the original values must be multiplied by 1000000. For > >> + more details look at table 71 and 72. > > > > There is no table 71 in the bindings... It seems you pasted it from > > somewhere. > > > > It's pretty obvious that "Table" in a binding refers to the datasheet. > But if you meant datasheet, not binding, you're looking at the wrong > datasheet then. > Also, that's how the rest of the file is written. > > >> + Note should be signed, but dtc doesn't currently maintain the > >> + sign. > > > > What do you mean? "Maintain" as allow or keep when building FDT? What's > > the problem of using negative numbers here and why it should be part of > > bindings? > > > > You're really clueless, I'll let you figure it out on your own. > Also, that's how the rest of the file is written. > > >> + $ref: /schemas/types.yaml#/definitions/uint64-matrix > >> + minItems: 3 > >> + maxItems: 64 > >> + items: > >> + minItems: 2 > >> + maxItems: 2 > > > > Instead describe the items with "description" (and maybe constraints) > > like here: > > > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > > > > That's how the rest of the file is written. > If you really want to use something different, you can submit a > patch later and fix the whole binding however you want. > > >> + > >> + required: > >> + - adi,custom-temp > >> + > >> "^rsense@": > >> type: object > >> description: > >> @@ -382,6 +425,18 @@ required: > >> - reg > >> - interrupts > >> > >> +allOf: > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - adi,ltc2983 > >> + - adi,ltc2984 > >> + then: > >> + patternProperties: > >> + "^temp@": false > >> + > >> additionalProperties: false > >> > >> examples: > > > > Best regards, > > Krzysztof > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 10:26 ` Jonathan Cameron @ 2022-10-17 10:37 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2022-10-17 10:37 UTC (permalink / raw) To: Cosmin Tanislav Cc: Krzysztof Kozlowski, Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 17 Oct 2022 11:26:56 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 17 Oct 2022 09:53:40 +0300 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > > > On 10/17/22 04:59, Krzysztof Kozlowski wrote: > > > On 14/10/2022 08:37, Cosmin Tanislav wrote: > > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > >> > > >> Add support for the following parts: > > >> * LTC2984 > > >> * LTC2986 > > >> * LTM2985 > > >> > > >> The LTC2984 is a variant of the LTC2983 with EEPROM. > > >> The LTC2986 is a variant of the LTC2983 with only 10 channels, > > >> EEPROM and support for active analog temperature sensors. > > >> The LTM2985 is software-compatible with the LTC2986. > > >> > > >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Cosmin, > > Please take care to keep things polite. Krzysztof is reviewing an > enormous number of bindings and is very helpful indeed. We should > respect his feedback and respond appropriately. Sometimes what is > 'obvious' to one person may need a clarifying comment or explanation > for another. Note I should have said we should respect ALL reviewers for taking the time to look at our code. Not just the particularly active ones! Jonathan > > Jonathan > > > > >> --- > > >> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- > > >> 1 file changed, 59 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > >> index 722781aa4697..c33ab524fb64 100644 > > >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > >> @@ -4,19 +4,27 @@ > > >> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > > >> $schema: http://devicetree.org/meta-schemas/core.yaml# > > >> > > >> -title: Analog Devices LTC2983 Multi-sensor Temperature system > > >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system > > >> > > >> maintainers: > > >> - Nuno Sá <nuno.sa@analog.com> > > >> > > >> description: | > > >> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System > > >> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital > > >> + Temperature Measurement Systems > > >> + > > >> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > > >> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > > >> > > >> properties: > > >> compatible: > > >> enum: > > >> - adi,ltc2983 > > >> + - adi,ltc2984 > > >> + - adi,ltc2986 > > >> + - adi,ltm2985 > > >> > > >> reg: > > >> maxItems: 1 > > >> @@ -26,7 +34,7 @@ properties: > > >> > > >> adi,mux-delay-config-us: > > >> description: > > >> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature > > >> + The device performs 2 or 3 internal conversion cycles per temperature > > >> result. Each conversion cycle is performed with different excitation and > > >> input multiplexer configurations. Prior to each conversion, these > > >> excitation circuits and input switch configurations are changed and an > > >> @@ -145,7 +153,7 @@ patternProperties: > > >> adi,three-conversion-cycles: > > >> description: > > >> Boolean property which set's three conversion cycles removing > > >> - parasitic resistance effects between the LTC2983 and the diode. > > >> + parasitic resistance effects between the device and the diode. > > >> type: boolean > > >> > > >> adi,average-on: > > >> @@ -353,6 +361,41 @@ patternProperties: > > >> description: Boolean property which set's the adc as single-ended. > > >> type: boolean > > >> > > >> + "^temp@": > > > > > > There is already a property for thermocouple. Isn't a thermocouple a > > > temperature sensor? IOW, why new property is needed? > > > > > This node is needed for active analog temperature sensors. > > It has fewer options than the thermocouple, as it only supports > > a table to map from voltage to temperature and specifying whether > > the measurement is differential or single-ended. > > > > If you did as much as glimpsed at the datasheet you would have > > understood. > > > > >> + type: object > > >> + description: > > >> + Represents a channel which is being used as an active analog temperature > > >> + sensor. > > >> + > > >> + properties: > > >> + adi,sensor-type: > > >> + description: > > >> + Identifies the sensor as an active analog temperature sensor. > > >> + $ref: /schemas/types.yaml#/definitions/uint32 > > >> + const: 31 > > >> + > > >> + adi,single-ended: > > >> + description: Boolean property which sets the sensor as single-ended. > > > > > > Drop "Boolean property which sets" - it's obvious from the type. > > > > > > > That's how the rest of the file is written. > > > > > > > > > > >> + type: boolean > > >> + > > >> + adi,custom-temp: > > >> + description: > > >> + This is a table, where each entry should be a pair of > > > > > > "This is a table" - obvious from the type. > > > > > > > That's how the rest of the file is written. > > > > >> + voltage(mv)-temperature(K). The entries must be given in nv and uK > > > > > > mv-K or nv-uK? Confusing... > > > > That's how the rest of the file is written. > > > > The chip uses mv-K, but the binding specifies nv-uK, the driver > > translates it into the appropriate unit. > > > > > > > >> + so that, the original values must be multiplied by 1000000. For > > >> + more details look at table 71 and 72. > > > > > > There is no table 71 in the bindings... It seems you pasted it from > > > somewhere. > > > > > > > It's pretty obvious that "Table" in a binding refers to the datasheet. > > But if you meant datasheet, not binding, you're looking at the wrong > > datasheet then. > > Also, that's how the rest of the file is written. > > > > >> + Note should be signed, but dtc doesn't currently maintain the > > >> + sign. > > > > > > What do you mean? "Maintain" as allow or keep when building FDT? What's > > > the problem of using negative numbers here and why it should be part of > > > bindings? > > > > > > > You're really clueless, I'll let you figure it out on your own. > > Also, that's how the rest of the file is written. > > > > > >> + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > >> + minItems: 3 > > >> + maxItems: 64 > > >> + items: > > >> + minItems: 2 > > >> + maxItems: 2 > > > > > > Instead describe the items with "description" (and maybe constraints) > > > like here: > > > > > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > > > > > > > That's how the rest of the file is written. > > If you really want to use something different, you can submit a > > patch later and fix the whole binding however you want. > > > > >> + > > >> + required: > > >> + - adi,custom-temp > > >> + > > >> "^rsense@": > > >> type: object > > >> description: > > >> @@ -382,6 +425,18 @@ required: > > >> - reg > > >> - interrupts > > >> > > >> +allOf: > > >> + - if: > > >> + properties: > > >> + compatible: > > >> + contains: > > >> + enum: > > >> + - adi,ltc2983 > > >> + - adi,ltc2984 > > >> + then: > > >> + patternProperties: > > >> + "^temp@": false > > >> + > > >> additionalProperties: false > > >> > > >> examples: > > > > > > Best regards, > > > Krzysztof > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 6:53 ` Cosmin Tanislav 2022-10-17 10:26 ` Jonathan Cameron @ 2022-10-17 23:26 ` Krzysztof Kozlowski 1 sibling, 0 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2022-10-17 23:26 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 17/10/2022 02:53, Cosmin Tanislav wrote: > > > On 10/17/22 04:59, Krzysztof Kozlowski wrote: >> On 14/10/2022 08:37, Cosmin Tanislav wrote: >>> From: Cosmin Tanislav <cosmin.tanislav@analog.com> >>> >>> Add support for the following parts: >>> * LTC2984 >>> * LTC2986 >>> * LTM2985 >>> >>> The LTC2984 is a variant of the LTC2983 with EEPROM. >>> The LTC2986 is a variant of the LTC2983 with only 10 channels, >>> EEPROM and support for active analog temperature sensors. >>> The LTM2985 is software-compatible with the LTC2986. >>> >>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >>> --- >>> .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++-- >>> 1 file changed, 59 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >>> index 722781aa4697..c33ab524fb64 100644 >>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml >>> @@ -4,19 +4,27 @@ >>> $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# >>> $schema: http://devicetree.org/meta-schemas/core.yaml# >>> >>> -title: Analog Devices LTC2983 Multi-sensor Temperature system >>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system >>> >>> maintainers: >>> - Nuno Sá <nuno.sa@analog.com> >>> >>> description: | >>> - Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System >>> + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital >>> + Temperature Measurement Systems >>> + >>> https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf >>> + https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf >>> + https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf >>> + https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf >>> >>> properties: >>> compatible: >>> enum: >>> - adi,ltc2983 >>> + - adi,ltc2984 >>> + - adi,ltc2986 >>> + - adi,ltm2985 >>> >>> reg: >>> maxItems: 1 >>> @@ -26,7 +34,7 @@ properties: >>> >>> adi,mux-delay-config-us: >>> description: >>> - The LTC2983 performs 2 or 3 internal conversion cycles per temperature >>> + The device performs 2 or 3 internal conversion cycles per temperature >>> result. Each conversion cycle is performed with different excitation and >>> input multiplexer configurations. Prior to each conversion, these >>> excitation circuits and input switch configurations are changed and an >>> @@ -145,7 +153,7 @@ patternProperties: >>> adi,three-conversion-cycles: >>> description: >>> Boolean property which set's three conversion cycles removing >>> - parasitic resistance effects between the LTC2983 and the diode. >>> + parasitic resistance effects between the device and the diode. >>> type: boolean >>> >>> adi,average-on: >>> @@ -353,6 +361,41 @@ patternProperties: >>> description: Boolean property which set's the adc as single-ended. >>> type: boolean >>> >>> + "^temp@": >> >> There is already a property for thermocouple. Isn't a thermocouple a >> temperature sensor? IOW, why new property is needed? >> > This node is needed for active analog temperature sensors. > It has fewer options than the thermocouple, as it only supports > a table to map from voltage to temperature and specifying whether > the measurement is differential or single-ended. > > If you did as much as glimpsed at the datasheet you would have > understood. We receive a lot of bindings to review. If I glimpse through every datasheet, when would I work? Instead of expecting reviewer to dive into datasheets for this one particular sensor, make it explicit in commit msg. > >>> + type: object >>> + description: >>> + Represents a channel which is being used as an active analog temperature >>> + sensor. >>> + >>> + properties: >>> + adi,sensor-type: >>> + description: >>> + Identifies the sensor as an active analog temperature sensor. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + const: 31 >>> + >>> + adi,single-ended: >>> + description: Boolean property which sets the sensor as single-ended. >> >> Drop "Boolean property which sets" - it's obvious from the type. >> > > That's how the rest of the file is written. Not really an argument... You can correct the other pieces in separate patch. > >> >> >>> + type: boolean >>> + >>> + adi,custom-temp: >>> + description: >>> + This is a table, where each entry should be a pair of >> >> "This is a table" - obvious from the type. >> > > That's how the rest of the file is written. > >>> + voltage(mv)-temperature(K). The entries must be given in nv and uK >> >> mv-K or nv-uK? Confusing... > > That's how the rest of the file is written. The same. > > The chip uses mv-K, but the binding specifies nv-uK, the driver > translates it into the appropriate unit. It does not matter here, what the driver is doing. Use only one unit here, matching the DTS. > >> >>> + so that, the original values must be multiplied by 1000000. For >>> + more details look at table 71 and 72. >> >> There is no table 71 in the bindings... It seems you pasted it from >> somewhere. >> > > It's pretty obvious that "Table" in a binding refers to the datasheet. There are multiple datasheets and how would I know to which one this refers? > But if you meant datasheet, not binding, you're looking at the wrong > datasheet then. > Also, that's how the rest of the file is written. Not really an argument... Poor examples like to spread, it's an effort to drop them. > >>> + Note should be signed, but dtc doesn't currently maintain the >>> + sign. >> >> What do you mean? "Maintain" as allow or keep when building FDT? What's >> the problem of using negative numbers here and why it should be part of >> bindings? >> > > You're really clueless, I'll let you figure it out on your own. Yes, I am clueless and that's not the way how the review conversation should look like. NAK. > Also, that's how the rest of the file is written. > >>> + $ref: /schemas/types.yaml#/definitions/uint64-matrix >>> + minItems: 3 >>> + maxItems: 64 >>> + items: >>> + minItems: 2 >>> + maxItems: 2 >> >> Instead describe the items with "description" (and maybe constraints) >> like here: >> >> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 >> > > That's how the rest of the file is written. > If you really want to use something different, you can submit a > patch later and fix the whole binding however you want. Nope. I expect the new pieces to be correct, not incorrect because "there is already poor pattern, so I will do the same". If inconsistency bothers you, anyone can fix it in following up patch. Also you. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 1:59 ` Krzysztof Kozlowski 2022-10-17 6:53 ` Cosmin Tanislav @ 2022-10-17 9:38 ` Nuno Sá 2022-10-17 10:04 ` Nuno Sá 2022-10-17 23:32 ` Krzysztof Kozlowski 1 sibling, 2 replies; 22+ messages in thread From: Nuno Sá @ 2022-10-17 9:38 UTC (permalink / raw) To: Krzysztof Kozlowski, Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav Hi Krzysztof, As I wrote the original bindings, let me reply to some of your points... On Sun, 2022-10-16 at 21:59 -0400, Krzysztof Kozlowski wrote: > On 14/10/2022 08:37, Cosmin Tanislav wrote: > > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > > > Add support for the following parts: > > * LTC2984 > > * LTC2986 > > * LTM2985 > > > > The LTC2984 is a variant of the LTC2983 with EEPROM. > > The LTC2986 is a variant of the LTC2983 with only 10 channels, > > EEPROM and support for active analog temperature sensors. > > The LTM2985 is software-compatible with the LTC2986. > > > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > > --- > > .../bindings/iio/temperature/adi,ltc2983.yaml | 63 > > +++++++++++++++++-- > > 1 file changed, 59 insertions(+), 4 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > index 722781aa4697..c33ab524fb64 100644 > > --- > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > +++ > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > @@ -4,19 +4,27 @@ > > $id: > > http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Analog Devices LTC2983 Multi-sensor Temperature system > > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor > > Temperature system > > > > maintainers: > > - Nuno Sá <nuno.sa@analog.com> > > > > description: | > > - Analog Devices LTC2983 Multi-Sensor Digital Temperature > > Measurement System > > + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor > > Digital > > + Temperature Measurement Systems > > + > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > > > > properties: > > compatible: > > enum: > > - adi,ltc2983 > > + - adi,ltc2984 > > + - adi,ltc2986 > > + - adi,ltm2985 > > > > reg: > > maxItems: 1 > > @@ -26,7 +34,7 @@ properties: > > > > adi,mux-delay-config-us: > > description: > > - The LTC2983 performs 2 or 3 internal conversion cycles per > > temperature > > + The device performs 2 or 3 internal conversion cycles per > > temperature > > result. Each conversion cycle is performed with different > > excitation and > > input multiplexer configurations. Prior to each conversion, > > these > > excitation circuits and input switch configurations are > > changed and an > > @@ -145,7 +153,7 @@ patternProperties: > > adi,three-conversion-cycles: > > description: > > Boolean property which set's three conversion cycles > > removing > > - parasitic resistance effects between the LTC2983 and the > > diode. > > + parasitic resistance effects between the device and the > > diode. > > type: boolean > > > > adi,average-on: > > @@ -353,6 +361,41 @@ patternProperties: > > description: Boolean property which set's the adc as > > single-ended. > > type: boolean > > > > + "^temp@": > > There is already a property for thermocouple. Isn't a thermocouple a > temperature sensor? IOW, why new property is needed? > Well, most of the patternProps in this bindings are temperature sensors... It's just that the device(s) support different types of them. 'adi,sensor-type' is used to identify each sensor (as this translates in different configurations being written in the device channels). > > + type: object > > + description: > > + Represents a channel which is being used as an active analog > > temperature > > + sensor. > > + > > + properties: > > + adi,sensor-type: > > + description: > > + Identifies the sensor as an active analog temperature > > sensor. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + const: 31 > > + > > + adi,single-ended: > > + description: Boolean property which sets the sensor as > > single-ended. > > Drop "Boolean property which sets" - it's obvious from the type. > > > > > + type: boolean > > + > > + adi,custom-temp: > > + description: > > + This is a table, where each entry should be a pair of > > "This is a table" - obvious from the type. > > > + voltage(mv)-temperature(K). The entries must be given in > > nv and uK > > mv-K or nv-uK? Confusing... Yeah, a bit. In Cosmin defense, I think he's just keeping the same "style" as the rest of the properties... > > > + so that, the original values must be multiplied by > > 1000000. For > > + more details look at table 71 and 72. > > There is no table 71 in the bindings... It seems you pasted it from > somewhere. I'm fairly sure this refers to the datasheet. I see now that this can be confusing (again this kind of references are being (ab)used in the rest of the file). > > > + Note should be signed, but dtc doesn't currently > > maintain the > > + sign. > > What do you mean? "Maintain" as allow or keep when building FDT? > What's > the problem of using negative numbers here and why it should be part > of > bindings? > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > + minItems: 3 > > + maxItems: 64 > > + items: > > + minItems: 2 > > + maxItems: 2 > > Instead describe the items with "description" (and maybe constraints) > like here: > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > Neat... My only comment (which probably applies to my previous ones) is that the rest of the properties are already in this "style". So maybe, follow up patches with small clean-ups would be more appropriate? > - Nuno Sá ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 9:38 ` Nuno Sá @ 2022-10-17 10:04 ` Nuno Sá 2022-10-17 23:32 ` Krzysztof Kozlowski 1 sibling, 0 replies; 22+ messages in thread From: Nuno Sá @ 2022-10-17 10:04 UTC (permalink / raw) To: Krzysztof Kozlowski, Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 2022-10-17 at 11:38 +0200, Nuno Sá wrote: > Hi Krzysztof, > > As I wrote the original bindings, let me reply to some of your > points... > > Just realized now there were already some replies (forgot to 'lei up' before replying...) - Nuno Sá ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 9:38 ` Nuno Sá 2022-10-17 10:04 ` Nuno Sá @ 2022-10-17 23:32 ` Krzysztof Kozlowski 2022-10-18 6:01 ` Nuno Sá 1 sibling, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2022-10-17 23:32 UTC (permalink / raw) To: Nuno Sá, Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 17/10/2022 05:38, Nuno Sá wrote: > Hi Krzysztof, > (...) >>> @@ -353,6 +361,41 @@ patternProperties: >>> description: Boolean property which set's the adc as >>> single-ended. >>> type: boolean >>> >>> + "^temp@": >> >> There is already a property for thermocouple. Isn't a thermocouple a >> temperature sensor? IOW, why new property is needed? >> > > Well, most of the patternProps in this bindings are temperature > sensors... It's just that the device(s) support different types of > them. 'adi,sensor-type' is used to identify each sensor (as this > translates in different configurations being written in the device > channels). Sure. > >>> + type: object >>> + description: >>> + Represents a channel which is being used as an active analog >>> temperature >>> + sensor. >>> + >>> + properties: >>> + adi,sensor-type: >>> + description: >>> + Identifies the sensor as an active analog temperature >>> sensor. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + const: 31 >>> + >>> + adi,single-ended: >>> + description: Boolean property which sets the sensor as >>> single-ended. >> >> Drop "Boolean property which sets" - it's obvious from the type. >> >> >> >>> + type: boolean >>> + >>> + adi,custom-temp: >>> + description: >>> + This is a table, where each entry should be a pair of >> >> "This is a table" - obvious from the type. >> >>> + voltage(mv)-temperature(K). The entries must be given in >>> nv and uK >> >> mv-K or nv-uK? Confusing... > > Yeah, a bit. In Cosmin defense, I think he's just keeping the same > "style" as the rest of the properties... That's not the best approach for two reasons: 1. The unit used by hardware matters less here, because bindings are used to write DTS. In many, many other cases there will be some translation (just take random voltage regulator bindings). 2. What the driver is doing matters even less. So just describe here what is expected in DTS. > >> >>> + so that, the original values must be multiplied by >>> 1000000. For >>> + more details look at table 71 and 72. >> >> There is no table 71 in the bindings... It seems you pasted it from >> somewhere. > > I'm fairly sure this refers to the datasheet. I see now that this can > be confusing (again this kind of references are being (ab)used in the > rest of the file). Yep, but there are now multiple datasheets, aren't there? > >> >>> + Note should be signed, but dtc doesn't currently >>> maintain the >>> + sign. >> >> What do you mean? "Maintain" as allow or keep when building FDT? >> What's >> the problem of using negative numbers here and why it should be part >> of >> bindings? >> >>> + $ref: /schemas/types.yaml#/definitions/uint64-matrix >>> + minItems: 3 >>> + maxItems: 64 >>> + items: >>> + minItems: 2 >>> + maxItems: 2 >> >> Instead describe the items with "description" (and maybe constraints) >> like here: >> >> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 >> > > Neat... My only comment (which probably applies to my previous ones) is > that the rest of the properties are already in this "style". So maybe, > follow up patches with small clean-ups would be more appropriate? Of course. It would be great if the file was improved before or after this one. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts 2022-10-17 23:32 ` Krzysztof Kozlowski @ 2022-10-18 6:01 ` Nuno Sá 0 siblings, 0 replies; 22+ messages in thread From: Nuno Sá @ 2022-10-18 6:01 UTC (permalink / raw) To: Krzysztof Kozlowski, Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 2022-10-17 at 19:32 -0400, Krzysztof Kozlowski wrote: > On 17/10/2022 05:38, Nuno Sá wrote: > > Hi Krzysztof, > > > > (...) > > > > > @@ -353,6 +361,41 @@ patternProperties: > > > > description: Boolean property which set's the adc as > > > > single-ended. > > > > type: boolean > > > > > > > > + "^temp@": > > > > > > There is already a property for thermocouple. Isn't a > > > thermocouple a > > > temperature sensor? IOW, why new property is needed? > > > > > > > Well, most of the patternProps in this bindings are temperature > > sensors... It's just that the device(s) support different types of > > them. 'adi,sensor-type' is used to identify each sensor (as this > > translates in different configurations being written in the device > > channels). > > Sure. > > > > > > > + type: object > > > > + description: > > > > + Represents a channel which is being used as an active > > > > analog > > > > temperature > > > > + sensor. > > > > + > > > > + properties: > > > > + adi,sensor-type: > > > > + description: > > > > + Identifies the sensor as an active analog > > > > temperature > > > > sensor. > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + const: 31 > > > > + > > > > + adi,single-ended: > > > > + description: Boolean property which sets the sensor as > > > > single-ended. > > > > > > Drop "Boolean property which sets" - it's obvious from the type. > > > > > > > > > > > > > + type: boolean > > > > + > > > > + adi,custom-temp: > > > > + description: > > > > + This is a table, where each entry should be a pair > > > > of > > > > > > "This is a table" - obvious from the type. > > > > > > > + voltage(mv)-temperature(K). The entries must be > > > > given in > > > > nv and uK > > > > > > mv-K or nv-uK? Confusing... > > > > Yeah, a bit. In Cosmin defense, I think he's just keeping the same > > "style" as the rest of the properties... > > That's not the best approach for two reasons: > 1. The unit used by hardware matters less here, because bindings are > used to write DTS. In many, many other cases there will be some > translation (just take random voltage regulator bindings). > > 2. What the driver is doing matters even less. > > So just describe here what is expected in DTS. > Alright, I see. So we just refer to nv-uK as that is what I wanted for dts to expect (reason being to have more resolution). > > > > > > > > > + so that, the original values must be multiplied by > > > > 1000000. For > > > > + more details look at table 71 and 72. > > > > > > There is no table 71 in the bindings... It seems you pasted it > > > from > > > somewhere. > > > > I'm fairly sure this refers to the datasheet. I see now that this > > can > > be confusing (again this kind of references are being (ab)used in > > the > > rest of the file). > > Yep, but there are now multiple datasheets, aren't there? > Hmm yeah that's true. By the time I wrote this binding I was not even thinking on the possibility of new parts being added to it... I guess the lesson in here is to avoid this kind os specific descriptions. > > > > > > > > > + Note should be signed, but dtc doesn't currently > > > > maintain the > > > > + sign. > > > > > > What do you mean? "Maintain" as allow or keep when building FDT? > > > What's > > > the problem of using negative numbers here and why it should be > > > part > > > of > > > bindings? > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > + minItems: 3 > > > > + maxItems: 64 > > > > + items: > > > > + minItems: 2 > > > > + maxItems: 2 > > > > > > Instead describe the items with "description" (and maybe > > > constraints) > > > like here: > > > > > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > > > > > > > Neat... My only comment (which probably applies to my previous > > ones) is > > that the rest of the properties are already in this "style". So > > maybe, > > follow up patches with small clean-ups would be more appropriate? > > Of course. It would be great if the file was improved before or after > this one. > Ok, IMO it would make sense to have it in this series but if Cosmin does not feel like fixing my mess :), I'll send a separate patch with your inputs... - Nuno Sá ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] iio: temperature: ltc2983: support more parts 2022-10-14 12:37 [PATCH 0/3] Support more parts in LTC2983 Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts Cosmin Tanislav @ 2022-10-14 12:37 ` Cosmin Tanislav 2022-10-14 15:44 ` Jonathan Cameron 2 siblings, 1 reply; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-14 12:37 UTC (permalink / raw) Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav From: Cosmin Tanislav <cosmin.tanislav@analog.com> Add support for the following parts: * LTC2984 * LTC2986 * LTM2985 The LTC2984 is a variant of the LTC2983 with EEPROM. The LTC2986 is a variant of the LTC2983 with only 10 channels, EEPROM and support for active analog temperature sensors. The LTM2985 is software-compatible with the LTC2986. Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> --- drivers/iio/temperature/ltc2983.c | 182 ++++++++++++++++++++++++++++-- 1 file changed, 175 insertions(+), 7 deletions(-) diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c index a60ccf183687..22977bcdd2a3 100644 --- a/drivers/iio/temperature/ltc2983.c +++ b/drivers/iio/temperature/ltc2983.c @@ -25,9 +25,12 @@ #define LTC2983_STATUS_REG 0x0000 #define LTC2983_TEMP_RES_START_REG 0x0010 #define LTC2983_TEMP_RES_END_REG 0x005F +#define LTC2983_EEPROM_KEY_REG 0x00B0 +#define LTC2983_EEPROM_READ_STATUS_REG 0x00D0 #define LTC2983_GLOBAL_CONFIG_REG 0x00F0 #define LTC2983_MULT_CHANNEL_START_REG 0x00F4 #define LTC2983_MULT_CHANNEL_END_REG 0x00F7 +#define LTC2986_EEPROM_STATUS_REG 0x00F9 #define LTC2983_MUX_CONFIG_REG 0x00FF #define LTC2983_CHAN_ASSIGN_START_REG 0x0200 #define LTC2983_CHAN_ASSIGN_END_REG 0x024F @@ -35,13 +38,21 @@ #define LTC2983_CUST_SENS_TBL_END_REG 0x03CF #define LTC2983_DIFFERENTIAL_CHAN_MIN 2 -#define LTC2983_MAX_CHANNELS_NR 20 #define LTC2983_MIN_CHANNELS_NR 1 #define LTC2983_SLEEP 0x97 #define LTC2983_CUSTOM_STEINHART_SIZE 24 #define LTC2983_CUSTOM_SENSOR_ENTRY_SZ 6 #define LTC2983_CUSTOM_STEINHART_ENTRY_SZ 4 +#define LTC2983_EEPROM_KEY 0xA53C0F5A +#define LTC2983_EEPROM_WRITE_CMD 0x15 +#define LTC2983_EEPROM_READ_CMD 0x16 +#define LTC2983_EEPROM_STATUS_FAILURE_MASK GENMASK(3, 1) +#define LTC2983_EEPROM_READ_FAILURE_MASK GENMASK(7, 0) + +#define LTC2983_EEPROM_WRITE_TIME_MS 2600 +#define LTC2983_EEPROM_READ_TIME_MS 20 + #define LTC2983_CHAN_START_ADDR(chan) \ (((chan - 1) * 4) + LTC2983_CHAN_ASSIGN_START_REG) #define LTC2983_CHAN_RES_ADDR(chan) \ @@ -171,6 +182,7 @@ enum { LTC2983_SENSOR_DIODE = 28, LTC2983_SENSOR_SENSE_RESISTOR = 29, LTC2983_SENSOR_DIRECT_ADC = 30, + LTC2983_SENSOR_ACTIVE_TEMP = 31, }; #define to_thermocouple(_sensor) \ @@ -191,7 +203,17 @@ enum { #define to_adc(_sensor) \ container_of(_sensor, struct ltc2983_adc, sensor) +#define to_temp(_sensor) \ + container_of(_sensor, struct ltc2983_temp, sensor) + +struct ltc2983_chip_info { + unsigned int max_channels_nr; + bool has_temp; + bool has_eeprom; +}; + struct ltc2983_data { + const struct ltc2983_chip_info *info; struct regmap *regmap; struct spi_device *spi; struct mutex lock; @@ -271,6 +293,12 @@ struct ltc2983_adc { bool single_ended; }; +struct ltc2983_temp { + struct ltc2983_sensor sensor; + struct ltc2983_custom_sensor *custom; + bool single_ended; +}; + /* * Convert to Q format numbers. These number's are integers where * the number of integer and fractional bits are specified. The resolution @@ -606,6 +634,22 @@ static int ltc2983_adc_assign_chan(struct ltc2983_data *st, return __ltc2983_chan_assign_common(st, sensor, chan_val); } +static int ltc2983_temp_assign_chan(struct ltc2983_data *st, + const struct ltc2983_sensor *sensor) +{ + struct ltc2983_temp *temp = to_temp(sensor); + u32 chan_val; + int ret; + + chan_val = LTC2983_ADC_SINGLE_ENDED(temp->single_ended); + + ret = __ltc2983_chan_custom_sensor_assign(st, temp->custom, &chan_val); + if (ret) + return ret; + + return __ltc2983_chan_assign_common(st, sensor, chan_val); +} + static struct ltc2983_sensor * ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data *st, const struct ltc2983_sensor *sensor) @@ -771,10 +815,10 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st, if (rtd->sensor_config & LTC2983_RTD_4_WIRE_MASK) { /* 4-wire */ u8 min = LTC2983_DIFFERENTIAL_CHAN_MIN, - max = LTC2983_MAX_CHANNELS_NR; + max = st->info->max_channels_nr; if (rtd->sensor_config & LTC2983_RTD_ROTATION_MASK) - max = LTC2983_MAX_CHANNELS_NR - 1; + max = st->info->max_channels_nr - 1; if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK) == LTC2983_RTD_KELVIN_R_SENSE_MASK) && @@ -1143,6 +1187,38 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child, return &adc->sensor; } +static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child, + struct ltc2983_data *st, + const struct ltc2983_sensor *sensor) +{ + struct ltc2983_temp *temp; + + temp = devm_kzalloc(&st->spi->dev, sizeof(*temp), GFP_KERNEL); + if (!temp) + return ERR_PTR(-ENOMEM); + + if (fwnode_property_read_bool(child, "adi,single-ended")) + temp->single_ended = true; + + if (!temp->single_ended && + sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) { + dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n", + sensor->chan); + return ERR_PTR(-EINVAL); + } + + temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp", + false, 4096, true); + if (IS_ERR(temp->custom)) + return ERR_CAST(temp->custom); + + /* set common parameters */ + temp->sensor.assign_chan = ltc2983_temp_assign_chan; + temp->sensor.fault_handler = ltc2983_common_fault_handler; + + return &temp->sensor; +} + static int ltc2983_chan_read(struct ltc2983_data *st, const struct ltc2983_sensor *sensor, int *val) { @@ -1302,10 +1378,10 @@ static int ltc2983_parse_dt(struct ltc2983_data *st) /* check if we have a valid channel */ if (sensor.chan < LTC2983_MIN_CHANNELS_NR || - sensor.chan > LTC2983_MAX_CHANNELS_NR) { + sensor.chan > st->info->max_channels_nr) { ret = -EINVAL; dev_err(dev, "chan:%d must be from %u to %u\n", sensor.chan, - LTC2983_MIN_CHANNELS_NR, LTC2983_MAX_CHANNELS_NR); + LTC2983_MIN_CHANNELS_NR, st->info->max_channels_nr); goto put_child; } else if (channel_avail_mask & BIT(sensor.chan)) { ret = -EINVAL; @@ -1345,6 +1421,9 @@ static int ltc2983_parse_dt(struct ltc2983_data *st) st->iio_channels--; } else if (sensor.type == LTC2983_SENSOR_DIRECT_ADC) { st->sensors[chan] = ltc2983_adc_new(child, st, &sensor); + } else if (st->info->has_temp && + sensor.type == LTC2983_SENSOR_ACTIVE_TEMP) { + st->sensors[chan] = ltc2983_temp_new(child, st, &sensor); } else { dev_err(dev, "Unknown sensor type %d\n", sensor.type); ret = -EINVAL; @@ -1371,6 +1450,46 @@ static int ltc2983_parse_dt(struct ltc2983_data *st) return ret; } +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd, + unsigned int wait_time, unsigned int status_reg, + unsigned long status_fail_mask) +{ + __be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY); + unsigned long time; + unsigned int val; + int ret; + + ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval, + sizeof(bval)); + if (ret) + return ret; + + reinit_completion(&st->completion); + + ret = regmap_write(st->regmap, LTC2983_STATUS_REG, + LTC2983_STATUS_START(true) | cmd); + if (ret) + return ret; + + time = wait_for_completion_timeout(&st->completion, + msecs_to_jiffies(wait_time)); + if (!time) { + dev_err(&st->spi->dev, "EEPROM command timed out\n"); + return -ETIMEDOUT; + } + + ret = regmap_read(st->regmap, status_reg, &val); + if (ret) + return ret; + + if (val & status_fail_mask) { + dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val); + return -EINVAL; + } + + return 0; +} + static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) { u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status; @@ -1396,6 +1515,15 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) if (ret) return ret; + if (st->info->has_eeprom && !assign_iio) { + ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_READ_CMD, + LTC2983_EEPROM_READ_TIME_MS, + LTC2983_EEPROM_READ_STATUS_REG, + LTC2983_EEPROM_READ_FAILURE_MASK); + if (!ret) + return 0; + } + for (chan = 0; chan < st->num_channels; chan++) { u32 chan_type = 0, *iio_chan; @@ -1435,9 +1563,13 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio) static const struct regmap_range ltc2983_reg_ranges[] = { regmap_reg_range(LTC2983_STATUS_REG, LTC2983_STATUS_REG), regmap_reg_range(LTC2983_TEMP_RES_START_REG, LTC2983_TEMP_RES_END_REG), + regmap_reg_range(LTC2983_EEPROM_KEY_REG, LTC2983_EEPROM_KEY_REG), + regmap_reg_range(LTC2983_EEPROM_READ_STATUS_REG, + LTC2983_EEPROM_READ_STATUS_REG), regmap_reg_range(LTC2983_GLOBAL_CONFIG_REG, LTC2983_GLOBAL_CONFIG_REG), regmap_reg_range(LTC2983_MULT_CHANNEL_START_REG, LTC2983_MULT_CHANNEL_END_REG), + regmap_reg_range(LTC2986_EEPROM_STATUS_REG, LTC2986_EEPROM_STATUS_REG), regmap_reg_range(LTC2983_MUX_CONFIG_REG, LTC2983_MUX_CONFIG_REG), regmap_reg_range(LTC2983_CHAN_ASSIGN_START_REG, LTC2983_CHAN_ASSIGN_END_REG), @@ -1482,6 +1614,12 @@ static int ltc2983_probe(struct spi_device *spi) st = iio_priv(indio_dev); + st->info = device_get_match_data(&spi->dev); + if (!st->info) + st->info = (void *)spi_get_device_id(spi)->driver_data; + if (!st->info) + return -ENODEV; + st->regmap = devm_regmap_init_spi(spi, <c2983_regmap_config); if (IS_ERR(st->regmap)) { dev_err(&spi->dev, "Failed to initialize regmap\n"); @@ -1524,6 +1662,15 @@ static int ltc2983_probe(struct spi_device *spi) return ret; } + if (st->info->has_eeprom) { + ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD, + LTC2983_EEPROM_WRITE_TIME_MS, + LTC2986_EEPROM_STATUS_REG, + LTC2983_EEPROM_STATUS_FAILURE_MASK); + if (ret) + return ret; + } + indio_dev->name = name; indio_dev->num_channels = st->iio_channels; indio_dev->channels = st->iio_chan; @@ -1554,14 +1701,35 @@ static int ltc2983_suspend(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(ltc2983_pm_ops, ltc2983_suspend, ltc2983_resume); +static const struct ltc2983_chip_info ltc2983_chip_info_data = { + .max_channels_nr = 20, +}; + +static const struct ltc2983_chip_info ltc2984_chip_info_data = { + .max_channels_nr = 20, + .has_eeprom = true, +}; + +static const struct ltc2983_chip_info ltc2986_chip_info_data = { + .max_channels_nr = 10, + .has_temp = true, + .has_eeprom = true, +}; + static const struct spi_device_id ltc2983_id_table[] = { - { "ltc2983" }, + { "ltc2983", (kernel_ulong_t)<c2983_chip_info_data }, + { "ltc2984", (kernel_ulong_t)<c2984_chip_info_data }, + { "ltc2986", (kernel_ulong_t)<c2986_chip_info_data }, + { "ltm2985", (kernel_ulong_t)<c2986_chip_info_data }, {}, }; MODULE_DEVICE_TABLE(spi, ltc2983_id_table); static const struct of_device_id ltc2983_of_match[] = { - { .compatible = "adi,ltc2983" }, + { .compatible = "adi,ltc2983", .data = <c2983_chip_info_data }, + { .compatible = "adi,ltc2984", .data = <c2984_chip_info_data }, + { .compatible = "adi,ltc2986", .data = <c2986_chip_info_data }, + { .compatible = "adi,ltm2985", .data = <c2986_chip_info_data }, {}, }; MODULE_DEVICE_TABLE(of, ltc2983_of_match); -- 2.37.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts 2022-10-14 12:37 ` [PATCH 3/3] " Cosmin Tanislav @ 2022-10-14 15:44 ` Jonathan Cameron 2022-10-17 6:59 ` Cosmin Tanislav 0 siblings, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-10-14 15:44 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Fri, 14 Oct 2022 15:37:24 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > From: Cosmin Tanislav <cosmin.tanislav@analog.com> > > Add support for the following parts: > * LTC2984 > * LTC2986 > * LTM2985 > > The LTC2984 is a variant of the LTC2983 with EEPROM. > The LTC2986 is a variant of the LTC2983 with only 10 channels, > EEPROM and support for active analog temperature sensors. > The LTM2985 is software-compatible with the LTC2986. > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> ... Hi Cosmin, Looks good except, I think we are still in the position that regmap for spi doesn't guarantee to bounce buffer the bulk accesses (last time I checked it actually did do so, but before that it didn't and there are obvious optimizations to take it back to not doing so - IRC Mark Brown's answer was we shouldn't rely on it..) Anyhow, the existing driver has instances of this so its no worse but we should really clean those up. Jonathan > > +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd, > + unsigned int wait_time, unsigned int status_reg, > + unsigned long status_fail_mask) > +{ > + __be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY); > + unsigned long time; > + unsigned int val; > + int ret; > + > + ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval, > + sizeof(bval)); SPI device and I was clearly dozing on existing driver but normally we avoid assuming that regmap will always use a bounce buffer for bulk accessors. Hence this should be a DMA safe buffer. > + if (ret) > + return ret; > + > + reinit_completion(&st->completion); > + > + ret = regmap_write(st->regmap, LTC2983_STATUS_REG, > + LTC2983_STATUS_START(true) | cmd); > + if (ret) > + return ret; > + > + time = wait_for_completion_timeout(&st->completion, > + msecs_to_jiffies(wait_time)); > + if (!time) { > + dev_err(&st->spi->dev, "EEPROM command timed out\n"); > + return -ETIMEDOUT; > + } > + > + ret = regmap_read(st->regmap, status_reg, &val); > + if (ret) > + return ret; > + > + if (val & status_fail_mask) { > + dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val); > + return -EINVAL; > + } > + > + return 0; > +} > + ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts 2022-10-14 15:44 ` Jonathan Cameron @ 2022-10-17 6:59 ` Cosmin Tanislav 2022-10-17 10:29 ` Jonathan Cameron 0 siblings, 1 reply; 22+ messages in thread From: Cosmin Tanislav @ 2022-10-17 6:59 UTC (permalink / raw) To: Jonathan Cameron Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On 10/14/22 18:44, Jonathan Cameron wrote: > On Fri, 14 Oct 2022 15:37:24 +0300 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> >> >> Add support for the following parts: >> * LTC2984 >> * LTC2986 >> * LTM2985 >> >> The LTC2984 is a variant of the LTC2983 with EEPROM. >> The LTC2986 is a variant of the LTC2983 with only 10 channels, >> EEPROM and support for active analog temperature sensors. >> The LTM2985 is software-compatible with the LTC2986. >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > > ... > > Hi Cosmin, > > Looks good except, I think we are still in the position that > regmap for spi doesn't guarantee to bounce buffer the bulk accesses > (last time I checked it actually did do so, but before that it didn't > and there are obvious optimizations to take it back to not doing so - > IRC Mark Brown's answer was we shouldn't rely on it..) > > Anyhow, the existing driver has instances of this so its no worse > but we should really clean those up. > > Jonathan > I can submit another patch for it. Although I'm pretty sure that SPI regmap implementation doesn't need DMA safe access for it, as I checked when I wrote the code. > >> >> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd, >> + unsigned int wait_time, unsigned int status_reg, >> + unsigned long status_fail_mask) >> +{ >> + __be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY); >> + unsigned long time; >> + unsigned int val; >> + int ret; >> + >> + ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval, >> + sizeof(bval)); > > SPI device and I was clearly dozing on existing driver but normally > we avoid assuming that regmap will always use a bounce buffer for bulk > accessors. Hence this should be a DMA safe buffer. > > >> + if (ret) >> + return ret; >> + >> + reinit_completion(&st->completion); >> + >> + ret = regmap_write(st->regmap, LTC2983_STATUS_REG, >> + LTC2983_STATUS_START(true) | cmd); >> + if (ret) >> + return ret; >> + >> + time = wait_for_completion_timeout(&st->completion, >> + msecs_to_jiffies(wait_time)); >> + if (!time) { >> + dev_err(&st->spi->dev, "EEPROM command timed out\n"); >> + return -ETIMEDOUT; >> + } >> + >> + ret = regmap_read(st->regmap, status_reg, &val); >> + if (ret) >> + return ret; >> + >> + if (val & status_fail_mask) { >> + dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts 2022-10-17 6:59 ` Cosmin Tanislav @ 2022-10-17 10:29 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2022-10-17 10:29 UTC (permalink / raw) To: Cosmin Tanislav Cc: Nuno Sá, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, linux-iio, devicetree, linux-kernel, Cosmin Tanislav On Mon, 17 Oct 2022 09:59:27 +0300 Cosmin Tanislav <demonsingur@gmail.com> wrote: > On 10/14/22 18:44, Jonathan Cameron wrote: > > On Fri, 14 Oct 2022 15:37:24 +0300 > > Cosmin Tanislav <demonsingur@gmail.com> wrote: > > > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com> > >> > >> Add support for the following parts: > >> * LTC2984 > >> * LTC2986 > >> * LTM2985 > >> > >> The LTC2984 is a variant of the LTC2983 with EEPROM. > >> The LTC2986 is a variant of the LTC2983 with only 10 channels, > >> EEPROM and support for active analog temperature sensors. > >> The LTM2985 is software-compatible with the LTC2986. > >> > >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > > > > ... > > > > Hi Cosmin, > > > > Looks good except, I think we are still in the position that > > regmap for spi doesn't guarantee to bounce buffer the bulk accesses > > (last time I checked it actually did do so, but before that it didn't > > and there are obvious optimizations to take it back to not doing so - > > IRC Mark Brown's answer was we shouldn't rely on it..) > > > > Anyhow, the existing driver has instances of this so its no worse > > but we should really clean those up. > > > > Jonathan > > > > I can submit another patch for it. Although I'm pretty sure that > SPI regmap implementation doesn't need DMA safe access for it, > as I checked when I wrote the code. It doesn't today (last time I checked it didn't anyway and that matches with what you found), but it did in the past and there are no guarantees it won't require DMA safe buffers in the future. I doubt I'll find the reference but we had a discussion with Mark Brown about this for a similar case a year or two back and he confirmed we should continue to assume that DMA safe buffers should be used. Jonathan > > > > >> > >> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd, > >> + unsigned int wait_time, unsigned int status_reg, > >> + unsigned long status_fail_mask) > >> +{ > >> + __be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY); > >> + unsigned long time; > >> + unsigned int val; > >> + int ret; > >> + > >> + ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval, > >> + sizeof(bval)); > > > > SPI device and I was clearly dozing on existing driver but normally > > we avoid assuming that regmap will always use a bounce buffer for bulk > > accessors. Hence this should be a DMA safe buffer. > > > > > >> + if (ret) > >> + return ret; > >> + > >> + reinit_completion(&st->completion); > >> + > >> + ret = regmap_write(st->regmap, LTC2983_STATUS_REG, > >> + LTC2983_STATUS_START(true) | cmd); > >> + if (ret) > >> + return ret; > >> + > >> + time = wait_for_completion_timeout(&st->completion, > >> + msecs_to_jiffies(wait_time)); > >> + if (!time) { > >> + dev_err(&st->spi->dev, "EEPROM command timed out\n"); > >> + return -ETIMEDOUT; > >> + } > >> + > >> + ret = regmap_read(st->regmap, status_reg, &val); > >> + if (ret) > >> + return ret; > >> + > >> + if (val & status_fail_mask) { > >> + dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-10-18 6:02 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-14 12:37 [PATCH 0/3] Support more parts in LTC2983 Cosmin Tanislav 2022-10-14 12:37 ` [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once Cosmin Tanislav 2022-10-14 14:11 ` Jonathan Cameron 2022-10-14 15:18 ` Jonathan Cameron 2022-10-15 16:35 ` Jonathan Cameron 2022-10-14 12:37 ` [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts Cosmin Tanislav 2022-10-14 15:37 ` Jonathan Cameron 2022-10-17 7:01 ` Cosmin Tanislav 2022-10-17 10:22 ` Jonathan Cameron 2022-10-17 1:59 ` Krzysztof Kozlowski 2022-10-17 6:53 ` Cosmin Tanislav 2022-10-17 10:26 ` Jonathan Cameron 2022-10-17 10:37 ` Jonathan Cameron 2022-10-17 23:26 ` Krzysztof Kozlowski 2022-10-17 9:38 ` Nuno Sá 2022-10-17 10:04 ` Nuno Sá 2022-10-17 23:32 ` Krzysztof Kozlowski 2022-10-18 6:01 ` Nuno Sá 2022-10-14 12:37 ` [PATCH 3/3] " Cosmin Tanislav 2022-10-14 15:44 ` Jonathan Cameron 2022-10-17 6:59 ` Cosmin Tanislav 2022-10-17 10:29 ` Jonathan Cameron
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).