| Message ID | 20230623113327.1062170-2-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: ad74413r: allow configuring digital input threshold | expand |
On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: > Allow specifying the threshold for which the channels configured as > digital input change state. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > > Running dt_binding_check on this with a too small or large value in > the example does give me an error, but the multipleOf does not seem to > be enforced; the value 1234567 is not flagged. I don't know if that's > expected (maybe I have too old versions of something). That's one for Rob. I checked a few others and behaviour was the same there. > .../devicetree/bindings/iio/addac/adi,ad74413r.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > index 590ea7936ad7..1f90ce3c7932 100644 > --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > @@ -51,6 +51,14 @@ properties: > Shunt (sense) resistor value in micro-Ohms. > default: 100000000 > > + digital-input-threshold-microvolt: Should this not have an adi vendor prefix, similar to "adi,digital-input-threshold-mode-fixed"? Cheers, Conor. > + description: > + Comparator threshold used by the channels configured to use the > + digital input function. > + minimum: 500000 > + maximum: 16000000 > + multipleOf: 500000 > + > reset-gpios: > maxItems: 1 > > @@ -143,6 +151,8 @@ examples: > refin-supply = <&ad74413r_refin>; > reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>; > > + digital-input-threshold-microvolt = <4000000>; > + > channel@0 { > reg = <0>; > > -- > 2.37.2 >
On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: > Allow specifying the threshold for which the channels configured as > digital input change state. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > > Running dt_binding_check on this with a too small or large value in > the example does give me an error, but the multipleOf does not seem to > be enforced; the value 1234567 is not flagged. I don't know if that's > expected (maybe I have too old versions of something). Thanks for the report. Indeed, 'multipleOf' was not handled correctly. I'll push a fix to dtschema shortly. Rob
On Fri, Jun 23, 2023 at 05:44:50PM +0100, Conor Dooley wrote: > On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: > > Allow specifying the threshold for which the channels configured as > > digital input change state. > > > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > --- > > > > Running dt_binding_check on this with a too small or large value in > > the example does give me an error, but the multipleOf does not seem to > > be enforced; the value 1234567 is not flagged. I don't know if that's > > expected (maybe I have too old versions of something). > > That's one for Rob. I checked a few others and behaviour was the same > there. > > > .../devicetree/bindings/iio/addac/adi,ad74413r.yaml | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > > index 590ea7936ad7..1f90ce3c7932 100644 > > --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > > +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > > @@ -51,6 +51,14 @@ properties: > > Shunt (sense) resistor value in micro-Ohms. > > default: 100000000 > > > > + digital-input-threshold-microvolt: > > Should this not have an adi vendor prefix, similar to > "adi,digital-input-threshold-mode-fixed"? Yes. Rob
On 23/06/2023 23.57, Rob Herring wrote: > On Fri, Jun 23, 2023 at 05:44:50PM +0100, Conor Dooley wrote: >> On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: >>> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>> index 590ea7936ad7..1f90ce3c7932 100644 >>> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>> @@ -51,6 +51,14 @@ properties: >>> Shunt (sense) resistor value in micro-Ohms. >>> default: 100000000 >>> >>> + digital-input-threshold-microvolt: >> >> Should this not have an adi vendor prefix, similar to >> "adi,digital-input-threshold-mode-fixed"? > > Yes. OK. But I'm not really sure what the rules are for when such a prefix must be added, so some guidance would be appreciated. There's - DO use a vendor prefix on device specific property names. Consider if properties could be common among devices of the same class. And my thinking was that a threshold for when a digital input should count as high/low would be a rather generic thing, so not particularly device specific. Also, this very binding has a shunt-resistor-micro-ohms, and the individual channels have a drive-strength-microamp (granted, that latter one is a recent one of mine and may have slipped through review?). I can certainly understand that when a property specifies a raw value to put into some register (or field), that's very specific to that chip (or small family of chips) - the adi,ch-func properties fall into that category. Rasmus
On 26/06/2023 10:15, Rasmus Villemoes wrote: > On 23/06/2023 23.57, Rob Herring wrote: >> On Fri, Jun 23, 2023 at 05:44:50PM +0100, Conor Dooley wrote: >>> On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: >>>> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>>> index 590ea7936ad7..1f90ce3c7932 100644 >>>> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>>> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml >>>> @@ -51,6 +51,14 @@ properties: >>>> Shunt (sense) resistor value in micro-Ohms. >>>> default: 100000000 >>>> >>>> + digital-input-threshold-microvolt: >>> >>> Should this not have an adi vendor prefix, similar to >>> "adi,digital-input-threshold-mode-fixed"? >> >> Yes. > > OK. But I'm not really sure what the rules are for when such a prefix > must be added, so some guidance would be appreciated. There's > > - DO use a vendor prefix on device specific property names. Consider if > properties could be common among devices of the same class. > > And my thinking was that a threshold for when a digital input should > count as high/low would be a rather generic thing, so not particularly > device specific. Then find some more users of it. > > Also, this very binding has a shunt-resistor-micro-ohms, and the > individual channels have a drive-strength-microamp (granted, that latter > one is a recent one of mine and may have slipped through review?). I can > certainly understand that when a property specifies a raw value to put > into some register (or field), that's very specific to that chip (or > small family of chips) - the adi,ch-func properties fall into that category. Best regards, Krzysztof
On Mon, 26 Jun 2023 10:29:22 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 26/06/2023 10:15, Rasmus Villemoes wrote: > > On 23/06/2023 23.57, Rob Herring wrote: > >> On Fri, Jun 23, 2023 at 05:44:50PM +0100, Conor Dooley wrote: > >>> On Fri, Jun 23, 2023 at 01:33:25PM +0200, Rasmus Villemoes wrote: > >>>> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > >>>> index 590ea7936ad7..1f90ce3c7932 100644 > >>>> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > >>>> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml > >>>> @@ -51,6 +51,14 @@ properties: > >>>> Shunt (sense) resistor value in micro-Ohms. > >>>> default: 100000000 > >>>> > >>>> + digital-input-threshold-microvolt: > >>> > >>> Should this not have an adi vendor prefix, similar to > >>> "adi,digital-input-threshold-mode-fixed"? > >> > >> Yes. > > > > OK. But I'm not really sure what the rules are for when such a prefix > > must be added, so some guidance would be appreciated. There's > > > > - DO use a vendor prefix on device specific property names. Consider if > > properties could be common among devices of the same class. > > > > And my thinking was that a threshold for when a digital input should > > count as high/low would be a rather generic thing, so not particularly > > device specific. > > Then find some more users of it. The hi8435 could make use of this, but it currently doesn't get these thresholds from DT (despite there being a reasonable argument that these should be characteristics of the board wiring etc) but rather from userspace controls. Might well be something in gpio drivers? Linus / Bartosz, any of the input gpio devices really threshold detectors? If so is there any precedence for a DT binding to set the threshold? > > > > > Also, this very binding has a shunt-resistor-micro-ohms, and the > > individual channels have a drive-strength-microamp (granted, that latter > > one is a recent one of mine and may have slipped through review?). I can > > certainly understand that when a property specifies a raw value to put > > into some register (or field), that's very specific to that chip (or > > small family of chips) - the adi,ch-func properties fall into that category. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml index 590ea7936ad7..1f90ce3c7932 100644 --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml @@ -51,6 +51,14 @@ properties: Shunt (sense) resistor value in micro-Ohms. default: 100000000 + digital-input-threshold-microvolt: + description: + Comparator threshold used by the channels configured to use the + digital input function. + minimum: 500000 + maximum: 16000000 + multipleOf: 500000 + reset-gpios: maxItems: 1 @@ -143,6 +151,8 @@ examples: refin-supply = <&ad74413r_refin>; reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>; + digital-input-threshold-microvolt = <4000000>; + channel@0 { reg = <0>;
Allow specifying the threshold for which the channels configured as digital input change state. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Running dt_binding_check on this with a too small or large value in the example does give me an error, but the multipleOf does not seem to be enforced; the value 1234567 is not flagged. I don't know if that's expected (maybe I have too old versions of something). .../devicetree/bindings/iio/addac/adi,ad74413r.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)