lkml.org 
[lkml]   [2018]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO
On Fri, Sep 21 2018 at 17:11 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On Tue, 04 Sep 2018 22:18:06 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
[...]
>> +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);
>
>write_relaxed, please.
>
>> + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> + }
>
>Overall, this requires some form of documentation (I'll have forgotten
>about the whole thing quickly enough).
>
Sure, I will address them both.
>> +
>> + 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;
>> + }
>> +
>> + 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);
>
>This message doesn't correspond to what you're doing here.
>
Well, I thought the message is most relevant because, we will not be
able to wake up from this GPIO IRQ. But, I will change it.

>> + kfree(pin_name);
>> + return ret;
>> + }
>> +
>> + irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>> + disable_irq(irq);
>
>Who enables this interrupt?
>
Suspend/resume code does the swap of enable/disable of PDC IRQ vs GPIO
IRQ in patch #4.

>There is a gap between request_irq and disable_irq, where you can take
>the interrupt, and not having set the handler data. Horrible things
>will happen in this situation.
>
>A slightly better way of doing that would be:
>
> // Prevent the interrupt from being enabled on request
> irq_set_status_flags(d->irq, IRQ_NOAUTOEN);
> ret = request_irq(...);
> irq_set_handler(...);
>
>and let the enable_irq() do its thing when it happens (where?).
>
Oh. This is better. Will amend.

Thanks for the review.

-- Lina

>> +
>> + return 0;
>> +}
>> +
>> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
>> +{
>> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>> + const void *name;
>> +
>> + if (pdc_irqd) {
>> + irq_set_handler_data(d->irq, NULL);
>> + name = free_irq(pdc_irqd->irq, d);
>> + kfree(name);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int msm_gpio_irq_reqres(struct irq_data *d)
>> +{
>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> + if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>> + dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>> + irqd_to_hwirq(d));
>> + return -EINVAL;
>> + }
>> +
>> + return msm_gpio_pdc_pin_request(d);
>> +}
>> +
>> +static void msm_gpio_irq_relres(struct irq_data *d)
>> +{
>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> + msm_gpio_pdc_pin_release(d);
>> + gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>> +}
>> +
>> static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> {
>> struct gpio_chip *chip;
>> @@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>> pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
>> pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>> + pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>> + pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>>
>> ret = gpiochip_add_data(&pctrl->chip, pctrl);
>> if (ret) {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
>Thanks,
>
> M.
>
>--
>Jazz is not dead, it just smell funny.

\
 
 \ /
  Last update: 2018-09-21 18:09    [W:0.316 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site