Re: [PATCH 07/10] dt-bindings:iio:adc:maxim,max1027: Pull out to separate binding doc.

From: Jonathan Cameron <jic23@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: linux-iio@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Philippe Reynes <tremyfr@yahoo.fr>
Subject: Re: [PATCH 07/10] dt-bindings:iio:adc:maxim,max1027: Pull out to separate binding doc.
Date: Sun, 1 Nov 2020 20:33:04 +0000	[thread overview]
Message-ID: <20201101203304.1abf4a77@archlinux> (raw)
In-Reply-To: <20201101142557.153c7b97@xps13>

On Sun, 1 Nov 2020 14:25:57 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> Jonathan Cameron <jic23@kernel.org> wrote on Sat, 31 Oct 2020 18:12:39
> +0000:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The afe/voltage-divider.yaml example uses this device with 2 properties
> > not provided by trivial-devices.yaml (spi-max-frequency and #io-channel-cells)
> > 
> > Solve that by creating a more specific binding doc.  
> 
> I don't know #io-channel-cells but spi-max-frequency is very common and
> is related to the fact that this device is a SPI device (hence, a SPI
> subnode), IMHO it has nothing to do with trivial-devices.yaml
> description.

There may be some logic to adding spi-max-frequency to the allowed properties
of devices listed in trivial-devices.  Right now it's not there so you will
get warnings if you check a dts file that has it in for bindings described
by that file.  I've been lazy here, but ideally we also want to document
a maximum for this property.  In this case, it should be 10000000 (10MHz).
I'll add that to this binding either in v2, or whilst applying.

The io-channel-cells, is specific to devices proving analog channel measurement
services that other bindings can make use of.  (typically ADCs and DACs).
Currently only applies to IIO bindings for ADCs and DACs.
It specifies how many parameters are needed as part of the phandle to identify
a channel.

Given most of trivial devices don't fall into that category, we need to do
a separate binding where it's known to be used anyway.

As a side note.. I just noticed these devices have reference voltage pins.
At somepoint (not necessarily now) we should look at adding appropriate regulator
bindings.

Jonathan

> 
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Philippe Reynes <tremyfr@yahoo.fr>
> > ---
> >  .../bindings/iio/adc/maxim,max1027.yaml       | 64 +++++++++++++++++++
> >  .../devicetree/bindings/trivial-devices.yaml  | 12 ----
> >  2 files changed, 64 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1027.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1027.yaml
> > new file mode 100644
> > index 000000000000..09e853c50c76
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1027.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1027.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Maxim MAX1027 and similar ADCs
> > +
> > +maintainers:
> > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > +  - Philippe Reynes <tremyfr@yahoo.fr>
> > +
> > +description: |
> > +  300ks/s SPI ADCs with temperature sensors.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +        # 10-bit 8 channels
> > +      - maxim,max1027
> > +        # 10-bit 12 channels
> > +      - maxim,max1029
> > +        # 10-bit 16 channels
> > +      - maxim,max1031
> > +         # 12-bit 8 channels
> > +      - maxim,max1227
> > +         # 12-bit 12 channels
> > +      - maxim,max1229
> > +         # 12-bit 16 channels
> > +      - maxim,max1231
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency: true
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +        maxadc: adc@0 {
> > +            compatible = "maxim,max1027";
> > +            reg = <0>;
> > +            #io-channel-cells = <1>;
> > +            interrupt-parent = <&gpio5>;
> > +            interrupts = <15 IRQ_TYPE_EDGE_RISING>;
> > +            spi-max-frequency = <1000000>;
> > +        };
> > +    };
> > +...
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index d154ea97e30d..185e09e61e16 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -108,18 +108,6 @@ properties:
> >            - isil,isl68137
> >              # 5 Bit Programmable, Pulse-Width Modulator
> >            - maxim,ds1050
> > -            # 10-bit 8 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1027
> > -            # 10-bit 12 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1029
> > -            # 10-bit 16 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1031
> > -            # 12-bit 8 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1227
> > -            # 12-bit 12 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1229
> > -            # 12-bit 16 channels 300ks/s SPI ADC with temperature sensor
> > -          - maxim,max1231
> >              # Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs
> >            - maxim,max1237
> >              # PECI-to-I2C translator for PECI-to-SMBus/I2C protocol conversion  
> 
> Thanks,
> Miquèl