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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230506191636.3cff4b24@jic23-huawei>
Date:   Sat, 6 May 2023 19:16:36 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Nuno Sá <noname.nuno@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: addac: ad74413: don't set DIN_SINK for functions
 other than digital input

On Thu, 4 May 2023 12:08:53 +0200
Rasmus Villemoes <linux@...musvillemoes.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
>