Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

From: Lina Iyer <ilina@codeaurora.org>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: swboyd@chromium.org, evgreen@chromium.org,
	linux-kernel@vger.kernel.org, rplsssn@codeaurora.org,
	linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com,
	bjorn.andersson@linaro.org, dianders@chromium.org,
	linus.walleij@linaro.org
Subject: Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
Date: Tue, 12 Mar 2019 10:35:09 -0600	[thread overview]
Message-ID: <20190312163509.GB8553@codeaurora.org> (raw)
In-Reply-To: <6452d538-5714-7e3a-1537-2dd1c4976653@arm.com>

On Mon, Mar 11 2019 at 04:55 -0600, Marc Zyngier wrote:
>On 22/02/2019 22:18, Lina Iyer wrote:
>> To allow GPIOs to wakeup the system from suspend or deep idle, the
>> wakeup capable GPIOs are setup in hierarchy with interrupts from the
>> wakeup-parent irqchip.
>>
>> In older SoC's, the TLMM will handover detection to the parent irqchip
>> and in newer SoC's, the parent irqchip may also be active as well as the
>> TLMM and therefore the GPIOs need to be masked at TLMM to avoid
>> duplicate interrupts. To enable both these configurations to exist,
>> allow the parent irqchip to dictate the TLMM irqchip's behavior when
>> masking/unmasking the interrupt.
>>
>> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

[...]

>> @@ -986,6 +1093,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>  	chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
>>
>>  	pctrl->irq_chip.name = "msmgpio";
>> +	pctrl->irq_chip.irq_eoi	= irq_chip_eoi_parent;
>>  	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
>>  	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
>>  	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>> @@ -994,6 +1102,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>  	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>>  	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>>
>> +	chip->irq.chip = &pctrl->irq_chip;
>> +	chip->irq.domain_ops = &msm_gpio_domain_ops;
>> +	chip->irq.handler = handle_edge_irq;
>> +	chip->irq.default_type = IRQ_TYPE_NONE;
>
>I know you're only moving this around, but can you explain why you're
>setting IRQ_TYPE_NONE in combination with handle_edge_irq? The two
>really should go together. If this doesn't work for some reason, please
>document it.
>
Yes, it was a carry-over from the existing code. I am not sure why that
seems to be a common practice among GPIO drivers.
IRQ_TYPE_EDGE_RISING would be a better option ?

>> +
>> +	dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
>> +	if (dn) {
>> +		chip->irq.parent_domain = irq_find_matching_host(dn,
>> +						 DOMAIN_BUS_WAKEUP);
>> +		of_node_put(dn);
>> +		if (!chip->irq.parent_domain)
>> +			return -EPROBE_DEFER;
>> +
>> +		chip->to_irq = msm_gpio_to_irq;
>> +	}
>> +
>>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>  	if (ret) {
>>  		dev_err(pctrl->dev, "Failed register gpiochip\n");
>> @@ -1015,26 +1139,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>  			dev_name(pctrl->dev), 0, 0, chip->ngpio);
>>  		if (ret) {
>>  			dev_err(pctrl->dev, "Failed to add pin range\n");
>> -			gpiochip_remove(&pctrl->chip);
>> -			return ret;
>> +			goto fail;
>>  		}
>>  	}
>>
>> -	ret = gpiochip_irqchip_add(chip,
>> -				   &pctrl->irq_chip,
>> -				   0,
>> -				   handle_edge_irq,
>> -				   IRQ_TYPE_NONE);
>> -	if (ret) {
>> -		dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
>> -		gpiochip_remove(&pctrl->chip);
>> -		return -ENOSYS;
>> -	}
>> -
>>  	gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
>>  				     msm_gpio_irq_handler);
>>
>>  	return 0;
>> +fail:
>> +	gpiochip_remove(&pctrl->chip);
>> +	return ret;
>>  }
>>

Thanks,
Lina