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

[József Horváth]

On Mon, May 03, 2021 at 11:30:08AM +0100, Jonathan Cameron wrote:

Hi Jonathan,

> On Sun, 2 May 2021 21:10:20 +0000
> József Horváth <info@xxxxxxxxxxx> wrote:
> 
> Hi József,
> 
> > >   
> > > > +
> > > > +  "#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.
> 
> Not sure I follow that.  I think we should only have these controlled via
> sysfs.  It will be a bit complex as two related controls, but that is a common situation
> and there is usually a sensible combination of options that makes sense.
> 
> For example, if we can meet the sampling frequency requested with the lower
> power oscillator we go for that.
> 

Ok, I'll do that.

> > 
> > >   
> > > > +
> > > > +  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.
> 
> Sorry, but no.  As mentioned in the driver thread, this doesn't sound like
> a characteristic of the hardware (board etc) so it doesn't belong in DT.
> It may be challenging to implement an interface that makes sense, but
> that doesn't mean we can avoid doing it.

Ok, I'll bring it to sysfs.

> 
> > 
> > > > +    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.
> 
> Understood, but that is clearly expressed by the 'required' above.
> It's easy enough to understand how to not have parts that aren't
> required without an example.

I'll bring the monitoring mode to sysfs, and in that case, the conditional requirement
 will be pointless, and interrupt property will be optional, so you are right, one example
 will be enough.

> 
> > 
> > > > +        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>;
> > > > +        };
> > > > +      };
> > > > +    };
> > > > +...
> > > > +  
> > >   
> 

Best regards
Jozsef