On Fri, 2022-07-08 at 15:24 +0000, Sa, Nuno wrote:

quoted

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Friday, July 8, 2022 5:05 PM
To: Sa, Nuno <Nuno.Sa@analog.com>
Cc: devicetree <redacted>; open list:GPIO
SUBSYSTEM [off-list ref]; linux-input <linux-
input@vger.kernel.org>; Dmitry Torokhov
[off-list ref]; Bartosz Golaszewski
[off-list ref]; Hennerich, Michael
[off-list ref]; Rob Herring
[off-list ref]; Krzysztof Kozlowski
[off-list ref]; Linus Walleij
[off-list ref]
Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support
gpi
key events as 'gpio keys'

[External]

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

quoted

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

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

quoted

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.

ok, I will drop the assignment...

quoted

quoted

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

Sure.

...

quoted

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.

So I think I have most of stuff understood for v2. About the handler, I
don't think we really need to set 'handle_edge_irq()' in
'irq_set_type()' as this is a nested threaded interrupt and so, the
desc->handle_irq() should never be called (I think, not 100% if there's
any strange case where it might).

That said, if you still think that I should do it (for correctness),
fine by me.

quoted

...

quoted

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?

Ahh, you're right. Forgot that in some cases WARN can actually panic.

quoted

hardware can't anymore function?

If we get in here, the device is probably in a very bad state but
that
does not mean that the system is...

I will just move to dev_warn(). Thanks for the remainder!

quoted

...

quoted

quoted

I don't know this code, can you summarize why this additional

mapping

quoted

quoted

is needed?

You have 18 possible pins to use as GPIOs (and hence be IRQ

sources). Now,

quoted

if you just want to use pins 16 and 17 that will map int hwirq 0
and 1.

But

quoted

what we get from the device in 'key_val - GPI_PIN_BASE' is, for

example,

quoted

16 and so we need to get the hwirq which will be 0. It's pretty
much

the

quoted

reverse of what it's being done in the GPIOs callbacks.

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

I will have to look at irq_valid_mask.

quoted

...

quoted

quoted

quoted

+       /*
+        * Default is active low which means key_press is
asserted

on

quoted

quoted

quoted

+        * 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

quoted

edge). The events are given by the device and it might be that in

some

quoted

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

quoted

from SW as there's no way from doing it in the device. That is
why we

don't

quoted

do more than just making sure the IRQ types are valid in

irq_set_type.

I see, thanks for elaboration.

...

quoted

quoted

quoted

+               handle_nested_irq(irq);

There is new helpers I believe for getting domain and handle an

IRQ.

quoted

quoted

Grep for the recent (one-two last cycles) changes in the GPIO

drivers.

quoted

quoted

Hmm, I think I saw it but somehow I though I could not use it

(because

quoted

of the previous calls to get the irq_type). Hmmm...

Maybe you can double check?

Sure, I think the helper can be used...

So I did looked and I think you are thinking about
'generic_handle_domain_irq()'. For nested threaded I could not find a
similar one (maybe a new helper to be added).

- Nuno Sá