[EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Sun, 14 Aug 2022 04:12:01 +0100,
Frank Li <frank.li at nxp.com> wrote:

> > > new file mode 100644
> > > index 0000000000000..bb111412d598f
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > > @@ -0,0 +1,443 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NXP MU worked as MSI controller
> > 
> > Freescale? Or NXP? Please make up your mind.
> 
> 
> [Frank Li] NXP and freescale is the same thing. 
> It is mux used at many place.

Pick one, and stick to it. Having two names for the same thing is
pointlessly confusing.

> > > +static struct irq_chip imx_mu_msi_irq_chip = {
> > > +     .name = "MU-MSI",
> > > +     .irq_ack = irq_chip_ack_parent,
> > 
> > Crucially, no irq_write_msi_msg callback. So we happily inherit
> > platform_msi_write_msg() and use the per descriptor write_msg()
> > callback. Who sets this? Nobody.
> 
> [Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS, 
>  irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();

That wasn't my question. But never mind, I found the call to
platform_msi_domain_alloc_irqs() in patch #4.

> > > +
> > > +     /* Initialize MSI domain parent */
> > > +     msi_data->parent = irq_domain_create_linear(fwnodes,
> > > +                                                 IMX_MU_CHANS,
> > > +                                                 &imx_mu_msi_domain_ops,
> > > +                                                 msi_data);
> > 
> > Consider setting the bus_token attribute for this domain to something
> > that isn't the default, as it otherwise clashes with the following
> > creation.
> 
> [Frank Li] Any suggestion? Which bus_token is good?

DOMAIN_BUS_NEXUS is what other drivers use.

> > > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> > 
> > I'm sorry, but you'll have to come up with something slightly more
> > descriptive than "a" or "b". At least add a qualifier to it. Same
> > thing for the DT by the way.
> 
> [Frank Li] MU spec using  term "A side" and "B side".  So I think "a" and "b"
> is enough.

No, it really isn't.

> 
> Or do you think "a-side" is better?

No, I would like something fully descriptive. The DT actually has
"Processor A-facing", which seems like a reasonable description.

	M.

-- 
Without deviation from the norm, progress is not possible.