Re: [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver

From: József Horváth
Date: Sun May 02 2021 - 17:11:15 EST

On Sun, May 02, 2021 at 06:22:55PM +0100, Jonathan Cameron wrote:
> On Sat, 1 May 2021 18:25:18 +0000
> Jozsef Horvath <info@xxxxxxxxxxx> wrote:
>
> > This is a device tree schema for iio driver for
> > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> >
> > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> >
> > Signed-off-by: Jozsef Horvath <info@xxxxxxxxxxx>
> > ---
> > ---
> > .../bindings/iio/adc/ti,ads7142.yaml | 198 ++++++++++++++++++
> > 1 file changed, 198 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> > new file mode 100644
> > index 000000000000..b4e752160156
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> > @@ -0,0 +1,198 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/iio/adc/ti,ads7142.yaml#";
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Texas Instruments ADS7142 adc driver device tree bindings
> > +
> > +maintainers:
> > + - József Horváth <info@xxxxxxxxxxx>
> > +
> > +description: |
> > + This document is for describing the required device tree parameters
> > + for ads7142 adc driver.
>
> Document describes hardware, not a particular driver. So just refer
> to the device here. There may well be other drives in future using
> the same binding (e.g. in an RTOS).
>
> > + The required parameters for proper operation are described below.
> > +
> > + Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> > +
> > + Operation modes supportedby the driver:
> > + When the 'ti,monitoring-mode' property is not present
> > + in the devicetree node definition, the driver initiates a single
> > + conversion in the device for each read request
> > + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > + This is a one-shot conversion, and it is called
> > + "Manual Mode" in the datasheet.
> > +
> > + When the 'ti,monitoring-mode' property is present
> > + in the devicetree node definition, the driver configures
> > + the device's digital window comparator and sets the device's
> > + data buffer operation mode to pre alert data mode.
> > + The driver reads the conversion result when the BUSY/RDY interrupt
> > + fires, and keeps the value until the next BUSY/RDY interrupt
> > + or the first read request
> > + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > + The digital window comparator and hysteresis parameters
> > + can be controlled by:
> > + - the devicetree definition of channel node
> > + - iio sysfs interfaces
> > + This is event driven conversion, and is called
> > + "Autonomous Mode with Pre Alert Data" in the datasheet.
> > + This mode can be used to wake up the system with the ALERT pin,
> > + in case when the monitored voltage level is out of the configured range.
>
> I talked about these in the driver review so look there for comments.
> Short summary is this is something userspace should have control off (assuming irq
> is wired up) not dt.
>
> > +
> > +properties:
> > + compatible:
> > + const: ti,ads7142
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description: |
> > + The BUSY/PDY pin is used as interrupt line in autonomous monitoring mode.
> > + maxItems: 1
> > +
> > + vref-supply:
> > + description: Regulator for the reference voltage
> > +
> > + power-supply: true
>
> These don't match the naming on the pin diagram.

I'll cange it to dvdd, and vref to avdd.

>
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + "#io-channel-cells":
> > + const: 1
> > +
> > + ti,osc-sel:
> > + description: |
> > + If present, the driver selects the high speed oscillator.
> > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> > + type: boolean
>
> This looks connected to the possible sampling frequencies when in various autonomous modes.
> Should it be controlled by userspace?

The sampling frequency is controlled with the osc-sel and n-clk.
I'll remove n-clk from sysfs.

>
> > +
> > + ti,n-clk:
> > + description: |
> > + nCLK is number of clocks in one conversion cycle.
> > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
>
> Sounds like a policy decision for userspace.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 255
> > + minimum: 0
> > +
> > + ti,monitoring-mode:
> > + description: |
> > + If present, the driver selects the autonomous monitoring mode with pre alert data.
> > + See chapter 7.4 Device Functional Modes in datasheet.
>
> As mentioned in the driver review, this looks like something we should control from userspace
> not dt to me.
>

I would keep this here, but it will be an enum.

> > + type: boolean
> > +
> > +patternProperties:
> > + "^channel@[0-1]$":
> > + $ref: "adc.yaml"
> > + type: object
> > + description: |
> > + Represents the external channels which are connected to the ADC.
> > + properties:
> > + reg:
> > + description: |
> > + The channel number.
> > + items:
> > + minimum: 0
> > + maximum: 1
> > + "ti,threshold-falling":
> > + description: The low threshold for channel
>
> For these, we need a strong argument presented in this doc for why they are not
> a question of policy (and hence why they should be in dt at all).

I'll remove all threshold and hysteresys from dt.

>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 4095
> > + minimum: 0
> > + "ti,threshold-rising":
> > + description: The high threshold for channel
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 4095
> > + minimum: 0
> > + "ti,hysteresis":
> > + description: The hysteresis for both comparators for channel
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 63
> > + minimum: 0
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > +allOf:
> > + - if:
> > + required:
> > + - ti,monitoring-mode
> > + then:
> > + required:
> > + - interrupts
> > +
> > +required:
> > + - compatible
> > + - "#io-channel-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + adc@18 {
>
> I would not bother having two examples. The second one covers more things afterall
> and the binding makes it clear what is required.
>

I do this because of the conditional requirement of interrupts.

> > + compatible = "ti,ads7142";
> > + reg = <0x18>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + #io-channel-cells = <1>;
> > +
> > + vref-supply = <&vdd_3v3_reg>;
> > + power-supply = <&vdd_1v8_reg>;
> > +
> > + channel@0 {
> > + reg = <0>;
> > + };
> > +
> > + channel@1 {
> > + reg = <1>;
> > + };
> > + };
> > + };
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + adc@1f {
> > + compatible = "ti,ads7142";
> > + reg = <0x1f>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + #io-channel-cells = <1>;
> > +
> > + vref-supply = <&vdd_3v3_reg>;
> > + power-supply = <&vdd_1v8_reg>;
> > +
> > + interrupt-parent = <&gpio>;
> > + interrupts = <7 2>;
> > +
> > + ti,monitoring-mode;
> > +
> > + channel@0 {
> > + reg = <0>;
> > + ti,threshold-falling = <1000>;
> > + ti,threshold-rising = <2000>;
> > + ti,hysteresis = <20>;
> > + };
> > +
> > + channel@1 {
> > + reg = <1>;
> > + ti,threshold-falling = <100>;
> > + ti,threshold-rising = <2500>;
> > + ti,hysteresis = <0>;
> > + };
> > + };
> > + };
> > +...
> > +
>