Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Fri, 2022-07-08 at 15:24 +0000, Sa, Nuno wrote:
> 
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Sent: Friday, July 8, 2022 5:05 PM
> > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> > Cc: devicetree <devicetree@xxxxxxxxxxxxxxx>; open list:GPIO
> > SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; linux-input <linux-
> > input@xxxxxxxxxxxxxxx>; Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx>; Bartosz Golaszewski
> > <brgl@xxxxxxxx>; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>; Rob Herring
> > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Linus Walleij
> > <linus.walleij@xxxxxxxxxx>
> > 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 <Nuno.Sa@xxxxxxxxxx> wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > > Sent: Friday, July 8, 2022 4:18 PM
> > > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@xxxxxxxxxx>
> > wrote:
> > 
> > ...
> > 
> > > > > +       kpad->gc.parent = &kpad->client->dev;
> > > > 
> > > > > +       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.
> 
> ok, I will drop the assignment...
> 
> > > The parent assignment was also to make things neater in
> > > /sys/kernel/debug/gpio.
> > 
> > Sure.
> > 
> > ...
> > 
> > > > > +       girq->handler = handle_simple_irq;
> > > > 
> > > > By default it should be handle_bad_irq() and locked in the -
> > > > > irq_set_type().
> > > > 
> > > > > +       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.

> > ...
> > 
> > > > > +       /* 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.
> > > 
> > > > > +       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.
> 
> > 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!
> 
> > ...
> > 
> > > > 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?
> 
> I will have to look at irq_valid_mask.
> 
> > ...
> > 
> > > > > +       /*
> > > > > +        * 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.
> > 
> > ...
> > 
> > > > > +               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?
> 
> 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á