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

[Jonathan Cameron <jic23@kernel.org>]

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
>