[PATCH 0/2] iio: ad74413r: allow configuring digital input threshold

[PATCH 0/2] iio: ad74413r: allow configuring digital input threshold

[Rasmus Villemoes]

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

-- 
2.37.2

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

[Rasmus Villemoes]

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

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>;
 
-- 
2.37.2

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Rob Herring]

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

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

[Rasmus Villemoes]

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

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

[Krzysztof Kozlowski]

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

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

[Jonathan Cameron]

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
> 

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

[Rob Herring]

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

[PATCH 2/2] iio: addac: ad74413r: wire up digital-input-threshold-microvolt DT property

[Rasmus Villemoes]

The threshold is common to all channels configured as digital
input.

So far, we have not seen the settings in the DIN_THRESH affecting
functions other than digital input, but with a4cba07e64e6 ("iio:
addac: ad74413: don't set DIN_SINK for functions other than digital
input") in mind, do not read and apply the
digital-input-threshold-microvolt setting unless at least one channel
has been configured as one of the digital input variants.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index e3366cf5eb31..efdd7fdd7ab9 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -129,6 +129,12 @@ struct ad74413r_state {
 #define AD74413R_GPO_CONFIG_COMPARATOR		0b011
 #define AD74413R_GPO_CONFIG_HIGH_IMPEDANCE	0b100
 
+#define AD74413R_REG_DIN_THRESH		0x22
+#define AD74413R_DIN_COMP_THRESH_MASK	GENMASK(5, 1)
+#define AD74413R_DIN_THRESH_MODE	BIT(0)
+#define AD74413R_DIN_THRESH_MODE_16V	BIT(0)
+#define AD74413R_DIN_THRESH_MODE_AVDD	0
+
 #define AD74413R_REG_ADC_CONV_CTRL	0x23
 #define AD74413R_CONV_SEQ_MASK		GENMASK(9, 8)
 #define AD74413R_CONV_SEQ_ON		0b00
@@ -1446,6 +1452,20 @@ static int ad74413r_probe(struct spi_device *spi)
 	}
 
 	if (st->num_comparator_gpios) {
+		u32 thresh, val, mask;
+
+		if (!device_property_read_u32(st->dev, "digital-input-threshold-microvolt",
+					      &thresh)) {
+			val = thresh/500000 - 1;
+			val = FIELD_PREP(AD74413R_DIN_COMP_THRESH_MASK, val);
+			val |= AD74413R_DIN_THRESH_MODE_16V;
+			mask = AD74413R_DIN_COMP_THRESH_MASK | AD74413R_DIN_THRESH_MODE;
+			ret = regmap_update_bits(st->regmap, AD74413R_REG_DIN_THRESH,
+						 mask, val);
+			if (ret)
+				return ret;
+		}
+
 		st->comp_gpiochip.owner = THIS_MODULE;
 		st->comp_gpiochip.label = st->chip_info->name;
 		st->comp_gpiochip.base = -1;
-- 
2.37.2

Re: [PATCH 2/2] iio: addac: ad74413r: wire up digital-input-threshold-microvolt DT property

[Jonathan Cameron]

On Fri, 23 Jun 2023 13:33:26 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> The threshold is common to all channels configured as digital
> input.
> 
> So far, we have not seen the settings in the DIN_THRESH affecting
> functions other than digital input, but with a4cba07e64e6 ("iio:
> addac: ad74413: don't set DIN_SINK for functions other than digital
> input") in mind, do not read and apply the
> digital-input-threshold-microvolt setting unless at least one channel
> has been configured as one of the digital input variants.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
LGTM. Will pick up once the minor change in binding is done if no
other feedback comes in.

Thanks,

Jonathan

> ---
>  drivers/iio/addac/ad74413r.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index e3366cf5eb31..efdd7fdd7ab9 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -129,6 +129,12 @@ struct ad74413r_state {
>  #define AD74413R_GPO_CONFIG_COMPARATOR		0b011
>  #define AD74413R_GPO_CONFIG_HIGH_IMPEDANCE	0b100
>  
> +#define AD74413R_REG_DIN_THRESH		0x22
> +#define AD74413R_DIN_COMP_THRESH_MASK	GENMASK(5, 1)
> +#define AD74413R_DIN_THRESH_MODE	BIT(0)
> +#define AD74413R_DIN_THRESH_MODE_16V	BIT(0)
> +#define AD74413R_DIN_THRESH_MODE_AVDD	0
> +
>  #define AD74413R_REG_ADC_CONV_CTRL	0x23
>  #define AD74413R_CONV_SEQ_MASK		GENMASK(9, 8)
>  #define AD74413R_CONV_SEQ_ON		0b00
> @@ -1446,6 +1452,20 @@ static int ad74413r_probe(struct spi_device *spi)
>  	}
>  
>  	if (st->num_comparator_gpios) {
> +		u32 thresh, val, mask;
> +
> +		if (!device_property_read_u32(st->dev, "digital-input-threshold-microvolt",
> +					      &thresh)) {
> +			val = thresh/500000 - 1;
> +			val = FIELD_PREP(AD74413R_DIN_COMP_THRESH_MASK, val);
> +			val |= AD74413R_DIN_THRESH_MODE_16V;
> +			mask = AD74413R_DIN_COMP_THRESH_MASK | AD74413R_DIN_THRESH_MODE;
> +			ret = regmap_update_bits(st->regmap, AD74413R_REG_DIN_THRESH,
> +						 mask, val);
> +			if (ret)
> +				return ret;
> +		}
> +
>  		st->comp_gpiochip.owner = THIS_MODULE;
>  		st->comp_gpiochip.label = st->chip_info->name;
>  		st->comp_gpiochip.base = -1;