[PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Rasmus Villemoes]

Apparently, despite the name Digital Input Configuration Register, the
settings in the DIN_CONFIGx registers also affect other channel
functions. In particular, setting a non-zero value in the DIN_SINK
field breaks the resistance measurement function.

Now, one can of course argue that specifying a drive-strength-microamp
property along with a adi,ch-func which is not one of the digital
input functions is a bug in the device tree. However, we have a rather
complicated setup with instances of ad74412r on external hardware
modules, and have set a default drive-strength-microamp in our DT
fragments describing those, merely modifying the adi,ch-func settings
to reflect however the modules have been wired up. And restricting
this setting to just being done for digital input doesn't make the
driver any more complex.

Fixes: 504eb485589d1 (iio: ad74413r: wire up support for drive-strength-microamp property)
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index e3366cf5eb31..6b0e8218f150 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1317,13 +1317,14 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
 		}
 
 		if (config->func == CH_FUNC_DIGITAL_INPUT_LOGIC ||
-		    config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER)
+		    config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER) {
 			st->comp_gpio_offsets[comp_gpio_i++] = i;
 
-		strength = config->drive_strength;
-		ret = ad74413r_set_comp_drive_strength(st, i, strength);
-		if (ret)
-			return ret;
+			strength = config->drive_strength;
+			ret = ad74413r_set_comp_drive_strength(st, i, strength);
+			if (ret)
+				return ret;
+		}
 
 		ret = ad74413r_set_gpo_config(st, i, gpo_config);
 		if (ret)
-- 
2.37.2

Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Nuno Sá]

Hi Rasmus,

Thanks for your patch... Just one comment below

On Wed, 2023-05-03 at 12:50 +0200, Rasmus Villemoes wrote:
> Apparently, despite the name Digital Input Configuration Register, the
> settings in the DIN_CONFIGx registers also affect other channel
> functions. In particular, setting a non-zero value in the DIN_SINK
> field breaks the resistance measurement function.
> 
> Now, one can of course argue that specifying a drive-strength-microamp
> property along with a adi,ch-func which is not one of the digital
> input functions is a bug in the device tree. However, we have a rather
> complicated setup with instances of ad74412r on external hardware
> modules, and have set a default drive-strength-microamp in our DT
> fragments describing those, merely modifying the adi,ch-func settings
> to reflect however the modules have been wired up. And restricting
> this setting to just being done for digital input doesn't make the
> driver any more complex.
> 
> Fixes: 504eb485589d1 (iio: ad74413r: wire up support for drive-strength-
> microamp property)
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index e3366cf5eb31..6b0e8218f150 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -1317,13 +1317,14 @@ static int ad74413r_setup_gpios(struct ad74413r_state
> *st)
>                 }
>  
>                 if (config->func == CH_FUNC_DIGITAL_INPUT_LOGIC ||
> -                   config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER)
> +                   config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER) {
>                         st->comp_gpio_offsets[comp_gpio_i++] = i;
>  
> -               strength = config->drive_strength;
> -               ret = ad74413r_set_comp_drive_strength(st, i, strength);
> -               if (ret)
> -                       return ret;
> +                       strength = config->drive_strength;
> +                       ret = ad74413r_set_comp_drive_strength(st, i,
> strength);
> +                       if (ret)
> +                               return ret;
> +               }


So, I'm not really that familiar with this part and, at this stage, I'm being
lazy to check the datasheet. My concern is about breaking some other users...
So, does it make any sense for having drive-strength-microamp in a non digital
input at all? Can anyone have a working device by specifying that dt parameter
on a non digital channel (or expect something from having that parameter set)?
Or the only effect is to actually have some functions misbehaving?

It feels to me (from your description) that this property is something that
makes sense to be restricted and should also have that dependency stated in the
bindings but it might be to late for that (as some users might have validated
their devicetrees already). On the driver side, if it's never right to have
these settings together, then the patch is valid since if someone has this, his
configuration is broken anyways (maybe that's also a valid point for the
bindings)...

