| Message ID | 20210506150637.35288-1-krzysztof.kozlowski@canonical.com |
|---|---|
| State | Accepted |
| Commit | 347978983b3453bc4d5a917ea34d1cd53b5fea08 |
| Headers | show |
| Series | dt-bindings: iio: afe: current-sense-shunt: add io-channel-cells | expand |
On Thu, 06 May 2021 11:06:37 -0400, Krzysztof Kozlowski wrote: > The current-sense-shunt is an IIO provider thus can be referenced by IIO > consumers (via "io-channels" property in consumer device node). > Such provider is required to describe number of cells used in phandle > lookup with "io-channel-cells" property. This also fixes dtbs_check > warnings like: > > arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: > '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' > > Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
Hi! On 2021-05-06 17:06, Krzysztof Kozlowski wrote: > The current-sense-shunt is an IIO provider thus can be referenced by IIO > consumers (via "io-channels" property in consumer device node). > Such provider is required to describe number of cells used in phandle > lookup with "io-channel-cells" property. This also fixes dtbs_check > warnings like: > > arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: > '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' > > Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > index 90439a8dc785..05166d8a3124 100644 > --- a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > @@ -24,12 +24,16 @@ properties: > description: | > Channel node of a voltage io-channel. > > + "#io-channel-cells": > + const: 0 > + > shunt-resistor-micro-ohms: > description: The shunt resistance. > > required: > - compatible > - io-channels > + - "#io-channel-cells" > - shunt-resistor-micro-ohms I know I'm listed as maintainer and all, but I have not kept up with the yaml conversion. Sorry. So, given that I might very well fundamentally misunderstand something, it does not sound correct that #io-channel-cells is now "required". I regard it as optional, and only needed if some other in-kernel driver is consuming the sensed current. What am I missing? Also, whatever is done in this binding should preferably also be done in the two "sister" afe bindings, i.e. current-sense-amplifier and voltage-divider. Cheers, Peter > additionalProperties: false > @@ -57,6 +61,7 @@ examples: > sysi { > compatible = "current-sense-shunt"; > io-channels = <&tiadc 0>; > + #io-channel-cells = <0>; > > /* Divide the voltage by 3300000/1000000 (or 3.3) for the current. */ > shunt-resistor-micro-ohms = <3300000>; >
On Sat, 8 May 2021 00:44:58 +0200 Peter Rosin <peda@axentia.se> wrote: > Hi! > > On 2021-05-06 17:06, Krzysztof Kozlowski wrote: > > The current-sense-shunt is an IIO provider thus can be referenced by IIO > > consumers (via "io-channels" property in consumer device node). > > Such provider is required to describe number of cells used in phandle > > lookup with "io-channel-cells" property. This also fixes dtbs_check > > warnings like: > > > > arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: > > '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' > > > > Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > > --- > > .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > > index 90439a8dc785..05166d8a3124 100644 > > --- a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > > +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > > @@ -24,12 +24,16 @@ properties: > > description: | > > Channel node of a voltage io-channel. > > > > + "#io-channel-cells": > > + const: 0 > > + > > shunt-resistor-micro-ohms: > > description: The shunt resistance. > > > > required: > > - compatible > > - io-channels > > + - "#io-channel-cells" > > - shunt-resistor-micro-ohms > > I know I'm listed as maintainer and all, but I have not kept up with the yaml > conversion. Sorry. So, given that I might very well fundamentally misunderstand > something, it does not sound correct that #io-channel-cells is now "required". > I regard it as optional, and only needed if some other in-kernel driver is > consuming the sensed current. What am I missing? > Agreed. This should be optional and I have deliberately not introduced it into all the bindings that could in theory support being used as providers. So far I've not pushed it out in a blanket fashion into existing bindings even as optional. > Also, whatever is done in this binding should preferably also be done in the > two "sister" afe bindings, i.e. current-sense-amplifier and voltage-divider. This particular case is squashing an error, so whilst I'm happy to have those gain the binding addition, I would like to see them in a separate patch as less likely they'd get back ported. If Kryysztof is fine with me just dropping the required I can pick up this patch. Thanks, Jonathan > > Cheers, > Peter > > > additionalProperties: false > > @@ -57,6 +61,7 @@ examples: > > sysi { > > compatible = "current-sense-shunt"; > > io-channels = <&tiadc 0>; > > + #io-channel-cells = <0>; > > > > /* Divide the voltage by 3300000/1000000 (or 3.3) for the current. */ > > shunt-resistor-micro-ohms = <3300000>; > >
On 08/05/2021 11:59, Jonathan Cameron wrote: > On Sat, 8 May 2021 00:44:58 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> Hi! >> >> On 2021-05-06 17:06, Krzysztof Kozlowski wrote: >>> The current-sense-shunt is an IIO provider thus can be referenced by IIO >>> consumers (via "io-channels" property in consumer device node). >>> Such provider is required to describe number of cells used in phandle >>> lookup with "io-channel-cells" property. This also fixes dtbs_check >>> warnings like: >>> >>> arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: >>> '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' >>> >>> Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>> --- >>> .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml >>> index 90439a8dc785..05166d8a3124 100644 >>> --- a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml >>> +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml >>> @@ -24,12 +24,16 @@ properties: >>> description: | >>> Channel node of a voltage io-channel. >>> >>> + "#io-channel-cells": >>> + const: 0 >>> + >>> shunt-resistor-micro-ohms: >>> description: The shunt resistance. >>> >>> required: >>> - compatible >>> - io-channels >>> + - "#io-channel-cells" >>> - shunt-resistor-micro-ohms >> >> I know I'm listed as maintainer and all, but I have not kept up with the yaml >> conversion. Sorry. So, given that I might very well fundamentally misunderstand >> something, it does not sound correct that #io-channel-cells is now "required". >> I regard it as optional, and only needed if some other in-kernel driver is >> consuming the sensed current. What am I missing? >> > > Agreed. This should be optional and I have deliberately not introduced it > into all the bindings that could in theory support being used as providers. > > So far I've not pushed it out in a blanket fashion into existing bindings > even as optional. > >> Also, whatever is done in this binding should preferably also be done in the >> two "sister" afe bindings, i.e. current-sense-amplifier and voltage-divider. > > This particular case is squashing an error, so whilst I'm happy to have those > gain the binding addition, I would like to see them in a separate patch as > less likely they'd get back ported. > > If Kryysztof is fine with me just dropping the required I can pick up this patch. Having here required number of cells helps any DT-user to seamlessly integrate with it (e.g. with his in-tree or out-of-tree DTS, with overlays). However it also can be added with such DTS or overlay, so in general I don't mind dropping the required piece. Thanks! Best regards, Krzysztof
On Mon, 10 May 2021 08:17:17 -0400 Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > On 08/05/2021 11:59, Jonathan Cameron wrote: > > On Sat, 8 May 2021 00:44:58 +0200 > > Peter Rosin <peda@axentia.se> wrote: > > > >> Hi! > >> > >> On 2021-05-06 17:06, Krzysztof Kozlowski wrote: > >>> The current-sense-shunt is an IIO provider thus can be referenced by IIO > >>> consumers (via "io-channels" property in consumer device node). > >>> Such provider is required to describe number of cells used in phandle > >>> lookup with "io-channel-cells" property. This also fixes dtbs_check > >>> warnings like: > >>> > >>> arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: > >>> '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' > >>> > >>> Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") > >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > >>> --- > >>> .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > >>> index 90439a8dc785..05166d8a3124 100644 > >>> --- a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > >>> +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml > >>> @@ -24,12 +24,16 @@ properties: > >>> description: | > >>> Channel node of a voltage io-channel. > >>> > >>> + "#io-channel-cells": > >>> + const: 0 > >>> + > >>> shunt-resistor-micro-ohms: > >>> description: The shunt resistance. > >>> > >>> required: > >>> - compatible > >>> - io-channels > >>> + - "#io-channel-cells" > >>> - shunt-resistor-micro-ohms > >> > >> I know I'm listed as maintainer and all, but I have not kept up with the yaml > >> conversion. Sorry. So, given that I might very well fundamentally misunderstand > >> something, it does not sound correct that #io-channel-cells is now "required". > >> I regard it as optional, and only needed if some other in-kernel driver is > >> consuming the sensed current. What am I missing? > >> > > > > Agreed. This should be optional and I have deliberately not introduced it > > into all the bindings that could in theory support being used as providers. > > > > So far I've not pushed it out in a blanket fashion into existing bindings > > even as optional. > > > >> Also, whatever is done in this binding should preferably also be done in the > >> two "sister" afe bindings, i.e. current-sense-amplifier and voltage-divider. > > > > This particular case is squashing an error, so whilst I'm happy to have those > > gain the binding addition, I would like to see them in a separate patch as > > less likely they'd get back ported. > > > > If Kryysztof is fine with me just dropping the required I can pick up this patch. > > Having here required number of cells helps any DT-user to seamlessly > integrate with it (e.g. with his in-tree or out-of-tree DTS, with > overlays). However it also can be added with such DTS or overlay, so in > general I don't mind dropping the required piece. Thanks! I dropped the required and applied to the togreg branch of iio.git. Thanks, Jonathan > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml index 90439a8dc785..05166d8a3124 100644 --- a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml @@ -24,12 +24,16 @@ properties: description: | Channel node of a voltage io-channel. + "#io-channel-cells": + const: 0 + shunt-resistor-micro-ohms: description: The shunt resistance. required: - compatible - io-channels + - "#io-channel-cells" - shunt-resistor-micro-ohms additionalProperties: false @@ -57,6 +61,7 @@ examples: sysi { compatible = "current-sense-shunt"; io-channels = <&tiadc 0>; + #io-channel-cells = <0>; /* Divide the voltage by 3300000/1000000 (or 3.3) for the current. */ shunt-resistor-micro-ohms = <3300000>;
The current-sense-shunt is an IIO provider thus can be referenced by IIO consumers (via "io-channels" property in consumer device node). Such provider is required to describe number of cells used in phandle lookup with "io-channel-cells" property. This also fixes dtbs_check warnings like: arch/arm/boot/dts/s5pv210-fascinate4g.dt.yaml: current-sense-shunt: '#io-channel-cells' does not match any of the regexes: 'pinctrl-[0-9]+' Fixes: ce66e52b6c16 ("dt-bindings:iio:afe:current-sense-shunt: txt to yaml conversion.") Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- .../devicetree/bindings/iio/afe/current-sense-shunt.yaml | 5 +++++ 1 file changed, 5 insertions(+)