| Message ID | 20200717024447.3128361-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros | expand |
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. > + 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 ;-). > + > +#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? > + .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. Thanks, M.
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. > > > + 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. > > + > > +#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? -Saravana
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.
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)) + 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[] = { + +#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 = { \ + .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.
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 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(+)