On Wed, 3 Nov 2021 16:17:24 +0200
Cosmin Tanislav [off-list ref] wrote:

quoted

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

quoted

even in order!  

I'm sorry, I'm new to the LKML.

quoted

quoted

quoted

Rule it out in the dt binding and enforce it at probe time not here  

which

quoted

quoted

is far too late.  

quoted

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

quoted

quoted

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

quoted

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
 

quoted

Logic parallel mode is reserved for set_multiple, when the GPIO is in  

logic

quoted

quoted

mode.  

So, it took me a while to understand what we would loose by 'only'  

providing

quoted

the logic parallel mode.  If we only had logic high / logic low as the  

options

quoted

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

quoted

for why we had high impedance and 100kOhms as options.

These allow you to implement shared buses over these pins.  Which  

incidentally

quoted

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

quoted

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?
 

quoted

GPIOs are referred to as GPO in the datasheet, so I used this name in  

the

quoted

quoted

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

quoted

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...

quoted

quoted

quoted

Error out of this function is fine, but why not just leave this  

channel

quoted

quoted

disabled
rather than failing to probe which I think is what will currently  

happen?

quoted

quoted

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.

quoted

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

Okay, so after some more thought I realized where I went wrong with
this implementation. I'm writing down my ideas before I go implement
it so we can get on the same page.

Device-tree configuration should only specify if a gpo is in comparator
mode or not. When not in comparator mode, the gpo is in output mode.
When in output mode, it is exposed as an output-only gpio pin.

When a channel is in digital input mode, an input-only gpio is exposed
for the value of the comparator result, which can be read via the
DIN_COMP_OUT register.

Yes, though I think you also need to somehow reflect that it's an entirely
different input rather than being the same pin as the output gpios
would be on if in that mode.  I'm not sure how you would do that in the
gpio subsystem, possibly the offset?

Regarding high impedance and pull down resistor, should I be implementing
these features by implementing gpio_chip->set_config function and
handling the PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
PIN_CONFIG_BIAS_PULL_DOWN flags?

Yes, I think so.

The comparator also has a setting for debounce time, so I could also handle
PIN_CONFIG_INPUT_DEBOUNCE.

Sound right to me, but gpio people may disagree :)

Jonathan

Thank you for your patience.
Cosmin.