On Wed, Feb 16, 2022 at 03:48:42PM +0000, Marc Zyngier wrote:
...
> > +static irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int
> > pin)
> > +{
> > + const struct mpm_gic_map *maps = priv->maps;
> > + int i;
> > +
> > + for (i = 0; i < priv->map_cnt; i++) {
> > + if (maps[i].pin == pin)
> > + return maps[i].hwirq;
> > + }
> > +
> > + return MPM_NO_PARENT_IRQ;
>
> I'd rather you change this helper to return a pointer to the mapping data,
> and NULL on failure. This will avoid inventing magic values which may
> or may not clash with an interrupt number in the future.
>
> > +}
> > +
> > +static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
> > + unsigned int nr_irqs, void *data)
> > +{
> > + struct qcom_mpm_priv *priv = domain->host_data;
> > + struct irq_fwspec *fwspec = data;
> > + struct irq_fwspec parent_fwspec;
> > + irq_hw_number_t parent_hwirq;
>
> Isn't this the pin number? If so, I'd rather you call it that.
We use term 'pin' in the driver as hwirq of MPM, while the parent_hwirq
here means hwirq of GIC. But it will be dropped anyway as I'm following
your suggestion to check mapping data instead of parent_hwirq.
I will address all other review comments. Thanks, Marc!
Shawn
>
> > + irq_hw_number_t hwirq;
> > + unsigned int type;
> > + int ret;
> > +
> > + ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > + &qcom_mpm_chip, priv);
> > + if (ret)
> > + return ret;
> > +
> > + parent_hwirq = get_parent_hwirq(priv, hwirq);
> > + if (parent_hwirq == MPM_NO_PARENT_IRQ)
> > + return irq_domain_disconnect_hierarchy(domain->parent, virq);
> > +
> > + if (type & IRQ_TYPE_EDGE_BOTH)
> > + type = IRQ_TYPE_EDGE_RISING;
> > +
> > + if (type & IRQ_TYPE_LEVEL_MASK)
> > + type = IRQ_TYPE_LEVEL_HIGH;
> > +
> > + parent_fwspec.fwnode = domain->parent->fwnode;
> > + parent_fwspec.param_count = 3;
> > + parent_fwspec.param[0] = 0;
> > + parent_fwspec.param[1] = parent_hwirq;
> > + parent_fwspec.param[2] = type;
> > +
> > + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > + &parent_fwspec);
> > +}
