| Message ID | 20230623113327.1062170-1-linux@rasmusvillemoes.dk |
|---|---|
| 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 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
The reset default value of the DIN_THRESH register is 0x0, meaning that the threshold for the digital input channels is 1/60 of AVDD. In most applications, that value is way too low and susceptible to noise. These patches introduce a new DT property, digital-input-threshold-microvolt, which if present will be used as the threshold in "16V" mode, i.e. as an absolute threshold, not proportional to AVDD. If someone needs the threshold to be proportional to AVDD, but being say 15/60, another DT property (mutually exclusive with this one) could be introduced. But since I don't need that and can't come up with a good name ('digital-input-threshold-60ths-avdd' ?) I punt that problem to whoever needs it. Rasmus Villemoes (2): dt-bindings: iio: ad74413r: add binding for digital input threshold iio: addac: ad74413r: wire up digital-input-threshold-microvolt DT property .../bindings/iio/addac/adi,ad74413r.yaml | 10 ++++++++++ drivers/iio/addac/ad74413r.c | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+)