Re: [PATCH 1/2] dt-bindings:iio:adc: add max14001 bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 14 Apr 2023 23:15:07 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> On 14/04/2023 12:28, Kim Seer Paller wrote:
> > Add bindings for MAX14001.
> > 
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.  
> 
> Subject: missing spaces between prefixes.
> 
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.

A few follow up comments inline,

Thanks,

Jonathan

> 
> > 
> > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> > ---
> >  .../bindings/iio/adc/adi,max14001.yaml        | 83 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > new file mode 100644
> > index 000000000..4546bf595
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2023 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MAX14001 ADC device driver  
> 
> Drop device driver. Bindings are for hardware, not Linux drivers.
> 
> > +
> > +maintainers:
> > +  - Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> > +
> > +description: |
> > +    Single channel 10 bit ADC with SPI interface. Datasheet
> > +    can be found here:
> > +      https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max14001
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 5000000
> > +
> > +  vref-supply:
> > +    description: Voltage reference to establish input scaling.
> > +
> > +  adi,use-fadc:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    type: boolean  
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Keep one.
> 
> > +    description: If set, the filtered ADC data (FADC register) will be read,
> > +                  otherwise the unfiltered ADC data (ADC register) will be read.  
> 
> Hmmmm, looks familiar. Don't we have existing property for this?

That should be a userspace decision, not a DT provided one.
We have a bunch of controls defined for controlling filters. Ideal is probably
to map it to that ABI. It's possible that isn't flexible enough though for
this case (I haven't dived into datasheet to find out1). If so propose ABI
additions with documentation in Documentation/ABI/testing/sysfs-bus-iio

> 
> > +
> > +  adi,inrush-mode:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    type: boolean
> > +    description: If set, the device will use FAST inrush mode,
> > +                  otherwise the device will use ADC controlled inrush mode.
> > +
> > +  adi,filter:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 0, 1, 2, 3 ]
> > +    description: |  
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +      0: Filtering off
> > +      1: Average 2 readings
> > +      2: Average 4 readings
> > +      3: Average 8 readings  
> 
> Isn't this also matching existing property for number of sample averaging?

Another one that belongs in the userspace ABI with driver picking a reaonable
default.  In IIO terms it's either a low pass filter, or oversampling depending
on exactly what the maths is. Low pass filter if next reading include elements
of the previous one (so a moving window). Oversampling if frequency of available
readings drops such that each real reading contributes to only one output.

If you need this stuff in the DT binding we need a strong argument for why
this is a feature of the analog signals being sampled, rather than trade
offs being made over noise etc.
 
> 
> > +
> > +  adi,current-source:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    type: boolean
> > +    description: If set, the 70uA current source will be connected to the REFIN pin,
> > +                  otherwise the current source will be turned off.
This is unusual enough that I wonder if a more specific name is needed.

adi,current-source-to-refin or maybe adi,current-source-for-shunt-volt-ref

I'm not an expert in these, so perhaps others have better suggestions for
how to describe this in a compact form.

adi,current-source alone could mean any number of things that aren't what we have
here.

Jonathan





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux