Message-ID: <20250825105712.3c9a3776@jic23-huawei>
Date: Mon, 25 Aug 2025 10:57:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Rob Herring <robh@...nel.org>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>, 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 Wed, 20 Aug 2025 16:03:40 -0500
Rob Herring <robh@...nel.org> wrote:

> On Sat, Aug 16, 2025 at 05:52:58PM +0100, Jonathan Cameron wrote:
> > 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.  
> 
> That sounds to me the wrong way around. A driver is free to not require 
> a property that the binding requires. They should only be not required 
> if not hooking them up on the board design is valid (which would then 
> imply a driver should be possible without them (or the h/w designer is 
> incompetent)).

I think we are saying the same thing in different ways.
The not wired being fine was what I was referring to.  That is very common
for these sorts of devices. That is the case even if the driver today does
not deal with that case and will fail to probe.  So a driver can require
an optional property if it is possible to write a driver that does not
(or alternatively make the driver more flexible in future)

Jonathan

> 
> Rob
> 

Powered by blists - more mailing lists