Message-ID: <8e88b601-1329-4cdb-bbd7-feb998c552e8@baylibre.com>
Date: Tue, 16 Sep 2025 11:40:47 -0500
From: David Lechner <dlechner@...libre.com>
To: Marilene Andrade Garcia <marilene.agarcia@...il.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Cc: Kim Seer Paller <kimseer.paller@...log.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 Marcelo Schmitt <Marcelo.Schmitt@...log.com>,
 Ceclan Dumitru <dumitru.ceclan@...log.com>,
 Jonathan Santos <Jonathan.Santos@...log.com>,
 Dragos Bogdan <dragos.bogdan@...log.com>
Subject: Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

> Notes:
> Since v10, I have not used exactly the same approach as Kim did in v9, nor 
> the same approach as in my v1. Instead, I merged both implementations, and 
> this v11 is quite different from both. Therefore, I have dropped the review 
> by Krzysztof Kozlowski. I am not very familiar with the kernel’s review 
> process, should I add it back? Should I list your names as Reviewed-by? 
> Thanks.

I think you made the right judgement call here. The changes are significant
enough to deserve another look. And you did the right thing by explaining why
you dropped the review tags.

> 
> The MAX14001 and MAX14002 both have the COUT output pin and the FAULT
> output pin, and work the same. I have decided to declare them as interrupts
> because I think some action should be done when they are hit. However, the
> implementation of these features is not present in the v11 driver code, as
> it was not in v9. But I plan to submit it in the next steps.

The devicetree bindings should be as complete as possible and not care
if the driver doesn't use everything. So adding them now is the right
thing to do.

> 
> 
>  .../bindings/iio/adc/adi,max14001.yaml        | 87 +++++++++++++++++++
>  MAINTAINERS                                   |  8 ++
>  2 files changed, 95 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 000000000000..c61119b16cf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023-2025 Analog Devices Inc.
> +# Copyright 2023 Kim Seer Paller
> +# Copyright 2025 Marilene Andrade Garcia
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX14001-MAX14002 ADC
> +
> +maintainers:
> +  - Kim Seer Paller <kimseer.paller@...log.com>
> +  - Marilene Andrade Garcia <marilene.agarcia@...il.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
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: adi,max14002
> +      - items:
> +          - const: adi,max14001
> +          - const: adi,max14002
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 5000000
> +
> +  vdd-supply:
> +    description:
> +      Isolated DC-DC power supply input voltage.
> +
> +  vddl-supply:
> +    description:
> +      Logic power supply.
> +
> +  refin-supply:
> +    description:
> +      ADC voltage reference supply.
> +
> +  interrupts:
> +    minItems: 1
> +    items:
> +      - description: |
> +          Asserts high when ADC readings exceed the upper threshold and low
> +          when below the lower threshold. Must be connected to the COUT pin.
> +      - description: |
> +          Alert output that asserts low during a number of different error
> +          conditions. The interrupt source must be attached to FAULT pin.
> +

I don't think the `|` is needed here. It is only needed when formatting
should be preserved, e.g. you have paragraphs or a list. Or when there
are certain characters like `:` in the text. But neither are the case here.

> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: cout
> +      - const: fault
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - vddl-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      max14001: adc@0 {
> +        compatible = "adi,max14001", "adi,max14002";
> +        reg = <0>;
> +        spi-max-frequency = <5000000>;

I would add:

           spi-lsb-first;

to this example so that people know it is a possibility.

I don't think we need to list it in the properties though since
it is included by $ref: /schemas/spi/spi-peripheral-props.yaml#.

> +        vdd-supply = <&vdd>;
> +        vddl-supply = <&vddl>;
> +      };
> +    };
> +...

Powered by blists - more mailing lists