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
