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@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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
