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

[Lina Iyer]

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@xxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>

[...]

> > @@ -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