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
