Re: [PATCH v1 3/3] iio:adc:ltc2471: add dt binding yaml

[Berghe, Darius]

On Sat, 2020-06-20 at 16:31 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 17 Jun 2020 16:35:23 +0300
> Darius Berghe <darius.berghe@xxxxxxxxxx> wrote:
> 
> > Add dt binding documentation for ltc2471 driver. This covers all supported
> > devices.
> > 
> > Signed-off-by: Darius Berghe <darius.berghe@xxxxxxxxxx>
> A few things inline but basically fine.
> 
> We should however also think about documenting power supplies.
> Even though the driver doesn't currently control the binding should
> be as complete as possible.
> 
> Jonathan
> 
Hi Jonathan,

And thanks for the review !

This chips have a fixed internal vref of 1.25V that is output on the REFOUT pin, there is no place for configuration here. Or perhaps did you mean the VCC (2.7V-5.5V) ? I'm not sure what the added value would be to add vref-supply and vcc-supply to yaml if they are not implemented. I find it confusing.

> > ---
> >  .../bindings/iio/adc/adi,ltc2471.yaml         | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ltc2471.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ltc2471.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ltc2471.yaml
> > new file mode 100644
> > index 000000000000..0b84e14ec984
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ltc2471.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2020 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/bindings/iio/adc/adi,ltc2471.yaml*__;Iw!!A3Ni8CS0y2Y!vUpDwSslcaNrc3db6AQ6x3gzYHbR_WxOtQyPinkeZCjgpiQ4elEbjMzDs1OGEYZou4E$ 
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!vUpDwSslcaNrc3db6AQ6x3gzYHbR_WxOtQyPinkeZCjgpiQ4elEbjMzDs1OG4cmRuW4$ 
> > +
> > +title: Analog Devices LTC2471 16-bit I2C Sigma-Delta ADC
> > +
> > +maintainers:
> > +  - Mike Looijmans <mike.looijmans@xxxxxxxx>
> > +
> > +description: |
> > +  Analog Devices LTC2471 (single-ended) and LTC2473 (differential) 16-bit
> > +  I2C Sigma-Delta ADC with selectable 208/833sps output rate.
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/24713fb.pdf
> > +
> > +  Analog Devices LTC2461 (single-ended) and LTC2463 (differential) 16-bit
> > +  I2C Sigma-Delta ADC with 60sps output rate.
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/24613fa.pdf
> 
> Put these two blocks in numeric order.  If we end up adding a bunch more
> devices it will be much more consistent if they are order.
> 

Ack, will do.

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ltc2471
> > +      - adi,ltc2473
> > +      - adi,ltc2461
> > +      - adi,ltc2463
> 
> Put them in numeric order.
> 

Ack, will do.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    i2c0 {
> > +      ltc2461@14 {
> 
> Should use a generic name
> adc@14
> 

Ack, will do.

> > +        compatible = "ltc2461";
> > +        reg = <0x14>;
> > +      };
> > +    };
> > +  - |
> > +    i2c0 {
> 
> Not a lot of point in two examples given how similar they are.
> I'd just keep the one. 
> 

Ack, will do.
I only chose to give two examples because the chip has 2 possible I2C slave addresses 0x14 and 0x54 depending on the AO pin value being low or high. But you're right, they're too simple and similar.

Best regards,
Darius

> > +      ltc2473@54 {
> > +        compatible = "ltc2473";
> > +        reg = <0x54>;
> > +      };
> > +    };
> > +