Re: [PATCH 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver

[Neil Armstrong]

Hi Chris,

On 17/07/2022 22:58, Chris Healy wrote:
[...]

> > [...]
> > > > > +        goto err2;
> > > > > +    }
> > > > > +
> > > > > +    irq_name = of_get_property(node, "interrupt-names", NULL);
> > > > > +    if (!irq_name)
> > > > > +        irq_name = "ddr_pmu";
> > > >
> > > > That's not how the "interrupt-names" property works. If you only have
> > > > a single interrupt then there's not much need for it to be named in
> > > > the DT at all. If you do want to use named interrupts then use
> > > > platform_get_irq_byname(), and the name should probably have a bnit
> > > > more functional meaning. Either way, please don't abuse the DT like this.
> > > Okay, actually there will be multiple interrupts , but not in current
> > > G12 series.
> >
> > That's fair enough, so we should try to anticipate it in the design of
> > the DT binding. If for instance future SoCs are going to move from
> > having a single combined overflow interrupt to a separate interrupt for
> > each counter, then the driver can reasonably continue to get them by
> > index and we'll effectively only need to update maxItems in the binding.
> > If on the other hand there's still going to be one combined overflow
> > interrupt, plus some other new interrupt for something completely
> > different, then it *could* be more appropriate to have names, and thus
> > to define and use a standard "overflow" name from the beginning even
> > when it is the only one present, so that we can remain consistent later
> > once more are added.
>
> My assumption is that the goal of having this "interrupt-names" in DT
> is to cover future cases where there is more than one DRAM controller
> instance in the SoC and you want to be able to discriminate between
> the two instances with this driver's interrupt name.  If this is true,
> as an alternative, you could do something like this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-mt65xx.c?h=v5.19-rc6&id=7fb9dc8109bf9713ffcda65617249099a1942f0f
>
> This should result in each instance having a unique name that includes
> the base address as the prefix to the interrupt name which should be
> sufficient for determining which instance is which.

It's ok to introduce interrupt-names in the bindings for newer SoCs,
since it's useless for the current ones, there's no need to introduce it right now.

It's also why it's simpler to introduce a compatible per SoC, so we can add
different attributes in the bindings depending on the compatible.

Neil

[...]