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@xxxxxxxxxxxxxxxxxx>
> ---
>  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á