linux-kernel - Re: [PATCH v4 3/5] dt-bindings: iio: adc: add ade9000

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250816175258.42286693@jic23-huawei>
Date: Sat, 16 Aug 2025 17:52:58 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: <robh@...nel.org>, <conor+dt@...nel.org>, <linux-iio@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] dt-bindings: iio: adc: add ade9000

On Fri, 15 Aug 2025 09:56:36 +0000
Antoniu Miclaus <antoniu.miclaus@...log.com> wrote:

> Add devicetree bindings support for ade9000.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>

Hi Antoniu,
Sorry I missed v3 last week. Garage door crisis ate up my review time!

A few minor comments inline.

Jonathan

> ---
> changes in v4:
>  - improve description formatting (remove unnecessary pipe symbols)
>  - move $ref to end and remove allOf section for cleaner structure
>  .../bindings/iio/adc/adi,ade9000.yaml         | 108 ++++++++++++++++++
>  1 file changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ade9000.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ade9000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ade9000.yaml
> new file mode 100644
> index 000000000000..bd374c0d57d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ade9000.yaml
> @@ -0,0 +1,108 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2025 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ade9000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADE9000 High Performance, Polyphase Energy Metering driver
> +
> +maintainers:
> +  - Antoniu Miclaus <antoniu.miclaus@...log.com>
> +
> +description: |
> +  The ADE9000 s a highly accurate, fully integrated, multiphase energy and power

is a 

> +  quality monitoring device. Superior analog performance and a digital signal
> +  processing (DSP) core enable accurate energy monitoring over a wide dynamic
> +  range. An integrated high end reference ensures low drift over temperature
> +  with a combined drift of less than ±25 ppm/°C maximum for the entire channel
> +  including a programmable gain amplifier (PGA) and an analog-to- digital
analog-to-digital

> +  converter (ADC).
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ADE9000.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ade9000
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  interrupts:
> +    maxItems: 3
> +
> +  interrupt-names:
> +    items:
> +      - const: irq0
> +      - const: irq1
> +      - const: dready

I always forget how these work.  Does this allow me to say irq1 and dready
are wired but not irq0? 

Similar to question on interrupts being required below, if it is plausible
the driver could be modified to work with a lesser set, the binding should allow
it.

> +
> +  reset-gpios:
> +    description:
> +      Must be the device tree identifier of the RESET pin. As the line is
> +      active low, it should be marked GPIO_ACTIVE_LOW.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  vref-supply: true
> +
> +  clocks:
> +    description: External clock source when not using crystal
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: clkin
> +
> +  "#clock-cells":
> +    description:
> +      ADE9000 can provide clock output via CLKOUT pin with external buffer.
> +    const: 0
> +
> +  clock-output-names:
> +    items:
> +      - const: clkout
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios

As with interrupts, can we not use it at all if the reset line is tied
to not reset?   Or is it a driver limitation (which is fine to have but shouldn't
affect the binding).

> +  - interrupts
> +  - interrupt-names
My usual question on interrupts.  Is the device completely useless without them or
is it just the case that we currently require them in the driver because we don't
poll for completion as an alternative?  Fine to require them in the driver even
if the binding doesn't require them.

> +  - vdd-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +          compatible = "adi,ade9000";
> +          reg = <0>;
> +          spi-max-frequency = <7000000>;
> +
> +          #clock-cells = <0>;
> +          reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> +          interrupts = <2 IRQ_TYPE_EDGE_FALLING>, <3 IRQ_TYPE_EDGE_FALLING>, <4 IRQ_TYPE_EDGE_FALLING>;
> +          interrupt-names = "irq0", "irq1", "dready";
> +          interrupt-parent = <&gpio>;
> +          /* Optional: external clock instead of crystal */
> +          /* clocks = <&ext_clock_24576khz>; */
> +          /* clock-names = "clkin"; */
It's an example so pick one of them - if anyone wants to know what else works they can
look at the binding.  If there is something sufficiently unusual to be non obvious, have
a second example. Having stuff as comment in here is untestable and not particularly
easy to find.

> +          clock-output-names = "clkout";
> +          vdd-supply = <&vdd_reg>;
> +      };
> +    };