Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
> Quoting Lina Iyer (2018-12-19 14:11:03)
> > +
> > +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > +                                unsigned int nr_irqs, void *arg)
> > +{
> > +       int ret;
> > +       irq_hw_number_t hwirq;
> > +       struct gpio_chip *gc = domain->host_data;
> > +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +       struct irq_fwspec *fwspec = arg;
> > +       struct qcom_irq_fwspec parent = { };
> > +       unsigned int type;
> > +
> > +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > +                                           &pctrl->irq_chip, gc);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (!domain->parent)
> > +               return 0;
> > +
> > +       parent.fwspec.fwnode      = domain->parent->fwnode;
> > +       parent.fwspec.param_count = 2;
> > +       parent.fwspec.param[0]    = hwirq;
> > +       parent.fwspec.param[1]    = type;
> > +
> > +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (parent.mask)
> > +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * TODO: Get rid of this and push it into gpiochip_to_irq()
>
> Hmm.. yeah we need to do this still. I think we can have a generic two
> cell function similar to irq_domain_xlate_twocell() that does the fwspec
> creation and uses some of the things that we pass to
> gpiochip_irqchip_add(), like the default level type. This existing
> function is not good to have, so there's work to do to get rid of this.

> I was also thinking that maybe we can make the alloc function above take
> a struct gpio_irq_fwspec structure that tells the alloc function what
> gpiochip the irq is for. That would mean that we need to change the
> gpio_to_irq() function below to be generic and stuff the chip inside the
> fwspec wrapper structure:
>
> 	struct gpio_irq_fwspec {
> 		struct irq_fwspec fwspec;
> 		struct gpio_chip *chip;
> 		unsigned int offset;
> 	};
>
> but I seem to recall that was not working for some reason.
I was thinking about this. If I understand you correctly, we want to
generalize the .translate and .alloc functions. We could move the
.translate to generic however, the alloc would still need to be specific
for the parent.mask. But we can do this without the gpio_irq_fwspec. I
presume you suggest this structure so we could pass the hwirq and type
to the .alloc function. but we have that in the fwspec. What am I
missing?

> > + */
> > +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct irq_fwspec fwspec;
> > +
> > +       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
> > +       fwspec.param[0] = offset;
> > +       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
> > +       fwspec.param_count = 2;
> > +
> > +       return irq_create_fwspec_mapping(&fwspec);
> > +}
> > +
> > +static const struct irq_domain_ops msm_gpio_domain_ops = {
> > +       .translate = msm_gpio_domain_translate,
> > +       .alloc     = msm_gpio_domain_alloc,
> > +       .free      = irq_domain_free_irqs_top,
> > +};
> > +