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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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.

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

...

> > > +       /* 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?
hardware can't anymore function?

...

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

...

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

-- 
With Best Regards,
Andy Shevchenko