| Message ID | 20180503221118.20754-1-chris.lesiak@licor.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: Fix logic for driver override | expand |
On 4/05/2018 06:11, Chris Lesiak 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 irq_request_resources repeated. irq_release_resources? > gpiochip_irq_relres should be used. > > Signed-off-by: Chris Lesiak <chris.lesiak@licor.com> > --- > 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; >
On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> 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@licor.com> > --- > 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. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/16/2018 07:49 AM, Linus Walleij wrote: > On Fri, May 4, 2018 at 12:11 AM, Chris Lesiak <chris.lesiak@licor.com> 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@licor.com> >> --- >> 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.
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@licor.com> 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@licor.com> > > > --- > > > 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
On 05/16/2018 08:43 AM, Thierry Reding wrote: > 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@licor.com> 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@licor.com> >>>> --- >>>> 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. >> To correct the record, I see that the comment was actually added to Rabin's patch by Linus. > 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? A reasonable argument. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 16, 2018 at 4:12 PM, Chris Lesiak <chris.lesiak@licor.com> wrote: > On 05/16/2018 08:43 AM, Thierry Reding wrote: > To correct the record, I see that the comment was actually added to Rabin's > patch by Linus. Oh I take the full blame. >> 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? > > A reasonable argument. The driver this was added for (ETRAX) is now gone. I looked around and AFAICT only one other driver actually uses this facility to override .irq_request/release_resources(): drivers/pinctrl/pinctrl-st.c Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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@licor.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)