On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno [off-list ref] wrote:

quoted

From: Andy Shevchenko <redacted>
Sent: Friday, July 8, 2022 4:18 PM
On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá [off-list ref] wrote:

...

quoted

quoted

+       kpad->gc.parent = &kpad->client->dev;

quoted

+       kpad->gc.of_node = kpad->client->dev.of_node;

We are going to remove of_node from GPIO. Moreover the parent
device
and its node is a duplication, just drop the latter and GPIO library
will take care of it.

Well the of_node was set so that I had a proper name in the IRQ domain
IIRC. Will this be handled in the GPIO lib in the future?

In your case it's a dup. So in _your_ case it will be handled in the
future. For the rest we already have an fwnode member.

The parent assignment was also to make things neater in
/sys/kernel/debug/gpio.

Sure.

...

quoted

quoted

+       girq->handler = handle_simple_irq;

By default it should be handle_bad_irq() and locked in the -

quoted

irq_set_type().

quoted

+       girq->threaded = true;

See documentation above.

I see... I will look at Docs. In practice I don't think this matters much
since this handler should never really be called (I think) as we just
call handle_nested_irq().

There are two different comments, one about handler, another about how
to properly write IRQ chip data structure and mask()/unmask()
callbacks.

...

quoted

quoted

+       /* should never happen */

Then why it's here?

because I do not trust the HW to always cooperate :). In theory,
we can get some invalid 'gpio' from it.

quoted

quoted

+       WARN_ON_ONCE(hwirq == ngpios);

On some setups this can lead to panic. Why? Is this so critical error?
hardware can't anymore function?

...

quoted

I don't know this code, can you summarize why this additional mapping
is needed?

You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now,
if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But
what we get from the device in 'key_val - GPI_PIN_BASE' is, for example,
16 and so we need to get the hwirq which will be 0. It's pretty much the
reverse of what it's being done in the GPIOs callbacks.

Any reason why irq_valid_mask can't be used for that?

...

quoted

quoted

+       /*
+        * Default is active low which means key_press is asserted on
+        * the falling edge.
+        */
+       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
+           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))

This is dup from ->irq_set_type(). Or how this can be not like this?

We get here if we get a key press (falling edge) or a key release (rising
edge). The events are given by the device and it might be that in some
cases we just want to handle/propagate key presses
(not sure if it makes sense). So we need to match it with the
appropriate irq_type requested. Note that this kind of controlling the IRQ
from SW as there's no way from doing it in the device. That is why we don't
do more than just making sure the IRQ types are valid in irq_set_type.

I see, thanks for elaboration.

...

quoted

quoted

+               handle_nested_irq(irq);

There is new helpers I believe for getting domain and handle an IRQ.
Grep for the recent (one-two last cycles) changes in the GPIO drivers.

Hmm, I think I saw it but somehow I though I could not use it (because
of the previous calls to get the irq_type). Hmmm...

Maybe you can double check?

-- 
With Best Regards,
Andy Shevchenko