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

[Jonathan Cameron]

On Mon, 22 Jun 2020 08:05:13 +0000
"Berghe, Darius" <Darius.Berghe@xxxxxxxxxx> wrote:

> 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) ?

Yes, VCC was what I was referring to.

> 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 are intended to describe the hardware rather than what we've implemented
support for in the driver.   It's fairly likely that we will end up supporting
regulator control sooner or later (tends to happen if a driver is getting much use
as someone will care about powering down the supplies when suspending etc).

So ideally we'd add support to the driver, but even if it's not there we can consider
adding it to the binding docs. However, it's not (to my mind) vital.

> 
> > > ---
> > >  .../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.

Agreed

Thanks,

Jonathan

> 
> Best regards,
> Darius
> 
> > > +      ltc2473@54 {
> > > +        compatible = "ltc2473";
> > > +        reg = <0x54>;
> > > +      };
> > > +    };
> > > +