diff mbox series

[v4,1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000

Message ID 20230306131312.7170-1-mike.looijmans@topic.nl
State Superseded, archived
Headers show
Series [v4,1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000 | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Mike Looijmans March 6, 2023, 1:13 p.m. UTC 
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 <mike.looijmans@topic.nl>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

(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

Comments

Andy Shevchenko March 6, 2023, 1:29 p.m. UTC | #1 
On Mon, Mar 06, 2023 at 02:13:12PM +0100, Mike Looijmans wrote:
> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> The ADS1000 is similar, but has a fixed data rate.

...

> +	/* Value is always 16-bit 2's complement */
> +	value = be16_to_cpu(buffer);

+ Blank line?

> +	/* Shift result to compensate for bit resolution vs. sample rate */
> +	value <<= 16 - ads1100_data_bits(data);

+ Blank line?

> +	*val = sign_extend32(value, 15);

...

> +	microvolts = regulator_get_voltage(data->reg_vdd);
> +	/*
> +	 * val2 is in 'micro' units, n = val2 / 1000000
> +	 * result must be millivolts, d = microvolts / 1000
> +	 * the full-scale value is d/n, corresponds to 2^15,
> +	 * hence the gain = (d / n) >> 15, factoring out the 1000 and moving the
> +	 * bitshift so everything fits in 32-bits yields this formula.
> +	 */
> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;

Perhaps adding MICROVOLT_PER_MILLIVOLT (to units.h) and use it here?

Besides that it's seems like

	microvolts = regulator_get_voltage(data->reg_vdd);
	gain = DIV_ROUNDUP_CLOSEST(microvolts, BIT(15)) *
	       MICROVOLT_PER_MILLIVOLT / val2;

> +	if (gain <= 0 || gain > 8)
> +		return -EINVAL;

As I commented out in the previous discussion (please, give a chance to the
reviewers to answer before issuing a new version of the series) this better
to be

	if (gain < BIT(0) || gain > BIT(3))

which will show the nature of power of two implicitly.

> +	regval = ffs(gain) - 1;
> +	ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);

Can be unified in one line.

> +	return 0;
> +}

...

> +			return ads1100_set_config_bits(
> +					data, ADS1100_DR_MASK,
> +					FIELD_PREP(ADS1100_DR_MASK, i));

Wrong indentation.
Please, check all your code for this kind of issues.
Mike Looijmans March 6, 2023, 2:44 p.m. UTC | #2 
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: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 06-03-2023 14:29, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 02:13:12PM +0100, Mike Looijmans wrote:
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
> ...
>
>> +	microvolts = regulator_get_voltage(data->reg_vdd);
>> +	/*
>> +	 * val2 is in 'micro' units, n = val2 / 1000000
>> +	 * result must be millivolts, d = microvolts / 1000
>> +	 * the full-scale value is d/n, corresponds to 2^15,
>> +	 * hence the gain = (d / n) >> 15, factoring out the 1000 and moving the
>> +	 * bitshift so everything fits in 32-bits yields this formula.
>> +	 */
>> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
> Perhaps adding MICROVOLT_PER_MILLIVOLT (to units.h) and use it here?

Would that require a separate patch?

I fear that would get feedback like "why not MICROCOULOB_PER_MILLICOULOMB".

If I fill in the equation then that "1000" is actually 
MICROVOLT_PER_MICROVOLT_PER_MILLIVOLT, which would evaluate to simply 
"MILLIVOLT".


>
> Besides that it's seems like
>
> 	microvolts = regulator_get_voltage(data->reg_vdd);
> 	gain = DIV_ROUNDUP_CLOSEST(microvolts, BIT(15)) *
> 	       MICROVOLT_PER_MILLIVOLT / val2;

Yeah, the DIV_ROUNDUP_CLOSEST is more readable.


>> +	if (gain <= 0 || gain > 8)
>> +		return -EINVAL;
> As I commented out in the previous discussion (please, give a chance to the
> reviewers to answer before issuing a new version of the series) this better
> to be
>
> 	if (gain < BIT(0) || gain > BIT(3))
>
> which will show the nature of power of two implicitly.
>
>> +	regval = ffs(gain) - 1;
>> +	ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);
> Can be unified in one line.

Combining it all, I'd arrive at this code:

     gain = DIV_ROUNDUP_CLOSEST(microvolts, BIT(15)) * MILLI / val2;
     if (gain < BIT(0) || gain > BIT(3))
         return -EINVAL;

     ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);


>> +	return 0;
>> +}
> ...
>
>> +			return ads1100_set_config_bits(
>> +					data, ADS1100_DR_MASK,
>> +					FIELD_PREP(ADS1100_DR_MASK, i));
> Wrong indentation.
> Please, check all your code for this kind of issues.
>
I always run it through checkpatch.pl but that didn't report on this 
indentation.

A bit of digging in the scripts directory yields "Lindent". Feeding my 
file to that indeed changes those lines (and some others too). I'll run 
my next patch through that.
Andy Shevchenko March 6, 2023, 2:58 p.m. UTC | #3 
On Mon, Mar 06, 2023 at 03:44:23PM +0100, Mike Looijmans wrote:
> On 06-03-2023 14:29, Andy Shevchenko wrote:

...

> Combining it all, I'd arrive at this code:
> 
>     gain = DIV_ROUNDUP_CLOSEST(microvolts, BIT(15)) * MILLI / val2;
>     if (gain < BIT(0) || gain > BIT(3))
>         return -EINVAL;
> 
>     ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);

Fine by me.

...

> > > +			return ads1100_set_config_bits(
> > > +					data, ADS1100_DR_MASK,
> > > +					FIELD_PREP(ADS1100_DR_MASK, i));
> > Wrong indentation.
> > Please, check all your code for this kind of issues.
> > 
> I always run it through checkpatch.pl but that didn't report on this
> indentation.

Checkpatch is:

1) not comprehensive;
2) not must-to-follow;

tool.

Use your common sense. You have a lot of room on the previous line at least
for 'data'. Trailing opening ( is not good indentation. But sometimes might
be unavoidable.

> A bit of digging in the scripts directory yields "Lindent". Feeding my file
> to that indeed changes those lines (and some others too). I'll run my next
> patch through that.

If it will look nice, why not?
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
new file mode 100644
index 000000000000..970ccab15e1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
@@ -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 <mike.looijmans@topic.nl>
+
+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>;
+        };
+    };
+...