linux-kernel - Re: [PATCH 3/3] iio: addac: add AD74413R driver

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>] [day] [month] [year] [list]

Message-ID: <20211030165406.149b9036@jic23-huawei>
Date:   Sat, 30 Oct 2021 16:54:06 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Cosmin Tanislav <demonsingur@...il.com>
Cc:     cosmin.tanislav@...log.com, Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Rob Herring <robh+dt@...nel.org>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: addac: add AD74413R driver

@Linus, some gpio related discussion below...

On Fri, 29 Oct 2021 00:38:31 +0300
Cosmin Tanislav <demonsingur@...il.com> wrote:

Too much context removed. I had to go back and look earlier in the thread
to work out what was being discussed.  Particularly as I think these aren't
even in order!

> > Often these are 1/(N**2) rather than 1/(N**2 - 1) as here.
> > Noise level is probably high enough this doesn't really matter though.
> > Data sheets are never entirely clear if ranges are inclusive or not...  
> 
> My max values are defined as being inclusive.
> On a 13bit DAC, raw values are in the range of 0 to 2**13 - 1, inclusive.
> 
OK

> > Rule it out in the dt binding and enforce it at probe time not here which  
> is far too late.
> > As below, this should be prevented at probe time not runtime.  
> 
> This is done so that the GPIO indices are kept the same as the hardware
> channels, 0, 1, 2, 3.
> Depending on their mode, some GPIOs can only be read and some of them can
> only be written to.
> I'm not sure how you would want to do this at probe time?

I'm not totally following, but took more a look at the datasheet.

Device has 4 GPO pins whch is fine.  Those are simple output only pins. For
those, if they are in a mode where you are controlling them then you can
cache the value - if they are in comparator mode then they aren't really
acting as GPIO pins at all the value you are reading is reflecting the
input on the other side of the device on a different pin. So in that
case don't register these with the GPIO subsystem at all.
Instead you are registering channels selected from A,B,C,D 

> 
> Logic parallel mode is reserved for set_multiple, when the GPIO is in logic
> mode.
So, it took me a while to understand what we would loose by 'only' providing
the logic parallel mode.  If we only had logic high / logic low as the options
then a sensible driver option would be to map any GPO configuration to the
logic parallel mode.  It enables more functionality.  However, that got me thinking
for why we had high impedance and 100kOhms as options.

These allow you to implement shared buses over these pins.  Which incidentally
should probably be mapped through to the various gpio subsystem controls
to reflect these options.

So the state combinations you might well have would be...

Logic low / logic high
100kOhm pull down / logic high (something like an i2c bus would use this)
tristate so logic low / logic high / high impedance.(don't care or off)

Other than the first one, these require you to not be in the GPO mode.

However, this is all stuff that depends on what these are wired to - so the
dts should reflect that rather than simply setting the mode to one of

+          0 - GPO_CONFIG_100K_PULL_DOWN
+          1 - GPO_CONFIG_LOGIC
+          3 - GPO_CONFIG_DEBOUNCED_COMPARATOR
+          4 - GPO_CONFIG_HIGH_IMPEDANCE

The only exception being the debounce comparator. So the question is where
would that be wired to?

> GPIOs are referred to as GPO in the datasheet, so I used this name in the
> driver too.

Sort of... As above, the output pins are GPO, the input pins are the ones
the comparator is running on - whilst there value is relected on the outputs
when in the right mode (and that's the only one you can read them in) they
are not the same channel.  I'm not sure they should map to the same index...

> 
> > Error out of this function is fine, but why not just leave this channel  
> disabled
> rather than failing to probe which I think is what will currently happen?
> 
> Sure, I could make invalid channel and gpo functions fallback to default
> function.
> But why tolerate errors in the dts configuration?

I don't think it is an error but rather a function you haven't enabled yet
in a specific driver.  dts is for the hardware, not your particular driver.

Mind you, I think the binding around this needs to change entirely anyway
so this will probably not end up relevant.

Jonathan