| Message ID | 20200320150114.9297-2-alazar@startmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Maxim MAX1241 driver | expand |
On Fri, 20 Mar 2020 17:01:14 +0200 Alexandru Lazar <alazar@startmail.com> wrote: > Add device-tree bindings documentation for the MAX1241 device driver. > > Signed-off-by: Alexandru Lazar <alazar@startmail.com> Please consider also adding the vdd-supply. It's not really required, but if you don't add it from the start chances are high that at some point someone else will need to add it. One trivial thing inline. Otherwise looks good to me. > --- > .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > new file mode 100644 > index 000000000000..de41d422ce3b > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright 2020 Ioan-Alexandru Lazar > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter > + > +maintainers: > + - Ioan-Alexandru Lazar <alazar@startmail.com> > + > +description: | > + Bindings for the max1241 12-bit, single-channel ADC device. This > + driver supports voltage reading and can optionally be configured for Driver shouldn't be mentioned in the binding. It's a description of the hardware only. > + power-down mode operation. The datasheet can be found at: > + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > + > +properties: > + compatible: > + enum: > + - maxim,max1241 > + > + reg: > + maxItems: 1 > + > + vref-supply: > + description: > + Device tree identifier of the regulator that provides the external > + reference voltage. > + maxItems: 1 > + > + shdn-gpios: > + description: > + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If > + specified, the /SHDN pin will be asserted between conversions, > + thus enabling power-down mode. > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - vref-supply > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + spi0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "maxim,max1241"; > + reg = <0>; > + vref-supply = <&vdd_3v3_reg>; > + spi-max-frequency = <1000000>; > + shdn-gpios = <&gpio 26 1>; > + }; > + }; > + > +
Hi Jonathan, > Please consider also adding the vdd-supply. > It's not really required, but if you don't add it from the start > chances are high that at some point someone else will need to > add it. Sorry if I'm missing something obvious here -- what vdd-supply is that? Are you thinking of the regulator used for the ADC's reference voltage? That's already there (vref-supply). Or did you mean I should add a definition for the regulator output used for the device's Vdd input (i.e. the positive supply voltage)? Needless to say, I'm happy to add it if you think it's a good idea. It's just I don't think I've seen it in other drivers (except maybe ad7192?) -- so I figured I'd ask before sending a botched v5. Best regards, Alex > > One trivial thing inline. Otherwise looks good to me. > > > --- > > .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > new file mode 100644 > > index 000000000000..de41d422ce3b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > @@ -0,0 +1,61 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright 2020 Ioan-Alexandru Lazar > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter > > + > > +maintainers: > > + - Ioan-Alexandru Lazar <alazar@startmail.com> > > + > > +description: | > > + Bindings for the max1241 12-bit, single-channel ADC device. This > > + driver supports voltage reading and can optionally be configured for > > Driver shouldn't be mentioned in the binding. It's a description of the > hardware only. > > > + power-down mode operation. The datasheet can be found at: > > + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - maxim,max1241 > > + > > + reg: > > + maxItems: 1 > > + > > + vref-supply: > > + description: > > + Device tree identifier of the regulator that provides the external > > + reference voltage. > > + maxItems: 1 > > + > > + shdn-gpios: > > + description: > > + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If > > + specified, the /SHDN pin will be asserted between conversions, > > + thus enabling power-down mode. > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - vref-supply > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + spi0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adc@0 { > > + compatible = "maxim,max1241"; > > + reg = <0>; > > + vref-supply = <&vdd_3v3_reg>; > > + spi-max-frequency = <1000000>; > > + shdn-gpios = <&gpio 26 1>; > > + }; > > + }; > > + > > + >
On Sat, 2020-03-21 at 21:35 +0200, Alexandru Lazar wrote: > Hi Jonathan, > > > Please consider also adding the vdd-supply. > > It's not really required, but if you don't add it from the start > > chances are high that at some point someone else will need to > > add it. > > Sorry if I'm missing something obvious here -- what vdd-supply is > that? Are you thinking of the regulator used for the ADC's reference > voltage? That's already there (vref-supply). > > Or did you mean I should add a definition for the regulator output used > for the device's Vdd input (i.e. the positive supply voltage)? Needless > to say, I'm happy to add it if you think it's a good idea. It's just I > don't think I've seen it in other drivers (except maybe ad7192?) -- so I > figured I'd ask before sending a botched v5. > Yep. Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF pin]. Not all drivers define Vdd. Some call it AVdd. You can check via: git grep -i vdd | cut -d: -f1 | sort | uniq -c in the drivers/iio folder It's an idea to add it, and that can give control to the driver to power-up the ADC, by defining a regulator [vdd-supply] in the device-tree. Maybe it could be interesting to move this to the IIO core as an option-flag. [But that's another discussion] > Best regards, > Alex > > > One trivial thing inline. Otherwise looks good to me. > > > > > --- > > > .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > new file mode 100644 > > > index 000000000000..de41d422ce3b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > @@ -0,0 +1,61 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright 2020 Ioan-Alexandru Lazar > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter > > > + > > > +maintainers: > > > + - Ioan-Alexandru Lazar <alazar@startmail.com> > > > + > > > +description: | > > > + Bindings for the max1241 12-bit, single-channel ADC device. This > > > + driver supports voltage reading and can optionally be configured for > > > > Driver shouldn't be mentioned in the binding. It's a description of the > > hardware only. > > > > > + power-down mode operation. The datasheet can be found at: > > > + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - maxim,max1241 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + vref-supply: > > > + description: > > > + Device tree identifier of the regulator that provides the external > > > + reference voltage. > > > + maxItems: 1 > > > + > > > + shdn-gpios: > > > + description: > > > + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If > > > + specified, the /SHDN pin will be asserted between conversions, > > > + thus enabling power-down mode. > > > + maxItems: 1 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - vref-supply > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/gpio/gpio.h> > > > + spi0 { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + adc@0 { > > > + compatible = "maxim,max1241"; > > > + reg = <0>; > > > + vref-supply = <&vdd_3v3_reg>; > > > + spi-max-frequency = <1000000>; > > > + shdn-gpios = <&gpio 26 1>; > > > + }; > > > + }; > > > + > > > +
> Yep. > Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF > pin]. > Not all drivers define Vdd. > Some call it AVdd. > > [...] > > It's an idea to add it, and that can give control to the driver to power-up the > ADC, by defining a regulator [vdd-supply] in the device-tree. Hmm... I don't know how useful this would be for the 124x family (I doubt anyone who needs one of these will power it from its own, independent supply), but it's a pretty harmless change. I can't think of any reason to say no :-). Thanks, Alex
On Sun, 22 Mar 2020 09:02:15 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Sat, 2020-03-21 at 21:35 +0200, Alexandru Lazar wrote: > > Hi Jonathan, > > > > > Please consider also adding the vdd-supply. > > > It's not really required, but if you don't add it from the start > > > chances are high that at some point someone else will need to > > > add it. > > > > Sorry if I'm missing something obvious here -- what vdd-supply is > > that? Are you thinking of the regulator used for the ADC's reference > > voltage? That's already there (vref-supply). > > > > Or did you mean I should add a definition for the regulator output used > > for the device's Vdd input (i.e. the positive supply voltage)? Needless > > to say, I'm happy to add it if you think it's a good idea. It's just I > > don't think I've seen it in other drivers (except maybe ad7192?) -- so I > > figured I'd ask before sending a botched v5. > > > > Yep. > Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF > pin]. > Not all drivers define Vdd. > Some call it AVdd. > > You can check via: git grep -i vdd | cut -d: -f1 | sort | uniq -c > in the drivers/iio folder > > It's an idea to add it, and that can give control to the driver to power-up the > ADC, by defining a regulator [vdd-supply] in the device-tree. > > Maybe it could be interesting to move this to the IIO core as an option-flag. > [But that's another discussion] Can't easily move it to the core because how you actually handle it is very much driver dependent. A devices that has state will keep it on most of the time, but other drivers will handle it in runtime pm if the device starts up quickly enough. We want this to be visible in individual drivers, even if it would be less code perhaps in the core. Jonathan > > > Best regards, > > Alex > > > > > One trivial thing inline. Otherwise looks good to me. > > > > > > > --- > > > > .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ > > > > 1 file changed, 61 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > new file mode 100644 > > > > index 000000000000..de41d422ce3b > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > > > @@ -0,0 +1,61 @@ > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +# Copyright 2020 Ioan-Alexandru Lazar > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter > > > > + > > > > +maintainers: > > > > + - Ioan-Alexandru Lazar <alazar@startmail.com> > > > > + > > > > +description: | > > > > + Bindings for the max1241 12-bit, single-channel ADC device. This > > > > + driver supports voltage reading and can optionally be configured for > > > > > > Driver shouldn't be mentioned in the binding. It's a description of the > > > hardware only. > > > > > > > + power-down mode operation. The datasheet can be found at: > > > > + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - maxim,max1241 > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + vref-supply: > > > > + description: > > > > + Device tree identifier of the regulator that provides the external > > > > + reference voltage. > > > > + maxItems: 1 > > > > + > > > > + shdn-gpios: > > > > + description: > > > > + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If > > > > + specified, the /SHDN pin will be asserted between conversions, > > > > + thus enabling power-down mode. > > > > + maxItems: 1 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - vref-supply > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/gpio/gpio.h> > > > > + spi0 { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + adc@0 { > > > > + compatible = "maxim,max1241"; > > > > + reg = <0>; > > > > + vref-supply = <&vdd_3v3_reg>; > > > > + spi-max-frequency = <1000000>; > > > > + shdn-gpios = <&gpio 26 1>; > > > > + }; > > > > + }; > > > > + > > > > +
On Sun, 22 Mar 2020 11:53:17 +0200 Alexandru Lazar <alazar@startmail.com> wrote: > > Yep. > > Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF > > pin]. > > Not all drivers define Vdd. > > Some call it AVdd. > > > > [...] > > > > It's an idea to add it, and that can give control to the driver to power-up the > > ADC, by defining a regulator [vdd-supply] in the device-tree. > > Hmm... I don't know how useful this would be for the 124x family (I > doubt anyone who needs one of these will power it from its own, > independent supply), You'd be surprised how often this gets added to drivers precisely because people will put it on a controllable supply. It may well not have it's own supply but it may share one with a bunch of other external chips and all of them need to use the regulator framework controls to make sure it's only disabled when they are all suspended etc. See the number of times Linus Walleij has added this for various sensors and ADCs because he has boards where the control is needed. Jonathan > but it's a pretty harmless change. I can't think of > any reason to say no :-). > > Thanks, > Alex
> You'd be surprised how often this gets added to drivers precisely because > people will put it on a controllable supply. It may well not have it's own > supply but it may share one with a bunch of other external chips and > all of them need to use the regulator framework controls to make sure it's > only disabled when they are all suspended etc. I figured it might be something like this :-). I've added the vdd-supply binding in v5. If this isn't something that can be easily handled in the core, do you think we can document it somewhere as a convention/common idiom? (Assuming it's not already documented, of course). It seems like it's something that all IIO devices would need. I can do the writing part. Thanks, Alex
On Sun, 22 Mar 2020 18:06:04 +0200 Alexandru Lazar <alazar@startmail.com> wrote: > > You'd be surprised how often this gets added to drivers precisely because > > people will put it on a controllable supply. It may well not have it's own > > supply but it may share one with a bunch of other external chips and > > all of them need to use the regulator framework controls to make sure it's > > only disabled when they are all suspended etc. > > I figured it might be something like this :-). I've added the vdd-supply > binding in v5. > > If this isn't something that can be easily handled in the core, do you > think we can document it somewhere as a convention/common idiom? > (Assuming it's not already documented, of course). It seems like it's > something that all IIO devices would need. I can do the writing part. Hmm. We could do with a sort of 'things you'd normally find in a driver' document. We don't have such a document, but interesting to think about what would be in it... Perhaps a 'best practice' document would be a better way of putting it. I don't really want to see a huge number of patches adding regulators to drivers that don't have them already for example. Clearly no one needed them yet :) If you want to take a stab at such a document that would be great. Jonathan > > Thanks, > Alex
On Fri, Mar 20, 2020 at 05:01:14PM +0200, Alexandru Lazar wrote: > Add device-tree bindings documentation for the MAX1241 device driver. > > Signed-off-by: Alexandru Lazar <alazar@startmail.com> > --- > .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > new file mode 100644 > index 000000000000..de41d422ce3b > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: GPL-2.0 Dual license new bindings please: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2020 Ioan-Alexandru Lazar > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter > + > +maintainers: > + - Ioan-Alexandru Lazar <alazar@startmail.com> > + > +description: | > + Bindings for the max1241 12-bit, single-channel ADC device. This > + driver supports voltage reading and can optionally be configured for > + power-down mode operation. The datasheet can be found at: > + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > + > +properties: > + compatible: > + enum: > + - maxim,max1241 > + > + reg: > + maxItems: 1 > + > + vref-supply: > + description: > + Device tree identifier of the regulator that provides the external > + reference voltage. > + maxItems: 1 Drop this. Supplies are always 1 item. > + > + shdn-gpios: shutdown-gpios is the semi standard name. > + description: > + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If > + specified, the /SHDN pin will be asserted between conversions, > + thus enabling power-down mode. > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - vref-supply > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + spi0 { Just 'spi' > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "maxim,max1241"; > + reg = <0>; > + vref-supply = <&vdd_3v3_reg>; > + spi-max-frequency = <1000000>; > + shdn-gpios = <&gpio 26 1>; > + }; > + }; > + > + > -- > 2.25.2 >
diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml new file mode 100644 index 000000000000..de41d422ce3b --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright 2020 Ioan-Alexandru Lazar +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim MAX1241 12-bit, single-channel analog to digital converter + +maintainers: + - Ioan-Alexandru Lazar <alazar@startmail.com> + +description: | + Bindings for the max1241 12-bit, single-channel ADC device. This + driver supports voltage reading and can optionally be configured for + power-down mode operation. The datasheet can be found at: + https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf + +properties: + compatible: + enum: + - maxim,max1241 + + reg: + maxItems: 1 + + vref-supply: + description: + Device tree identifier of the regulator that provides the external + reference voltage. + maxItems: 1 + + shdn-gpios: + description: + GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If + specified, the /SHDN pin will be asserted between conversions, + thus enabling power-down mode. + maxItems: 1 + +required: + - compatible + - reg + - vref-supply + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + spi0 { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "maxim,max1241"; + reg = <0>; + vref-supply = <&vdd_3v3_reg>; + spi-max-frequency = <1000000>; + shdn-gpios = <&gpio 26 1>; + }; + }; + +
Add device-tree bindings documentation for the MAX1241 device driver. Signed-off-by: Alexandru Lazar <alazar@startmail.com> --- .../bindings/iio/adc/maxim,max1241.yaml | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml