On Wed, May 16, 2018 at 08:34:36AM -0500, Chris Lesiak wrote:
> On 05/16/2018 07:49 AM, Linus Walleij wrote:
>
> > On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@xxxxxxxxx> wrote:
> >
> > > Fix incorrect logic in gpiochip_irqchip_add_key().
> > > A driver needs to override both irq_request_resources and
> > > irq_request_resources, otherwise gpiochip_irq_reqres and
> > > gpiochip_irq_relres should be used.
> > >
> > > Signed-off-by: Chris Lesiak <chris.lesiak@xxxxxxxxx>
> > > ---
> > > drivers/gpio/gpiolib.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bdd68ff197dc..3e441879fea7 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1831,7 +1831,7 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
> > > * It is possible for a driver to override this, but only if the
> > > * alternative functions are both implemented.
> > > */
> > > - if (!irqchip->irq_request_resources &&
> > > + if (!irqchip->irq_request_resources ||
> > > !irqchip->irq_release_resources) {
> > > irqchip->irq_request_resources = gpiochip_irq_reqres;
> > > irqchip->irq_release_resources = gpiochip_irq_relres;
> > This code was refactored by Thierry, but the logic is from
> > Rabin's patch
> > commit 8b67a1f0ad1f260f1a4032d5f7b032ac113bfa7d
> > "gpio: don't override irq_*_resources() callbacks"
> >
> > I think the intention was to only allow gpiolib's implementation
> > to kick in if the driver provided no callbacks at all.
>
> I agree that is what Rabin's patch implemented, but it doesn't match the
> patches comment.
>
> The code in kernel/irq/manage.c is certainly happy of one (or both) of
> irqchip->irq_request_resources and irqchip->irq_release_resources are null.
> But it seems likely that a driver, if needing to override at all, would need
> to override the pair. The existing drivers follow that pattern.
>
> As such, I think Rabin's comment was correct, but his code wrong.
I disagree. I don't think the core should be overriding anything that
the driver has explicitly set up. I don't see why a driver shouldn't be
allowed to set up only one of them. What if ->irq_release_resources()
doesn't need to do anything for a specific driver? Should the driver be
required to provide an empty dummy just so the core doesn't override an
implementation of ->irq_request_resources() that the driver specified?
Thierry
Attachment:
signature.asc
Description: PGP signature
