On Wed, Sep 21, 2022 at 09:18:34AM -0600, Raul Rangel wrote: > On Tue, Sep 20, 2022 at 6:32 AM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Mon, Sep 19, 2022 at 09:59:09AM -0600, Raul E Rangel wrote: ... > > > + if (irq_ctx.irq == -ENOENT) > > > + irq_ctx.irq = acpi_dev_gpio_irq_wake_get(adev, 0, &irq_ctx.wake_capable); > > > > I just realized, that there is an inconsistency on how we fill the wake_capable > > parameter. In some cases we check for IRQ for an error condition (IRQ not found) > > and in some the wake_capable still be filled. > > > > Here the best approach I believe is to add > > > > if (irq_ctx.irq < 0) > > return irq_ctx.irq; > > > > I.o.w. we apply the rule "do not fill the output parameters when it's known > > to be an error condition". > > > > > + if (wake_capable) > > > + *wake_capable = irq_ctx.wake_capable; > > > > > + return irq_ctx.irq; > > > > I applied the following: > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index ba64e505183595..1618f5619d5ed9 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -220,7 +220,7 @@ int i2c_acpi_get_irq(struct i2c_client *client, > bool *wake_capable) > if (irq_ctx.irq == -ENOENT) > irq_ctx.irq = acpi_dev_gpio_irq_wake_get(adev, 0, > &irq_ctx.wake_capable); > > - if (wake_capable) > + if (irq_ctx.irq > 0 && wake_capable) > *wake_capable = irq_ctx.wake_capable; > > return irq_ctx.irq; While it's working solution it is not so flexible since basically any addition of a new code will require if (irq > 0), that's why I'm in favour of my proposal rather than yours approach. -- With Best Regards, Andy Shevchenko