- Nuno Sá

Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Rasmus Villemoes]

On 04/05/2023 09.28, Nuno Sá wrote:
> Hi Rasmus,
> 

> So, I'm not really that familiar with this part and, at this stage, I'm being
> lazy to check the datasheet. 

Well, the data sheet is not particularly helpful here, which is why I
ended up with this mess.

> My concern is about breaking some other users...

I highly doubt there are users yet (other than my customer); this
binding+driver implementation only just landed.

> So, does it make any sense for having drive-strength-microamp in a non digital
> input at all?

That's the problem with the data sheet, it doesn't really say that the
DIN_SINK register has any effect whatsoever when the channel function is
set to something other than digital input (either flavor). Perhaps it
does hint that setting it to something non-zero is probably not a good
idea, because DIN_SINK is automatically set to 0 whenever the channel
function is set/changed, so one needs a good reason to change DIN_SINK
afterwards.

We just experimentally found out that when we added the DIN_SINK to fix
the digital input functions, when we got around to testing the
resistance measurement function that ended up broken due to the non-zero
DIN_SINK.

> Can anyone have a working device by specifying that dt parameter
> on a non digital channel (or expect something from having that parameter set)?
> Or the only effect is to actually have some functions misbehaving?

The data sheet doesn't say that the DIN_SINK should have any effect for
other functions, so I'm pretty sure it's only the latter: some functions
misbehave.

> On the driver side, if it's never right to have
> these settings together, then the patch is valid since if someone has this, his
> configuration is broken anyways (maybe that's also a valid point for the
> bindings)...

Yes, I do believe that it's a broken description (whether or not the
bindings specify that), and drivers don't need to go out of their way to
validate or fixup such brokenness. But in this particular case, there's
really no extra burden on the driver to not put garbage in DIN_SINK when
a not-digital-input function has been chosen (the patch is a two-liner
with 'git show -w').

Rasmus

Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Jonathan Cameron]

On Thu, 4 May 2023 12:08:53 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 04/05/2023 09.28, Nuno Sá wrote:
> > Hi Rasmus,
> >   
> 
> > So, I'm not really that familiar with this part and, at this stage, I'm being
> > lazy to check the datasheet.   
> 
> Well, the data sheet is not particularly helpful here, which is why I
> ended up with this mess.
> 
> > My concern is about breaking some other users...  
> 
> I highly doubt there are users yet (other than my customer); this
> binding+driver implementation only just landed.
> 
> > So, does it make any sense for having drive-strength-microamp in a non digital
> > input at all?  
> 
> That's the problem with the data sheet, it doesn't really say that the
> DIN_SINK register has any effect whatsoever when the channel function is
> set to something other than digital input (either flavor). Perhaps it
> does hint that setting it to something non-zero is probably not a good
> idea, because DIN_SINK is automatically set to 0 whenever the channel
> function is set/changed, so one needs a good reason to change DIN_SINK
> afterwards.
> 
> We just experimentally found out that when we added the DIN_SINK to fix
> the digital input functions, when we got around to testing the
> resistance measurement function that ended up broken due to the non-zero
> DIN_SINK.
> 
> > Can anyone have a working device by specifying that dt parameter
> > on a non digital channel (or expect something from having that parameter set)?
> > Or the only effect is to actually have some functions misbehaving?  
> 
> The data sheet doesn't say that the DIN_SINK should have any effect for
> other functions, so I'm pretty sure it's only the latter: some functions
> misbehave.
> 
> > On the driver side, if it's never right to have
> > these settings together, then the patch is valid since if someone has this, his
> > configuration is broken anyways (maybe that's also a valid point for the
> > bindings)...  
> 
> Yes, I do believe that it's a broken description (whether or not the
> bindings specify that), and drivers don't need to go out of their way to
> validate or fixup such brokenness. But in this particular case, there's
> really no extra burden on the driver to not put garbage in DIN_SINK when
> a not-digital-input function has been chosen (the patch is a two-liner
> with 'git show -w').

