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

Add bindings for MAX14001.

The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
binary inputs.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 .../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

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.

> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../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@analog.com>
> +
> +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?

> +
> +  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?

> +
> +  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.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "okay";

Drop status

> +
> +        adc@0 {
> +            compatible = "adi,max14001";
> +            reg = <0>;
> +            spi-max-frequency = <5000000>;
> +            vref-supply = <&vref_reg>;
> +            adi,use-fadc;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0e64787aa..766847ad2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12573,6 +12573,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/sound/max9860.txt
>  F:	sound/soc/codecs/max9860.*
>  
> +MAX14001 IIO ADC DRIVER

Are you sure you ordered it correctly?

Best regards,
Krzysztof

Hi Kim,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.3-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kim-Seer-Paller/iio-adc-add-max14001-support/20230414-183416
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20230414102844.21579-1-kimseer.paller%40analog.com
patch subject: [PATCH 1/2] dt-bindings:iio:adc: add max14001 bindings
reproduce:
        # https://github.com/intel-lab-lkp/linux/commit/8e4267ba9a592dc820ad029c5e602098ec981159
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kim-Seer-Paller/iio-adc-add-max14001-support/20230414-183416
        git checkout 8e4267ba9a592dc820ad029c5e602098ec981159
        make menuconfig
        # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
        make htmldocs

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304151615.k0j79iDf-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/iio/dac/adi,max14001.yaml

On Fri, 14 Apr 2023 23:15:07 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@analog.com>
> > ---
> >  .../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@analog.com>
> > +
> > +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