| Message ID | 1597058460-16211-3-git-send-email-mkshah@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip: qcom: pdc: Introduce irq_set_wake call | expand |
Quoting Maulik Shah (2020-08-10 04:20:55) > msmgpio irqchip is not using return value of irq_set_wake call. > Start using it. Does this work when the irq parent isn't setup in a hierarchy? I seem to recall that this was written this way because sometimes irq_set_irq_wake() would fail for the summary irq so it was a best effort setting of wake on the summary line. > > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy") > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 90edf61..c264561 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1077,12 +1077,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > * when TLMM is powered on. To allow that, enable the GPIO > * summary line to be wakeup capable at GIC. > */ > - if (d->parent_data) > - irq_chip_set_wake_parent(d, on); > - > - irq_set_irq_wake(pctrl->irq, on); > + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) > + return irq_chip_set_wake_parent(d, on); So this bit is probably fine. > > - return 0; > + return irq_set_irq_wake(pctrl->irq, on); But this one is probably not fine. > } > > static int msm_gpio_irq_reqres(struct irq_data *d)
Hi, On Tue, Aug 11, 2020 at 12:34 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Maulik Shah (2020-08-10 04:20:55) > > msmgpio irqchip is not using return value of irq_set_wake call. > > Start using it. > > Does this work when the irq parent isn't setup in a hierarchy? I seem to > recall that this was written this way because sometimes > irq_set_irq_wake() would fail for the summary irq so it was a best > effort setting of wake on the summary line. > > > > > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy") > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > --- > > drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 90edf61..c264561 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -1077,12 +1077,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > > * when TLMM is powered on. To allow that, enable the GPIO > > * summary line to be wakeup capable at GIC. > > */ > > - if (d->parent_data) > > - irq_chip_set_wake_parent(d, on); > > - > > - irq_set_irq_wake(pctrl->irq, on); > > + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) > > + return irq_chip_set_wake_parent(d, on); > > So this bit is probably fine. > > > > > - return 0; > > + return irq_set_irq_wake(pctrl->irq, on); > > But this one is probably not fine. Interesting. I wasn't aware of the history and thus assumed this was a bug. If Stephen is remembering correctly, please add a comment saying that we are purposely ignoring the return value in this case. -Doug
Hi, On 8/12/2020 1:04 AM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-08-10 04:20:55) >> msmgpio irqchip is not using return value of irq_set_wake call. >> Start using it. > Does this work when the irq parent isn't setup in a hierarchy? yes it works fine even when parent isn't setup in hierarchy. > I seem to > recall that this was written this way because sometimes > irq_set_irq_wake() would fail for the summary irq so it was a best > effort setting of wake on the summary line. Thanks for pointing this. It was written this way since previously GIC driver neither had IRQCHIP_SKIP_SET_WAKE flag nor it implemented .irq_set_wake callback, so the call to irq_set_irq_wake() to set_irq_wake_real() used to return error -ENXIO in past. I see this is already taken care now in GIC drivers by adding IRQCHIP_SKIP_SET_WAKE flag. > >> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy") >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Reviewed-by: Douglas Anderson <dianders@chromium.org> >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index 90edf61..c264561 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -1077,12 +1077,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> * when TLMM is powered on. To allow that, enable the GPIO >> * summary line to be wakeup capable at GIC. >> */ >> - if (d->parent_data) >> - irq_chip_set_wake_parent(d, on); >> - >> - irq_set_irq_wake(pctrl->irq, on); >> + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) >> + return irq_chip_set_wake_parent(d, on); > So this bit is probably fine. > >> >> - return 0; >> + return irq_set_irq_wake(pctrl->irq, on); > But this one is probably not fine. As per above both of them are fine. Thanks, Maulik > >> } >> >> static int msm_gpio_irq_reqres(struct irq_data *d)
Quoting Maulik Shah (2020-08-13 00:17:18) > Hi, > > On 8/12/2020 1:04 AM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-08-10 04:20:55) > >> msmgpio irqchip is not using return value of irq_set_wake call. > >> Start using it. > > Does this work when the irq parent isn't setup in a hierarchy? > yes it works fine even when parent isn't setup in hierarchy. > > I seem to > > recall that this was written this way because sometimes > > irq_set_irq_wake() would fail for the summary irq so it was a best > > effort setting of wake on the summary line. > Thanks for pointing this. > > It was written this way since previously GIC driver neither had > IRQCHIP_SKIP_SET_WAKE flag nor it implemented .irq_set_wake callback, > > so the call to irq_set_irq_wake() to set_irq_wake_real() used to return > error -ENXIO in past. > > I see this is already taken care now in GIC drivers by adding > IRQCHIP_SKIP_SET_WAKE flag. Ok, great. Thanks for double checking. Can you add those details to the commit message so we don't forget? And then I'm happy to see: Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Mon, Aug 10, 2020 at 1:21 PM Maulik Shah <mkshah@codeaurora.org> wrote: > msmgpio irqchip is not using return value of irq_set_wake call. > Start using it. > > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy") > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I suppose it needs to go in with the rest of the patches. Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 90edf61..c264561 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -1077,12 +1077,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) * when TLMM is powered on. To allow that, enable the GPIO * summary line to be wakeup capable at GIC. */ - if (d->parent_data) - irq_chip_set_wake_parent(d, on); - - irq_set_irq_wake(pctrl->irq, on); + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return irq_chip_set_wake_parent(d, on); - return 0; + return irq_set_irq_wake(pctrl->irq, on); } static int msm_gpio_irq_reqres(struct irq_data *d)
msmgpio irqchip is not using return value of irq_set_wake call. Start using it. Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy") Signed-off-by: Maulik Shah <mkshah@codeaurora.org> Reviewed-by: Douglas Anderson <dianders@chromium.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)