Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros

[Marc Zyngier <maz@kernel.org>]

On 2020-07-17 18:50, Saravana Kannan wrote:
> On Fri, Jul 17, 2020 at 3:49 AM Marc Zyngier <maz@kernel.org> wrote:
>> 
>> Hi Saravana,
>> 
>> Thanks for re-spinning this one.
>> 
>> On Fri, 17 Jul 2020 03:44:47 +0100,
>> Saravana Kannan <saravanak@google.com> wrote:
>> >
>> > Compiling an irqchip driver as a platform driver needs to bunch of
>> > things to be done right:
>> > - Making sure the parent domain is initialized first
>> > - Making sure the device can't be unbound from sysfs
>> > - Disallowing module unload if it's built as a module
>> > - Finding the parent node
>> > - Etc.
>> >
>> > Instead of trying to make sure all future irqchip platform drivers get
>> > this right, provide boilerplate macros that take care of all of this.
>> >
>> > An example use would look something like this. Where acme_foo_init and
>> > acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE.
>> >
>> > IRQCHIP_PLATFORM_DRIVER_BEGIN
>> 
>> I think there is some value in having the BEGIN statement containing
>> the driver name, see below.
>> 
>> > IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init)
>> > IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init)
>> > IRQCHIP_PLATFORM_DRIVER_END(acme_irq)
>> >
>> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>> > ---
>> >  drivers/irqchip/irqchip.c | 22 ++++++++++++++++++++++
>> >  include/linux/irqchip.h   | 23 +++++++++++++++++++++++
>> >  2 files changed, 45 insertions(+)
>> >
>> > diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
>> > index 2b35e68bea82..236ea793f01c 100644
>> > --- a/drivers/irqchip/irqchip.c
>> > +++ b/drivers/irqchip/irqchip.c
>> > @@ -10,8 +10,10 @@
>> >
>> >  #include <linux/acpi.h>
>> >  #include <linux/init.h>
>> > +#include <linux/of_device.h>
>> >  #include <linux/of_irq.h>
>> >  #include <linux/irqchip.h>
>> > +#include <linux/platform_device.h>
>> >
>> >  /*
>> >   * This special of_device_id is the sentinel at the end of the
>> > @@ -29,3 +31,23 @@ void __init irqchip_init(void)
>> >       of_irq_init(__irqchip_of_table);
>> >       acpi_probe_device_table(irqchip);
>> >  }
>> > +
>> > +int platform_irqchip_probe(struct platform_device *pdev)
>> > +{
>> > +     struct device_node *np = pdev->dev.of_node;
>> > +     struct device_node *par_np = of_irq_find_parent(np);
>> > +     of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
>> > +
>> > +     if (!irq_init_cb)
>> > +             return -EINVAL;
>> > +
>> > +     if (par_np == np)
>> > +             par_np = NULL;
>> > +
>> > +     /* If there's a parent irqchip, make sure it has been initialized. */
>> > +     if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY))
>> 
>> There is no guarantee that the calling driver wants BUS_ANY as a token
>> for its parent. It may work for now, if you only have dependencies to
>> drivers that only expose a single domain, but that's not a general
>> purpose check..
>> 
>> At least, please add a comment saying that the new driver may want to
>> check that the irqdomain it depends on may not be available.
> 
> This is just checking if the parent interrupt controller has been
> initialized. It's just saying that if NONE of the parent irq domains
> have been registered, it's not time for this interrupt controller to
> initialize. And yes, as you said, the actual init code can do more
> checks and defer probe too. Maybe I'll just put the 2nd sentence as
> the comment.

Sure, go ahead.

> 
>> 
>> > +             return -EPROBE_DEFER;
>> > +
>> > +     return irq_init_cb(np, par_np);
>> > +}
>> > +EXPORT_SYMBOL_GPL(platform_irqchip_probe);
>> > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
>> > index 950e4b2458f0..6d5eba7cbbb7 100644
>> > --- a/include/linux/irqchip.h
>> > +++ b/include/linux/irqchip.h
>> > @@ -13,6 +13,7 @@
>> >
>> >  #include <linux/acpi.h>
>> >  #include <linux/of.h>
>> > +#include <linux/platform_device.h>
>> >
>> >  /*
>> >   * This macro must be used by the different irqchip drivers to declare
>> > @@ -26,6 +27,28 @@
>> >   */
>> >  #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn)
>> >
>> > +extern int platform_irqchip_probe(struct platform_device *pdev);
>> > +
>> > +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \
>> > +static const struct of_device_id __irqchip_match_table[] = {
>> 
>> How about:
>> 
>> #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \
>> static const struct of_device_id __irqchip_match_table_##drv_name = {
>> 
>> which makes it easier to debug when you want to identify specific
>> structures in an object (otherwise, they all have the same
>> name...). it is also much more pleasing aesthetically ;-).
> 
> I totally agree. I wanted BEGIN to have the name and END to not have
> to specify the name. But I couldn't figure out a way to do it. I
> assumed you wouldn't want the names repeated in both BEGIN and END. If
> you are okay with that, I prefer your suggestion too.

I'm perfectly fine having the name in both the BEGIN and END tags.
It has a nice LaTeX twist to it ;-).

> 
>> > +
>> > +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn },
>> > +
>> > +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name)                \
>> > +     {},                                             \
>> > +};                                                   \
>> > +MODULE_DEVICE_TABLE(of, __irqchip_match_table);              \
>> > +static struct platform_driver drv_name##_driver = {  \
>> 
>> const?
> 
> Sure.
> 
>> > +     .probe  = platform_irqchip_probe,               \
>> > +     .driver = {                                     \
>> > +             .name = #drv_name,                      \
>> > +             .owner = THIS_MODULE,                   \
>> > +             .of_match_table = __irqchip_match_table,\
>> > +             .suppress_bind_attrs = true,            \
>> > +     },                                              \
>> > +};                                                   \
>> > +builtin_platform_driver(drv_name##_driver)
>> > +
>> >  /*
>> >   * This macro must be used by the different irqchip drivers to declare
>> >   * the association between their version and their initialization function.
>> > --
>> > 2.28.0.rc0.105.gf9edc3c819-goog
>> >
>> >
>> 
>> Otherwise looks good. When you respin it, it would be good to also
>> submit one user of this API by converting an existing driver, as I'd
>> hate to merge something that has no user.
> 
> The only one I know will work is the qcom pdc one from John. So I was
> hoping John would respin his patch if you accept this one or I was
> going to redo it after it shows up on linux-next. Maybe MTK can use
> this too for their other series?

I have queued John's PDC work in irq/irqchip-5.9, which I will get
into -next over the weekend. Feel free to post a patch reworking
his last patch, which will give a very nice overview of what we gain.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel