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á
