dt-bindings: iio: adc: Add TI ADS1100 and ADS1000
Subsystems (2)
PATCH v3 1/2 dt-bindings: iio: adc: Add TI ADS1100 and ADS1000
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
---
(no changes since v2)
Changes in v2:
"reg" property is mandatory.
Add vdd-supply and #io-channel-cells
.../bindings/iio/adc/ti,ads1100.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml new file +46 -0
@@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI ADS1100/ADS1000 single channel I2C analog to digital converter + +maintainers: + - Mike Looijmans <[email protected]> + +description: | + Datasheet at: https://www.ti.com/lit/gpn/ads1100 + +properties: + compatible: + enum: + - ti,ads1100 + - ti,ads1000 + + reg: + maxItems: 1 + + vdd-supply: true + + "#io-channel-cells": + const: 0 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + adc@49 { + compatible = "ti,ads1100"; + reg = <0x49>; + }; + }; +...
Signature · 1 line
2.17.1
PATCH v3 2/2 iio: adc: Add TI ADS1100 and ADS1000
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
---
Changes in v3:
Add #include linux/bitfield.h and linux/bits.h
Changes in v2:
Remove "driver for" from title
Use proper PM_RUNTIME macros
Fix indents
Use dev_err_probe()
Unsigned index and post-increment
Use GENMASK and FIELD_GET
remove ads1100_set_conv_mode
remove Kconfig BUFFER dependencies
remove unused #include
Set SCALE instead of HARDWAREGAIN
Use devm_add_action_or_reset
drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ti-ads1100.c | 434 +++++++++++++++++++++++++++++++++++ 3 files changed, 445 insertions(+) create mode 100644 drivers/iio/adc/ti-ads1100.c
drivers/iio/adc/Kconfig +10 -0
@@ -1207,6 +1207,16 @@ config TI_ADS1015 This driver can also be built as a module. If so, the module will be called ti-ads1015. +config TI_ADS1100 + tristate "Texas Instruments ADS1100 and ADS1000 ADC" + depends on I2C + help + If you say yes here you get support for Texas Instruments ADS1100 and + ADS1000 ADC chips. + + This driver can also be built as a module. If so, the module will be + called ti-ads1100. + config TI_ADS7950 tristate "Texas Instruments ADS7950 ADC driver" depends on SPI && GPIOLIB
drivers/iio/adc/Makefile +1 -0
@@ -107,6 +107,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o +obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
drivers/iio/adc/ti-ads1100.c new file +434 -0
434 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 2 lines · Mike Looijmans
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
...
drivers/iio/adc/ti-ads1100.c line 119
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+/* Shift result to compensate for bit resolution vs. sample rate */ + value <<=16 -ads1100_data_bits (data); + *val =sign_extend32 (value,15 );
Why not simply
*val = sign_extend32(value, ads1100_data_bits(data) - 1);
?
(Double check for off-by-one usage)
...
drivers/iio/adc/ti-ads1100.c line 135
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
Can you use more math / plain English to describe the formula? Otherwise we can see the very same in the code and point of the comment is doubtful.
drivers/iio/adc/ti-ads1100.c line 136
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+ gain = ((microvolts +BIT (14 )) >>15 ) *1000 / val2;
Something from units.h?
...
drivers/iio/adc/ti-ads1100.c line 139
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) {
ffs()/__ffs() (look at the documentation for the difference and use proper one).
Quoted 4 lines
drivers/iio/adc/ti-ads1100.c new file +4
-0+ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }
...
drivers/iio/adc/ti-ads1100.c line 154
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+for (i =0 ; i < size; ++i) {
Why pre-increment?
drivers/iio/adc/ti-ads1100.c line 157
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+if (ads1100_data_rate[i] == rate) { + return ads1100_set_config_bits( + data, ADS1100_DR_MASK,
Strange indentation.
Review
Quoted 2 lines
(quoted diff) +2
-0+ FIELD_PREP(ADS1100_DR_MASK, i)); + }
Do you need {} ?
Quoted 1 line
(quoted diff) +1
-0+ }
...
drivers/iio/adc/ti-ads1100.c line 167
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+int millivolts =regulator_get_voltage (data->reg_vdd) /1000 ;
units.h?
...
drivers/iio/adc/ti-ads1100.c line 171
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+ data->scale_avail[i *2 ] = millivolts;
I would write ' * 2 + 0]', but it's up to you.
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+ data->scale_avail[i *2 +1 ] =15 + i;
...
drivers/iio/adc/ti-ads1100.c line 226
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+ *val =regulator_get_voltage (data->reg_vdd) /1000 ;
units.h?
...
drivers/iio/adc/ti-ads1100.c line 232
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+ *val = ads1100_data_rate[ +FIELD_GET (ADS1100_DR_MASK , data->config)];
Strange indentation, just use a single line.
...
drivers/iio/adc/ti-ads1100.c line 363
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+ ret =devm_iio_device_register (dev, indio_dev); +if (ret <0 )
Why ' < 0'?
Quoted 2 lines
(quoted diff) +2
-0+ return dev_err_probe(dev, ret, + "Failed to register IIO device\n");
...
drivers/iio/adc/ti-ads1100.c line 376
Quoted 7 lines
drivers/iio/adc/ti-ads1100.c new file +7
-0+static int ads1100_runtime_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev)); +struct ads1100_data *data =iio_priv (indio_dev); + +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +regulator_disable (data->reg_vdd);
Wrong devm / non-devm ordering.
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
...
drivers/iio/adc/ti-ads1100.c line 408
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static const struct i2c_device_id ads1100_id[] = { + {"ads1100" , }, + {"ads1000" , },
Inner commas are not needed.
Quoted 2 lines
(quoted diff) +2
-0+ {} +};
...
drivers/iio/adc/ti-ads1100.c line 415
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static const struct of_device_id ads1100_of_match[] = { + { .compatible ="ti,ads1100" , }, + { .compatible ="ti,ads1000" , },
Ditto.
Quoted 2 lines
(quoted diff) +2
-0+ {} +};
...
Quoted 1 line
+
Redundant blank line.
Quoted 1 line
+module_i2c_driver(ads1100_driver);
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Comments below. Mailserver has a top-post fetish and will inject a signature here somewhere...
No further comment from me means "agree, will implement in v4"...
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: www.topic.nl
Please consider the environment before printing this e-mail
Quoted 5 lines · Andy Shevchenko
Quoted 2 lines · Mike Looijmans
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
...
Quoted 8 lines
drivers/iio/adc/ti-ads1100.c line 119
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+/* Shift result to compensate for bit resolution vs. sample rate */ + value <<=16 -ads1100_data_bits (data); + *val =sign_extend32 (value,15 );Why not simply
*val = sign_extend32(value, ads1100_data_bits(data) - 1);?
As discussed with Jonathan Cameron, the register is right-justified and the number of bits depend on the data rate. Rather than having the "scale" change when the sample rate changes, we chose to adjust the sample result so it's always left-justified.
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c line 135
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ Can you use more math / plain English to describe the formula? Otherwise we can see the very same in the code and point of the comment is doubtful.
I'll try to explain it better.
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c line 136
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +1
-0+ gain = ((microvolts +BIT (14 )) >>15 ) *1000 / val2;Something from
units.h?
Could put MILLI here, but I doubt if that improves things. Its actually MICRO/(MICRO/MILLI) given the explanation above... Not helping much...
Quoted 5 lines
...
drivers/iio/adc/ti-ads1100.c line 139
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) {ffs()/__ffs() (look at the documentation for the difference and use proper one).
Thought of it, but I'd rather have it return EINVAL for attempting to set the analog gain to "7" (0nly 1,2,4,8 allowed).
Quoted 3 lines
...
drivers/iio/adc/ti-ads1100.c line 154
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+for (i =0 ; i < size; ++i) {Why pre-increment?
Spent too much time with other coding guidelines, missed this one... Will change.
Quoted 5 lines
...
drivers/iio/adc/ti-ads1100.c line 167
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+int millivolts =regulator_get_voltage (data->reg_vdd) /1000 ;
units.h?
Should I write:
regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS);
I doubt that improves readability.
Quoted 11 lines
...
...drivers/iio/adc/ti-ads1100.c line 376
Quoted 7 lines
drivers/iio/adc/ti-ads1100.c new file +7
-0+static int ads1100_runtime_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev)); +struct ads1100_data *data =iio_priv (indio_dev); + +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +regulator_disable (data->reg_vdd);Wrong devm / non-devm ordering.
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Signature · 1 line
Mike Looijmans
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 15 lines · Mike Looijmans
Quoted 5 lines
...
...Quoted 9 lines
Quoted 8 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static int ads1100_runtime_suspend (struct device *dev) +{ + struct iio_dev *indio_dev =i2c_get_clientdata(to_i2c_client(dev));
+ structads1100_data*data =iio_priv(indio_dev);
+
+ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); +regulator_disable(data->reg_vdd);Wrong devm / non-devm ordering.
Quoted 3 lines
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Strictly speaking we need to unregister the IIO device before disabling the regulator, otherwise there is a small window where the IIO device still exists, but doesn't work anymore. This is a very theoretical scenario though.
You are lucky :) There is a new function devm_regulator_get_enable()[1], which will manage the regulator_disable() for you. Using that will also reduce the boilerplate in probe() a bit
- Lars
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 21 lines · Lars-Peter Clausen
Quoted 16 lines · Mike Looijmans
Quoted 5 lines
...
...Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static int ads1100_runtime_suspend (struct device *dev) +{ + struct iio_dev *indio_dev =i2c_get_clientdata(to_i2c_client(dev));
+ structads1100_data*data =iio_priv(indio_dev); +
+ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
+regulator_disable(data->reg_vdd);Wrong devm / non-devm ordering.
Quoted 3 lines
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Quoted 8 lines
Strictly speaking we need to unregister the IIO device before disabling the regulator, otherwise there is a small window where the IIO device still exists, but doesn't work anymore. This is a very theoretical scenario though.
You are lucky :) There is a new function
devm_regulator_get_enable()[1], which will manage theregulator_disable()for you. Using that will also reduce the boilerplate inprobe()a bit- Lars
Sorry, just saw that Andy's comment was on the suspend() function, not remove(). In that case there is of course no need for any devm things. But still a good idea to use devm_regulator_get_enable() in probe for the boiler plate.
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 2 lines · Lars-Peter Clausen
Quoted 1 line · Lars-Peter Clausen
On 3/1/23 23:49, Mike Looijmans wrote:
...
Quoted 27 lines
Quoted 27 lines
Quoted 11 lines
Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static int ads1100_runtime_suspend (struct device *dev) +{ + struct iio_dev *indio_dev =i2c_get_clientdata(to_i2c_client(dev));
+ structads1100_data*data =iio_priv(indio_dev); +
+ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
+regulator_disable(data->reg_vdd);Wrong devm / non-devm ordering.
Quoted 5 lines
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Strictly speaking we need to unregister the IIO device before disabling the regulator, otherwise there is a small window where the IIO device still exists, but doesn't work anymore. This is a very theoretical scenario though.
You are lucky :) There is a new function
devm_regulator_get_enable()[1], which will manage theregulator_disable()for you. Using that will also reduce the boilerplate inprobe()a bit
Quoted 6 lines
Quoted 2 lines
Sorry, just saw that Andy's comment was on the
suspend()function, notremove(). In that case there is of course no need for any devm things. But still a good idea to usedevm_regulator_get_enable()in probe for the boiler plate.
Yeah, sorry, I mistakenly took it as ->remove().
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Lars-Peter ClausenQuoted 34 lines · On Thu, 2 Mar 2023 05:20:38 -0800 Lars-Peter Clausen <[email protected]> wrote:
Quoted 33 lines · Lars-Peter Clausen
Quoted 16 lines · Mike Looijmans
Quoted 5 lines
...
...Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+static int ads1100_runtime_suspend (struct device *dev) +{ + struct iio_dev *indio_dev =i2c_get_clientdata(to_i2c_client(dev));
+ structads1100_data*data =iio_priv(indio_dev); +
+ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
+regulator_disable(data->reg_vdd);Wrong devm / non-devm ordering.
Quoted 5 lines
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Strictly speaking we need to unregister the IIO device before disabling the regulator, otherwise there is a small window where the IIO device still exists, but doesn't work anymore. This is a very theoretical scenario though.
You are lucky :) There is a new function
devm_regulator_get_enable()[1], which will manage theregulator_disable()for you. Using that will also reduce the boilerplate inprobe()a bit
Quoted 9 lines
Quoted 4 lines
- Lars
Sorry, just saw that Andy's comment was on the
suspend()function, notremove(). In that case there is of course no need for any devm things. But still a good idea to usedevm_regulator_get_enable()in probe for the boiler plate.
You can't because (annoyingly) devem_regulator_get_enable() doesn't provide you access to the struct regulator that you need to be able to turn it of for power management.
That case only works for the leave the power on all the time cases.
Jonathan
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Mike LooijmansQuoted 2 lines · Mike Looijmans
Quoted 1 line · Andy Shevchenko
On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:
...
Quoted 5 lines
Quoted 5 lines
drivers/iio/adc/ti-ads1100.c line 119
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+/* Shift result to compensate for bit resolution vs. sample rate */ + value <<=16 -ads1100_data_bits (data); + *val =sign_extend32 (value,15 );Why not simply
Quoted 8 lines
Quoted 3 lines
*val = sign_extend32(value, ads1100_data_bits(data) - 1);?
As discussed with Jonathan Cameron, the register is right-justified and the number of bits depend on the data rate. Rather than having the "scale" change when the sample rate changes, we chose to adjust the sample result so it's always left-justified.
Hmm... OK, but it adds unneeded code I think.
...
Quoted 6 lines
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c line 139
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) {ffs()/__ffs() (look at the documentation for the difference and use proper one).
Thought of it, but I'd rather have it return EINVAL for attempting to set the analog gain to "7" (0nly 1,2,4,8 allowed).
I'm not sure what you are implying.
You have open coded something that has already to be a function which on some architectures become a single assembly instruction.
That said, drop your for-loop if-cond and use one of the proposed directly. Then you may compare the result to what ever you want to be a limit and return whatever error code you want to.
...
Quoted 5 lines
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c line 154
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+for (i =0 ; i < size; ++i) {Why pre-increment?
Spent too much time with other coding guidelines, missed this one... Will change.
I don't remember that's in coding guidelines, but it's standard practice in the Linux kernel project. Yeah, we have a few hundreds of the pre-increments, but reasons may be quite different for those.
...
Quoted 8 lines
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c line 167
Quoted 1 line
drivers/iio/adc/ti-ads1100.c new file +1
-0+int millivolts =regulator_get_voltage (data->reg_vdd) /1000 ;
units.h?Should I write:
regulator_get_voltage(data->reg_vdd)/ (MICROS / MILLIS);I doubt that improves readability.
Yeah, it should be something like MICROVOLT_PER_MILLIVOLT. But it's not defined yet.
...
Quoted 9 lines
Quoted 8 lines
drivers/iio/adc/ti-ads1100.c line 376
Quoted 7 lines
drivers/iio/adc/ti-ads1100.c new file +7
-0+static int ads1100_runtime_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev)); +struct ads1100_data *data =iio_priv (indio_dev); + +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +regulator_disable (data->reg_vdd);Wrong devm / non-devm ordering.
Quoted 5 lines
Don't understand your remark, can you explain further please?
devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
Ah, it's my mistake, I misread it as ->remove().
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 13 lines · On Thu, 2 Mar 2023 16:23:14 +0200 Andy Shevchenko…
Quoted 2 lines · Mike Looijmans
Quoted 1 line · Andy Shevchenko
On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:
...
Quoted 7 lines
Quoted 7 lines
drivers/iio/adc/ti-ads1100.c line 119
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+/* Shift result to compensate for bit resolution vs. sample rate */ + value <<=16 -ads1100_data_bits (data); + *val =sign_extend32 (value,15 );Why not simply
*val = sign_extend32(value, ads1100_data_bits(data) - 1);
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
?
As discussed with Jonathan Cameron, the register is right-justified and the number of bits depend on the data rate. Rather than having the "scale" change when the sample rate changes, we chose to adjust the sample result so it's always left-justified.
Hmm... OK, but it adds unneeded code I think.
There isn't a way to do it in one go that I can think of.
The first statement is multiplying the value by a power of 2, not just sign extending it. You could sign extend first then shift to do the multiply, but ends up same amount of code.
It does look a bit like a weird open coded sign extension though so I can see where the confusion came from!
Quoted 10 lines
...
Quoted 6 lines
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c line 139
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) {ffs()/__ffs() (look at the documentation for the difference and use proper one).
Thought of it, but I'd rather have it return EINVAL for attempting to set the analog gain to "7" (0nly 1,2,4,8 allowed).
Quoted 8 lines
I'm not sure what you are implying.
You have open coded something that has already to be a function which on some architectures become a single assembly instruction.
That said, drop your for-loop if-cond and use one of the proposed directly. Then you may compare the result to what ever you want to be a limit and return whatever error code you want to
Agreed, could do it with appropriate ffs() followed by if (BIT(i) != gain) return -EINVAL;
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 5 lines · Jonathan Cameron
Quoted 3 lines · On Thu, 2 Mar 2023 16:23:14 +0200 Andy Shevchenko…
Quoted 2 lines · Mike Looijmans
Quoted 1 line · Andy Shevchenko
On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:
...
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/iio/adc/ti-ads1100.c line 119
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c new file +3
-0+/* Shift result to compensate for bit resolution vs. sample rate */ + value <<=16 -ads1100_data_bits (data); + *val =sign_extend32 (value,15 );Why not simply
*val = sign_extend32(value, ads1100_data_bits(data) - 1);Quoted 8 lines
Quoted 6 lines
Quoted 1 line
?
As discussed with Jonathan Cameron, the register is right-justified and the number of bits depend on the data rate. Rather than having the "scale" change when the sample rate changes, we chose to adjust the sample result so it's always left-justified.
Hmm... OK, but it adds unneeded code I think.
Quoted 7 lines
There isn't a way to do it in one go that I can think of.
The first statement is multiplying the value by a power of 2, not just sign extending it. You could sign extend first then shift to do the multiply, but ends up same amount of code.It does look a bit like a weird open coded sign extension though so I can see where the confusion came from!
I see, for the negative value both approaches will work, for the positive the original one will provide a multiplied value.
Yeah, doesn't seem to be a subject to the (micro-)optimizations.
...
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
drivers/iio/adc/ti-ads1100.c line 139
Quoted 2 lines
drivers/iio/adc/ti-ads1100.c new file +2
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) {ffs()/__ffs() (look at the documentation for the difference and use proper one).
Thought of it, but I'd rather have it return EINVAL for attempting to set the analog gain to "7" (0nly 1,2,4,8 allowed).
I'm not sure what you are implying.
Quoted 8 lines
Quoted 6 lines
You have open coded something that has already to be a function which on some architectures become a single assembly instruction.
That said, drop your for-loop if-cond and use one of the proposed directly. Then you may compare the result to what ever you want to be a limit and return whatever error code you want to
Agreed, could do it with appropriate
ffs()followed by if (BIT(i) != gain) return-EINVAL;
I meant something different.
i = ffs(gain); // or __ffs(gain)? if (i >= 4) return -EINVAL;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Mike LooijmansQuoted 4 lines · On Tue, 28 Feb 2023 07:31:51 +0100 Mike Looijmans <[email protected]>…
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
Signed-off-by Mike Looijmans <[email protected]>
Hi Mike,
A few minor things + one request for a test as trying to chase a possible ref count overflow around the runtime_pm was giving me a enough of a headache that it's easier to ask you just to poke it and see. If it doesn't fail as I expect I'll take a closer look!
Jonathan
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+static int ads1100_set_scale (struct ads1100_data *data,int val,int val2) +{ +int microvolts; +int gain; +int i; + +/* With Vdd between 2.7 and 5V, the scale is always below 1 */ +if (val) +return -EINVAL ; +
drivers/iio/adc/ti-ads1100.c line 143
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ microvolts =regulator_get_voltage (data->reg_vdd); +/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ + gain = ((microvolts +BIT (14 )) >>15 ) *1000 / val2; + +for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }
Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads...
Is perhaps nicer than the loop.
Quoted 13 lines
drivers/iio/adc/ti-ads1100.c new file +11
-0+ +return -EINVAL ; +} +static void ads1100_disable_continuous (void *data) +{ +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +} + +static int ads1100_probe (struct i2c_client *client) +{ +struct iio_dev *indio_dev;
drivers/iio/adc/ti-ads1100.c line 320
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+struct ads1100_data *data; +struct device *dev = &client->dev; +int ret; + + indio_dev =devm_iio_device_alloc (dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + + data =iio_priv (indio_dev); +i2c_set_clientdata (client, indio_dev);
You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata() below by taking advantage of the fact you have a local dev pointer.
dev_set_drvdata(dev, indio_dev);
and no confusing mix is left. Of course it's doing the same thing but to my mind slightly nicer to use the same one.
Quoted 12 lines
drivers/iio/adc/ti-ads1100.c new file +12
-0+ data->client = client; +mutex_init (&data->lock); + + indio_dev->name ="ads1100" ; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = &ads1100_channel; + indio_dev->num_channels =1 ; + indio_dev->info = &ads1100_info; + + data->reg_vdd =devm_regulator_get (dev,"vdd" ); +if (IS_ERR (data->reg_vdd)) + return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
drivers/iio/adc/ti-ads1100.c line 342
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ "Failed to get vdd regulator\n"); + + ret =regulator_enable (data->reg_vdd); +if (ret <0 ) +return dev_err_probe (dev,PTR_ERR (data->reg_vdd), +"Failed to enable vdd regulator\n" ); + + ret =devm_add_action_or_reset (dev, ads1100_reg_disable, data->reg_vdd); +if (ret) +return ret;
Please could you check a subtle interaction of runtime pm and this devm managed flow.
I think we can hit the following flow.
1) In runtime suspend (wait long enough for this to happen).
2) Unbind the driver (rmmod will do)
3) During the unbind we exit suspend then enter it again before we call remove (that's just part of the normal remove flow).
4) We then end up calling regulator disable when it's already disabled.
We've traditionally avoided that by having the remove explicitly call pm_runtime_get_sync() before we then disable runtime pm. I don't think that happens with devm_pm_runtime_enable() but I could be missing a path where it does.
If the sequence goes wrong you should get a warning about an unbalanced regulator disable. The fix would be an extra devm_add_action_or_reset() before the devm_iio_device_register() below that just calls pm_runtime_get_sync() to force the state to on.
Gah. These subtle paths always give me a headache.
We don't normally have too much problem with this because many runtime_resume / suspend functions don't change reference counts.
Quoted 20 lines
drivers/iio/adc/ti-ads1100.c new file +20
-0+ + ret =ads1100_setup (data); +if (ret) +return dev_err_probe (dev, ret, +"Failed to communicate with device\n" ); + + ret =devm_add_action_or_reset (dev, ads1100_disable_continuous, data); +if (ret) +return ret; + +ads1100_calc_scale_avail (data); + +pm_runtime_set_autosuspend_delay (dev,ADS1100_SLEEP_DELAY_MS ); +pm_runtime_use_autosuspend (dev); +pm_runtime_set_active (dev); + ret =devm_pm_runtime_enable (dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to enable pm_runtime\n" ); + + ret =devm_iio_device_register (dev, indio_dev);
drivers/iio/adc/ti-ads1100.c line 372
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+if (ret <0 ) +return dev_err_probe (dev, ret, +"Failed to register IIO device\n" ); + +return 0 ; +} + +static int ads1100_runtime_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev));
Fine to just short cut this dance with
struct iio_dev *indio_dev = dev_get_drvdata(dev);
It's a bit nasty from a readability point of view, but the pattern is so common we've kind of gotten used to it.
Review
Quoted 1 line
(quoted diff) +1
-0+ struct ads1100_data *data = iio_priv(indio_dev);
As you don't need the indio_dev, can combine all this into
struct ads110_data *data = iio_priv(dev_get_drvdata(dev));
drivers/iio/adc/ti-ads1100.c line 383
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +regulator_disable (data->reg_vdd); + +return 0 ; +} + +static int ads1100_runtime_resume (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev));
As above.
Quoted 17 lines
drivers/iio/adc/ti-ads1100.c new file +17
-0+struct ads1100_data *data =iio_priv (indio_dev); +int ret; + + ret =regulator_enable (data->reg_vdd); +if (ret) { +dev_err (&data->client->dev,"Failed to enable Vdd\n" ); +return ret; + } + +/* +* We'll always change the mode bit in the config register, so there is +* no need here to "force" a write to the config register. If the device +* has been power-cycled, we'll re-write its config register now. +*/ +return ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_CONTINUOUS ); +} +
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: www.topic.nl
Please consider the environment before printing this e-mail
Quoted 6 lines · Jonathan Cameron
Quoted 3 lines · On Tue, 28 Feb 2023 07:31:51 +0100 Mike Looijmans <[email protected]>…
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
Quoted 7 lines
Quoted 1 line
Signed-off-by Mike Looijmans <[email protected]>Hi Mike,
A few minor things + one request for a test as trying to chase a possible ref count overflow around the
runtime_pmwas giving me a enough of a headache that it's easier to ask you just to poke it and see. If it doesn't fail as I expect I'll take a closer look!
Will do, but it may take a few days to get access to the hardware again. I'll report on that later.
Quoted 29 lines
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+static int ads1100_set_scale (struct ads1100_data *data,int val,int val2) +{ +int microvolts; +int gain; +int i; + +/* With Vdd between 2.7 and 5V, the scale is always below 1 */ +if (val) +return -EINVAL ; +drivers/iio/adc/ti-ads1100.c line 143
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ microvolts =regulator_get_voltage (data->reg_vdd); +/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ + gain = ((microvolts +BIT (14 )) >>15 ) *1000 / val2; + +for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads...
Quoted 1 line
Is perhaps nicer than the loop.
Yes, takes out a loop.
Quoted 22 lines
Quoted 9 lines
drivers/iio/adc/ti-ads1100.c new file +8
-0+static void ads1100_disable_continuous (void *data) +{ +ads1100_set_config_bits (data,ADS1100_CFG_SC ,ADS1100_SINGLESHOT ); +} + +static int ads1100_probe (struct i2c_client *client) +{ +struct iio_dev *indio_dev;drivers/iio/adc/ti-ads1100.c line 320
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+struct ads1100_data *data; +struct device *dev = &client->dev; +int ret; + + indio_dev =devm_iio_device_alloc (dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + + data =iio_priv (indio_dev); +i2c_set_clientdata (client, indio_dev);You can avoid the slightly nasty mix of
i2c_set_clientdatavsdev_get_drvdata()below by taking advantage of the fact you have a local dev pointer.
Quoted 3 lines
dev_set_drvdata(dev, indio_dev);and no confusing mix is left. Of course it's doing the same thing but to my mind slightly nicer to use the same one.
Since I don't need indio_dev anywhere, I might as well say this directly:
dev_set_drvdata(dev, data);
(Only the pm_ routines use this)
Quoted 2 lines
...
Signature · 1 line
Mike Looijmans
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Jonathan CameronMet vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: www.topic.nl
Please consider the environment before printing this e-mail
Quoted 54 lines · Jonathan Cameron
Quoted 4 lines · On Tue, 28 Feb 2023 07:31:51 +0100 Mike Looijmans <[email protected]>…
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
Signed-off-by Mike Looijmans <[email protected]>Hi Mike,
A few minor things + one request for a test as trying to chase a possible ref count overflow around the
runtime_pmwas giving me a enough of a headache that it's easier to ask you just to poke it and see. If it doesn't fail as I expect I'll take a closer look!Jonathan
...
Quoted 12 lines
drivers/iio/adc/ti-ads1100.c new file +12
-0+ data->client = client; +mutex_init (&data->lock); + + indio_dev->name ="ads1100" ; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = &ads1100_channel; + indio_dev->num_channels =1 ; + indio_dev->info = &ads1100_info; + + data->reg_vdd =devm_regulator_get (dev,"vdd" ); +if (IS_ERR (data->reg_vdd)) + return dev_err_probe(dev, PTR_ERR(data->reg_vdd),drivers/iio/adc/ti-ads1100.c line 342
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ "Failed to get vdd regulator\n"); + + ret =regulator_enable (data->reg_vdd); +if (ret <0 ) +return dev_err_probe (dev,PTR_ERR (data->reg_vdd), +"Failed to enable vdd regulator\n" ); + + ret =devm_add_action_or_reset (dev, ads1100_reg_disable, data->reg_vdd); +if (ret) +return ret;Please could you check a subtle interaction of runtime pm and this devm managed flow.
I think we can hit the following flow.
1) In runtime suspend (wait long enough for this to happen).
2) Unbind the driver (rmmod will do)
3) During the unbind we exit suspend then enter it again before we call remove (that's just part of the normal remove flow).
4) We then end up calling regulator disable when it's already disabled.We've traditionally avoided that by having the remove explicitly call
pm_runtime_get_sync()before we then disable runtime pm. I don't think that happens withdevm_pm_runtime_enable()but I could be missing a path where it does.
Quoted 8 lines
If the sequence goes wrong you should get a warning about an unbalanced regulator disable. The fix would be an extra
devm_add_action_or_reset()before thedevm_iio_device_register()below that just callspm_runtime_get_sync()to force the state to on.Gah. These subtle paths always give me a headache.
We don't normally have too much problem with this because manyruntime_resume/ suspend functions don't change reference counts.
Just did this test, waited a few seconds, checked /sys/kernel/debug/regulator... that the regulator had been disabled.
Then executed:
echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind
to unload the driver, and no messages were added to the kernel log.
I could see the driver going away and removing itself from iio and regulators.
Tried this a couple of times (using bind/unbind), and no problem reported.
Hopes this helps with your headaches...
Signature · 1 line
Mike Looijmans
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 85 lines · On Mon, 6 Mar 2023 12:21:44 +0100 Mike Looijmans <[email protected]>…
Met vriendelijke groet / kind regards,
Mike Looijmans
System ExpertTOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The NetherlandsT: +31 (0) 499 33 69 69
E: [email protected]
W: www.topic.nlPlease consider the environment before printing this e-mail
Quoted 54 lines · Jonathan Cameron
Quoted 4 lines · On Tue, 28 Feb 2023 07:31:51 +0100 Mike Looijmans <[email protected]>…
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
Signed-off-by Mike Looijmans <[email protected]>Hi Mike,
A few minor things + one request for a test as trying to chase a possible ref count overflow around the
runtime_pmwas giving me a enough of a headache that it's easier to ask you just to poke it and see. If it doesn't fail as I expect I'll take a closer look!Jonathan
...
Quoted 12 lines
drivers/iio/adc/ti-ads1100.c new file +12
-0+ data->client = client; +mutex_init (&data->lock); + + indio_dev->name ="ads1100" ; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = &ads1100_channel; + indio_dev->num_channels =1 ; + indio_dev->info = &ads1100_info; + + data->reg_vdd =devm_regulator_get (dev,"vdd" ); +if (IS_ERR (data->reg_vdd)) + return dev_err_probe(dev, PTR_ERR(data->reg_vdd),drivers/iio/adc/ti-ads1100.c line 342
Quoted 10 lines
drivers/iio/adc/ti-ads1100.c new file +10
-0+ "Failed to get vdd regulator\n"); + + ret =regulator_enable (data->reg_vdd); +if (ret <0 ) +return dev_err_probe (dev,PTR_ERR (data->reg_vdd), +"Failed to enable vdd regulator\n" ); + + ret =devm_add_action_or_reset (dev, ads1100_reg_disable, data->reg_vdd); +if (ret) +return ret;Please could you check a subtle interaction of runtime pm and this devm managed flow.
I think we can hit the following flow.
1) In runtime suspend (wait long enough for this to happen).
2) Unbind the driver (rmmod will do)
3) During the unbind we exit suspend then enter it again before we call remove (that's just part of the normal remove flow).
4) We then end up calling regulator disable when it's already disabled.We've traditionally avoided that by having the remove explicitly call
pm_runtime_get_sync()before we then disable runtime pm. I don't think that happens withdevm_pm_runtime_enable()but I could be missing a path where it does.Quoted 8 lines
If the sequence goes wrong you should get a warning about an unbalanced regulator disable. The fix would be an extra
devm_add_action_or_reset()before thedevm_iio_device_register()below that just callspm_runtime_get_sync()to force the state to on.Gah. These subtle paths always give me a headache.
We don't normally have too much problem with this because manyruntime_resume/ suspend functions don't change reference counts.Just did this test, waited a few seconds, checked
/sys/kernel/debug/regulator... that the regulator had been disabled.Then executed:
echo -n 3-004a >/sys/bus/i2c/drivers/ads1100/unbind
Quoted 8 lines
to unload the driver, and no messages were added to the kernel log.
I could see the driver going away and removing itself from iio and regulators.
Tried this a couple of times (using bind/unbind), and no problem reported.
Hopes this helps with your headaches...
Thanks!
Quoted 1 line
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Jonathan CameronQuoted 2 lines · Jonathan Cameron
On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> wrote:
...
Quoted 15 lines
drivers/iio/adc/ti-ads1100.c line 143
Quoted 6 lines
drivers/iio/adc/ti-ads1100.c new file +6
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads...
Quoted 1 line
Is perhaps nicer than the loop.
Even better:
if (!gain || !is_power_of_2(gain)) return -EINVAL; i = ffs(gain); if (i >= 4) return -EINVAL;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Jonathan Cameron
On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> wrote:
...
Quoted 13 lines
Quoted 13 lines
drivers/iio/adc/ti-ads1100.c line 143
Quoted 6 lines
drivers/iio/adc/ti-ads1100.c new file +6
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL;
Quoted 8 lines
Quoted 3 lines
ads...Is perhaps nicer than the loop.
Even better:
if (!gain || !is_power_of_2(gain)) return -EINVAL;
Or if you want to combine all checks:
if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain)) return -EINVAL; ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain)); return 0;
(You can play with bloat-o-meter for the code generation and see which one is
better from that aspect)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Jonathan Cameron
On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> wrote:
...
Quoted 19 lines
Quoted 19 lines
Quoted 15 lines
drivers/iio/adc/ti-ads1100.c line 143
Quoted 6 lines
drivers/iio/adc/ti-ads1100.c new file +6
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads...Quoted 1 line
Is perhaps nicer than the loop.
Even better:
Quoted 7 lines
Quoted 2 lines
if (!gain || !is_power_of_2(gain)) return -EINVAL;Or if you want to combine all checks:
if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain)) return -EINVAL;
Just a side note. I have been wanting to have is_in_range() for a long time. Perhaps we can add a such to the kernel (math.h)
/* Check if in the range [start .. end] */
#define is_in_range(value, start, end) (value >= start && value <= end)
With it, the above will probably better look
if (!is_in_range(gain, BIT(0), BIT(3) || !is_power_of_2(gain))
Quoted 5 lines
ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain)); return 0;(You can play with bloat-o-meter for the code generation and see which one is
better from that aspect)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
In reply to Andy ShevchenkoMet vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: www.topic.nl
Please consider the environment before printing this e-mail
Quoted 21 lines · Andy Shevchenko
Quoted 2 lines · Jonathan Cameron
On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> wrote:...
Quoted 15 lines
drivers/iio/adc/ti-ads1100.c line 143
Quoted 6 lines
drivers/iio/adc/ti-ads1100.c new file +6
-0+for (i =0 ; i <4 ; i++) { +if (BIT (i) == gain) { +ads1100_set_config_bits (data,ADS1100_PGA_MASK , i); +return 0 ; + } + }Andy's suggestion of something like..
if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads...Quoted 1 line
Is perhaps nicer than the loop.
Quoted 9 lines
Even better:
if (!gain || !is_power_of_2(gain)) return -EINVAL; i = ffs(gain); if (i >= 4) return -EINVAL;
I'd guess that "is_power_of_2" is about the same complexity as "ffs".
If we want smaller code, in retrospect, I'd vote for omitting that power-of-two check altogether.
The IIO device reports this for a 3v3 Vdd:
# cat /sys/bus/iio/devices/iio\:device1/scale_available 0.100708007 0.050354003 0.025177001 0.012588500 # echo 0.012588500 > /sys/bus/iio/devices/iio:device1/scale
That last statement results in val2=12588 in this call.
The whole point of this exercise is that the value '0.012588' corresponds to a gain of '8' here. There's already quite a bit of rounding going on, since writing to the scale turns the value into "micro" scale.
Also, looking at the "avail" table. the values for "7" and "8" are much closer together than "3" and "4".
And the correct formula for the inverse of "gain = BIT(i);" is "i = ffs(gain) - 1;" because ffs(1) == 1
So I propose this code:
if (gain <= 0 || gain > 8)
return -EINVAL;
regval = ffs(gain) - 1; ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);
Signature · 1 line
Mike Looijmans
Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000
Quoted 4 lines · Mike Looijmans
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Jonathan Cameron
On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> wrote:
...
Quoted 3 lines
So I propose this code:
if (gain <= 0 || gain > 8)
Maybe BIT(0) and BIT(3) will be more explicit.
Otherwise I'm fine with it.
Quoted 4 lines
return
-EINVAL;regval = ffs(gain) - 1;
ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 5 lines · Mike Looijmans
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate.
Signed-off-by Mike Looijmans <[email protected]>
Quoted 7 lines
---
(no changes since v2)
Changes in v2:
"reg" property is mandatory.
Add vdd-supply and #io-channel-cells
Best regards,
Krzysztof