Message ID | 20180824200157.9993-1-ilina@codeaurora.org |
---|---|
Headers | show |
Series | Wakeup GPIO support for SDM845 SoC | expand |
Hi Lina, On Fri, Aug 24, 2018 at 02:01:53PM -0600, Lina Iyer wrote: > QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on > domain can wakeup the SoC, when interrupts and GPIOs are routed to the > its interrupt controller. Only select GPIOs that are deemed wakeup wording nit: "are routed to the|its interrupt controller" > capable are routed to specific PDC pins. During low power state, the > pinmux interrupt controller may be non-functional but the PDC would be. > The PDC can detect the wakeup GPIO is triggered and bring the TLMM to an > operational state. > > Interrupts that are level triggered will be detected at the TLMM when > the controller becomes operational. Edge interrupts however need to be > replayed again. > > Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ, > but keep it disabled. During suspend, we can enable the PDC IRQ instead > of the GPIO IRQ, which may or not be detected. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > Changes in v2: > - Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ > Changes in v1: > - Trigger GPIO in h/w from PDC IRQ handler > - Avoid big tables for GPIO-PDC map, pick from DT instead > - Use handler_data > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 96 ++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 0e22f52b2a19..b675ea56a4ff 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > const struct msm_pingroup *g; > unsigned long flags; > u32 val; > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > > g = &pctrl->soc->groups[d->hwirq]; > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > + if (pdc_irqd) > + irq_set_irq_type(pdc_irqd->irq, type); > + > /* > * For hw without possibility of detecting both edges > */ > @@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > unsigned long flags; > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > + if (pdc_irqd) > + irq_set_irq_wake(pdc_irqd->irq, on); > + > irq_set_irq_wake(pctrl->irq, on); > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > @@ -863,6 +871,92 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) > return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0; > } > > +static irqreturn_t wake_irq_gpio_handler(int irq, void *data) > +{ > + struct irq_data *irqd = data; > + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + const struct msm_pingroup *g; > + unsigned long flags; > + u32 val; > + > + if (!irqd_is_level_type(irqd)) { > + g = &pctrl->soc->groups[irqd->hwirq]; > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + val = BIT(g->intr_status_bit); > + writel(val, pctrl->regs + g->intr_status_reg); > + raw_spin_unlock_irqrestore(&pctrl->lock, flags); > + } > + > + return IRQ_HANDLED; > +} > + > +static int msm_gpio_pdc_pin_request(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + struct platform_device *pdev = to_platform_device(pctrl->dev); > + const char *pin_name; > + int irq; > + int ret; > + > + pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq); > + if (!pin_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname(pdev, pin_name); > + if (irq < 0) { > + kfree(pin_name); > + return 0; Do I understand correctly that this is the case where the pin isn't routed to the PDC? > + } > + > + ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d), > + pin_name, d); > + if (ret) { > + pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq); '\n' is missing > + kfree(pin_name); > + return ret; > + } > + > + irq_set_handler_data(d->irq, irq_get_irq_data(irq)); > + disable_irq(irq); > + > + return 0; > +} > + > +static int msm_gpio_pdc_pin_release(struct irq_data *d) > +{ > + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); > + > + if (pdc_irqd) { > + irq_set_handler_data(d->irq, NULL); > + free_irq(pdc_irqd->irq, d); You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request(). IIUC it should be available in irq_desc->action->name. Cheers Matthias
Hi Lina, On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer wrote: > During suspend the system may power down some of the system rails. As a > result, the TLMM hw block may not be operational anymore and wakeup > capable GPIOs will not be detected. The PDC however will be operational > and the GPIOs that are routed to the PDC as IRQs can wake the system up. > > To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a > GPIO trips, use TLMM for active and switch to PDC for suspend. When > entering suspend, disable the TLMM wakeup interrupt and instead enable > the PDC IRQ and revert upon resume. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > Changes in v2: > - Fix PDC IRQ max port, 126 is the max supported in h/w > - Use PDC hwirq in bitmap, linux numbers could be large > - Setup DISABLE_UNLAZY for both TLMM and PDC IRQs > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 70 +++++++++++++++++++++++++++++- > drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++ > 2 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index b675ea56a4ff..a880cefbd248 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -37,6 +37,7 @@ > #include "../pinctrl-utils.h" > > #define MAX_NR_GPIO 300 > +#define MAX_PDC_HWIRQ 126 > #define PS_HOLD_OFFSET 0x820 > > /** > @@ -51,6 +52,7 @@ > * @enabled_irqs: Bitmap of currently enabled irqs. > * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge > * detection. > + * @pdc_hwirqs: Bitmap of wakeup capable irqs. > * @soc; Reference to soc_data of platform specific data. > * @regs: Base address for the TLMM register map. > */ > @@ -68,11 +70,15 @@ struct msm_pinctrl { > > DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); > DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); > + DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ); > > const struct msm_pinctrl_soc_data *soc; > void __iomem *regs; > + struct irq_domain *pdc_irq_domain; > }; > > +static bool in_suspend; > + > static int msm_get_groups_count(struct pinctrl_dev *pctldev) > { > struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > - if (pdc_irqd) > + if (pdc_irqd && !in_suspend) { > irq_set_irq_wake(pdc_irqd->irq, on); > + if (on) > + set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); > + else > + clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); > + } > > irq_set_irq_wake(pctrl->irq, on); > > @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d) > } > > irq_set_handler_data(d->irq, irq_get_irq_data(irq)); > + irq_set_handler_data(irq, d); > + irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY); > + irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); > disable_irq(irq); > + if (!pctrl->pdc_irq_domain) > + pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain; > > return 0; > } > @@ -1069,6 +1085,58 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) > } > } > > +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev) > +{ > + struct msm_pinctrl *pctrl = dev_get_drvdata(dev); > + struct irq_data *irqd; > + unsigned int irq; > + int i; > + > + in_suspend = true; > + for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) { > + irq = irq_find_mapping(pctrl->pdc_irq_domain, i); > + irqd = irq_get_handler_data(irq); > + /* > + * We don't know if the TLMM will be functional > + * or not, during the suspend. If its functional, > + * we do not want duplicate interrupts from PDC. > + * Hence disable the GPIO IRQ and enable PDC IRQ. > + */ > + if (irqd_is_wakeup_set(irqd)) { > + disable_irq_wake(irqd->irq); > + disable_irq(irqd->irq); > + enable_irq(irq); > + } Would it make sense to limit this to edge triggered interrupts since the interrupt handler does nothing for level triggered ones? Cheers Matthias
On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote: > Enable TLMM IRQs to be sensed by PDC when we enter suspend. It is > possible that the TLMM may be powered off and not detect GPIOs that are > configured as wake up interrupts. By hooking into suspend callbacks, we > allow PDC IRQs to take over and wake up the system if wakeup interrupts > are triggered. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c > index 2ab7a8885757..cc333b8afb99 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c > +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c > @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = { > { }, > }; > > +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late, > + msm_pinctrl_resume_late) > +}; > + I expect these four lines to be duplicated in every platform file, so I think it would be better to just move it to pinctrl-msm.c and extern declare it in pinctrl-msm.h. > static struct platform_driver sdm845_pinctrl_driver = { > .driver = { > .name = "sdm845-pinctrl", > .of_match_table = sdm845_pinctrl_of_match, > + .pm = &msm_pinctrl_dev_pm_ops, > }, > .probe = sdm845_pinctrl_probe, > .remove = msm_pinctrl_remove, Regards, Bjorn
On Mon, Aug 27 2018 at 18:31 -0600, Bjorn Andersson wrote: >On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote: > >> Enable TLMM IRQs to be sensed by PDC when we enter suspend. It is >> possible that the TLMM may be powered off and not detect GPIOs that are >> configured as wake up interrupts. By hooking into suspend callbacks, we >> allow PDC IRQs to take over and wake up the system if wakeup interrupts >> are triggered. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c >> index 2ab7a8885757..cc333b8afb99 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c >> @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = { >> { }, >> }; >> >> +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = { >> + SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late, >> + msm_pinctrl_resume_late) >> +}; >> + > >I expect these four lines to be duplicated in every platform file, so I >think it would be better to just move it to pinctrl-msm.c and extern >declare it in pinctrl-msm.h. > Sure. >> static struct platform_driver sdm845_pinctrl_driver = { >> .driver = { >> .name = "sdm845-pinctrl", >> .of_match_table = sdm845_pinctrl_of_match, >> + .pm = &msm_pinctrl_dev_pm_ops, >> }, >> .probe = sdm845_pinctrl_probe, >> .remove = msm_pinctrl_remove, > >Regards, >Bjorn
On Mon, Aug 27 2018 at 16:57 -0600, Matthias Kaehlcke wrote: >Hi Lina, > >On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer wrote: >> During suspend the system may power down some of the system rails. As a >> result, the TLMM hw block may not be operational anymore and wakeup >> capable GPIOs will not be detected. The PDC however will be operational >> and the GPIOs that are routed to the PDC as IRQs can wake the system up. >> >> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a >> GPIO trips, use TLMM for active and switch to PDC for suspend. When >> entering suspend, disable the TLMM wakeup interrupt and instead enable >> the PDC IRQ and revert upon resume. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> Changes in v2: >> - Fix PDC IRQ max port, 126 is the max supported in h/w >> - Use PDC hwirq in bitmap, linux numbers could be large >> - Setup DISABLE_UNLAZY for both TLMM and PDC IRQs >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 70 +++++++++++++++++++++++++++++- >> drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++ >> 2 files changed, 72 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index b675ea56a4ff..a880cefbd248 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -37,6 +37,7 @@ >> #include "../pinctrl-utils.h" >> >> #define MAX_NR_GPIO 300 >> +#define MAX_PDC_HWIRQ 126 >> #define PS_HOLD_OFFSET 0x820 >> >> /** >> @@ -51,6 +52,7 @@ >> * @enabled_irqs: Bitmap of currently enabled irqs. >> * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge >> * detection. >> + * @pdc_hwirqs: Bitmap of wakeup capable irqs. >> * @soc; Reference to soc_data of platform specific data. >> * @regs: Base address for the TLMM register map. >> */ >> @@ -68,11 +70,15 @@ struct msm_pinctrl { >> >> DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); >> DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); >> + DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ); >> >> const struct msm_pinctrl_soc_data *soc; >> void __iomem *regs; >> + struct irq_domain *pdc_irq_domain; >> }; >> >> +static bool in_suspend; >> + >> static int msm_get_groups_count(struct pinctrl_dev *pctldev) >> { >> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); >> @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> - if (pdc_irqd) >> + if (pdc_irqd && !in_suspend) { >> irq_set_irq_wake(pdc_irqd->irq, on); >> + if (on) >> + set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); >> + else >> + clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs); >> + } >> >> irq_set_irq_wake(pctrl->irq, on); >> >> @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d) >> } >> >> irq_set_handler_data(d->irq, irq_get_irq_data(irq)); >> + irq_set_handler_data(irq, d); >> + irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY); >> + irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); >> disable_irq(irq); >> + if (!pctrl->pdc_irq_domain) >> + pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain; >> >> return 0; >> } >> @@ -1069,6 +1085,58 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) >> } >> } >> >> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev) >> +{ >> + struct msm_pinctrl *pctrl = dev_get_drvdata(dev); >> + struct irq_data *irqd; >> + unsigned int irq; >> + int i; >> + >> + in_suspend = true; >> + for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) { >> + irq = irq_find_mapping(pctrl->pdc_irq_domain, i); >> + irqd = irq_get_handler_data(irq); >> + /* >> + * We don't know if the TLMM will be functional >> + * or not, during the suspend. If its functional, >> + * we do not want duplicate interrupts from PDC. >> + * Hence disable the GPIO IRQ and enable PDC IRQ. >> + */ >> + if (irqd_is_wakeup_set(irqd)) { >> + disable_irq_wake(irqd->irq); >> + disable_irq(irqd->irq); >> + enable_irq(irq); >> + } > >Would it make sense to limit this to edge triggered interrupts since >the interrupt handler does nothing for level triggered ones? > Sure. -- Lina
On Mon, Aug 27 2018 at 16:35 -0600, Matthias Kaehlcke wrote: >Hi Lina, > >On Fri, Aug 24, 2018 at 02:01:53PM -0600, Lina Iyer wrote: >> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on >> domain can wakeup the SoC, when interrupts and GPIOs are routed to the >> its interrupt controller. Only select GPIOs that are deemed wakeup > >wording nit: "are routed to the|its interrupt controller" > Okay. >> capable are routed to specific PDC pins. During low power state, the >> pinmux interrupt controller may be non-functional but the PDC would be. >> The PDC can detect the wakeup GPIO is triggered and bring the TLMM to an >> operational state. >> >> Interrupts that are level triggered will be detected at the TLMM when >> the controller becomes operational. Edge interrupts however need to be >> replayed again. >> >> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ, >> but keep it disabled. During suspend, we can enable the PDC IRQ instead >> of the GPIO IRQ, which may or not be detected. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> Changes in v2: >> - Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ >> Changes in v1: >> - Trigger GPIO in h/w from PDC IRQ handler >> - Avoid big tables for GPIO-PDC map, pick from DT instead >> - Use handler_data >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 96 ++++++++++++++++++++++++++++++ >> 1 file changed, 96 insertions(+) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index 0e22f52b2a19..b675ea56a4ff 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) >> const struct msm_pingroup *g; >> unsigned long flags; >> u32 val; >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> >> g = &pctrl->soc->groups[d->hwirq]; >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> + if (pdc_irqd) >> + irq_set_irq_type(pdc_irqd->irq, type); >> + >> /* >> * For hw without possibility of detecting both edges >> */ >> @@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> unsigned long flags; >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> + if (pdc_irqd) >> + irq_set_irq_wake(pdc_irqd->irq, on); >> + >> irq_set_irq_wake(pctrl->irq, on); >> >> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> @@ -863,6 +871,92 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) >> return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0; >> } >> >> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data) >> +{ >> + struct irq_data *irqd = data; >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + const struct msm_pingroup *g; >> + unsigned long flags; >> + u32 val; >> + >> + if (!irqd_is_level_type(irqd)) { >> + g = &pctrl->soc->groups[irqd->hwirq]; >> + raw_spin_lock_irqsave(&pctrl->lock, flags); >> + val = BIT(g->intr_status_bit); >> + writel(val, pctrl->regs + g->intr_status_reg); >> + raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int msm_gpio_pdc_pin_request(struct irq_data *d) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + struct platform_device *pdev = to_platform_device(pctrl->dev); >> + const char *pin_name; >> + int irq; >> + int ret; >> + >> + pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq); >> + if (!pin_name) >> + return -ENOMEM; >> + >> + irq = platform_get_irq_byname(pdev, pin_name); >> + if (irq < 0) { >> + kfree(pin_name); >> + return 0; > >Do I understand correctly that this is the case where the pin isn't >routed to the PDC? > Yes, correct. >> + } >> + >> + ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d), >> + pin_name, d); >> + if (ret) { >> + pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq); > >'\n' is missing > ok. >> + kfree(pin_name); >> + return ret; >> + } >> + >> + irq_set_handler_data(d->irq, irq_get_irq_data(irq)); >> + disable_irq(irq); >> + >> + return 0; >> +} >> + >> +static int msm_gpio_pdc_pin_release(struct irq_data *d) >> +{ >> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq); >> + >> + if (pdc_irqd) { >> + irq_set_handler_data(d->irq, NULL); >> + free_irq(pdc_irqd->irq, d); > >You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request(). >IIUC it should be available in irq_desc->action->name. > Yes, I didn't realize that free_irq returns the name when I posted this series (noted in the cover letter). Will fix. -- Lina