[1/2] dt-bindings: iio: ad74413r: add binding for digital input threshold

  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(+)
  

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
>