> -----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. > > ... > > > > > + /* 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... - Nuno Sá