If we can tighten the DT binding to rule out something that should not be
set than that would be good.  Tightening bindings is fine - we don't mind
validation of bindings failing on peoples DTs as long as we didn't 'break'
them actually working.

Jonathan

> 
> Rasmus
> 

Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Rasmus Villemoes]

On 06/05/2023 20.16, Jonathan Cameron wrote:
> On Thu, 4 May 2023 12:08:53 +0200
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> On 04/05/2023 09.28, Nuno Sá wrote:

>>> Can anyone have a working device by specifying that dt parameter
>>> on a non digital channel (or expect something from having that parameter set)?
>>> Or the only effect is to actually have some functions misbehaving?  
>>
>> The data sheet doesn't say that the DIN_SINK should have any effect for
>> other functions, so I'm pretty sure it's only the latter: some functions
>> misbehave.
>>
>>> On the driver side, if it's never right to have
>>> these settings together, then the patch is valid since if someone has this, his
>>> configuration is broken anyways (maybe that's also a valid point for the
>>> bindings)...  
>>
>> Yes, I do believe that it's a broken description (whether or not the
>> bindings specify that), and drivers don't need to go out of their way to
>> validate or fixup such brokenness. But in this particular case, there's
>> really no extra burden on the driver to not put garbage in DIN_SINK when
>> a not-digital-input function has been chosen (the patch is a two-liner
>> with 'git show -w').
> 
> If we can tighten the DT binding to rule out something that should not be
> set than that would be good.  Tightening bindings is fine - we don't mind
> validation of bindings failing on peoples DTs as long as we didn't 'break'
> them actually working.

Well, I'm afraid I don't have any idea how to spell that constraint in
the yaml-language (help appreciated).

And I assume a dt binding update would be a separate patch anyway, so
could you please consider applying this patch?

Thanks,
Rasmus

Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions other than digital input

[Jonathan Cameron]

On Mon, 22 May 2023 10:44:11 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 06/05/2023 20.16, Jonathan Cameron wrote:
> > On Thu, 4 May 2023 12:08:53 +0200
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> >> On 04/05/2023 09.28, Nuno Sá wrote:  
> 
> >>> Can anyone have a working device by specifying that dt parameter
> >>> on a non digital channel (or expect something from having that parameter set)?
> >>> Or the only effect is to actually have some functions misbehaving?    
> >>
> >> The data sheet doesn't say that the DIN_SINK should have any effect for
> >> other functions, so I'm pretty sure it's only the latter: some functions
> >> misbehave.
> >>  
> >>> On the driver side, if it's never right to have
> >>> these settings together, then the patch is valid since if someone has this, his
> >>> configuration is broken anyways (maybe that's also a valid point for the
> >>> bindings)...    
> >>
> >> Yes, I do believe that it's a broken description (whether or not the
> >> bindings specify that), and drivers don't need to go out of their way to
> >> validate or fixup such brokenness. But in this particular case, there's
> >> really no extra burden on the driver to not put garbage in DIN_SINK when
> >> a not-digital-input function has been chosen (the patch is a two-liner
> >> with 'git show -w').  
> > 
> > If we can tighten the DT binding to rule out something that should not be
> > set than that would be good.  Tightening bindings is fine - we don't mind
> > validation of bindings failing on peoples DTs as long as we didn't 'break'
> > them actually working.  
> 
> Well, I'm afraid I don't have any idea how to spell that constraint in
> the yaml-language (help appreciated).

Lots of examples in tree of this sort of thing.   Look for a 
: false with something other than additionalProperties or unevaluatedProperties
Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
for example.

In short you have an allOf block containing a list of rules, one of which
is a match on particular conditions to set particular properties to 'false'
which means that any attempt to have them set when that condition is met
results in an error from the dts checking scripts.

> 
> And I assume a dt binding update would be a separate patch anyway, so
> could you please consider applying this patch?
Fair enough. Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> 
> Thanks,
> Rasmus
>