| Message ID | 20211202122122.23548-2-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm MPM irqchip driver support | expand |
On 12/2/21 4:21 AM, Shawn Guo wrote: > It makes sense to just pass device_node for callback in IRQCHIP_DECLARE > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because > platform_driver probe/init usually needs device pointer for various > purposes, e.g. resource allocation, service request, device prefixed > message output, etc. Create a new callback type irqchip_init_cb_t which > takes platform_device pointer as parameter, and update the existing > IRQCHIP_PLATFORM_DRIVER users accordingly. > > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Could you copy all recipients on all 3 patches plus your cover letter next time so we have the full context? Thanks! [snip] > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev, > struct device_node *parent) > { > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, > + bcm7120_l2_intc_iomap_7120, > "BCM7120 L2"); If you look further into that driver, you will see that we do something like this in bcm7120_l2_intc_probe: pdev = of_find_device_by_node(dn); if (!pdev) { ret = -ENODEV; goto out_free_data; } which would be completely superfluous now that we pass a platform_device directly. Can you rework your patch so as to eliminate that of_find_device_by_ndoe() (and the companion put_device call)?
On 2021-12-02 17:52, Florian Fainelli wrote: > On 12/2/21 4:21 AM, Shawn Guo wrote: >> It makes sense to just pass device_node for callback in >> IRQCHIP_DECLARE >> case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because >> platform_driver probe/init usually needs device pointer for various >> purposes, e.g. resource allocation, service request, device prefixed >> message output, etc. Create a new callback type irqchip_init_cb_t >> which >> takes platform_device pointer as parameter, and update the existing >> IRQCHIP_PLATFORM_DRIVER users accordingly. >> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> >> Cc: Neil Armstrong <narmstrong@baylibre.com> >> Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Could you copy all recipients on all 3 patches plus your cover letter > next time so we have the full context? Thanks! > > [snip] > >> >> -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, >> +static int __init bcm7120_l2_intc_probe_7120(struct platform_device >> *pdev, >> struct device_node *parent) >> { >> - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, >> + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, >> + bcm7120_l2_intc_iomap_7120, >> "BCM7120 L2"); > > If you look further into that driver, you will see that we do something > like this in bcm7120_l2_intc_probe: > > pdev = of_find_device_by_node(dn); > if (!pdev) { > ret = -ENODEV; > goto out_free_data; > } > > which would be completely superfluous now that we pass a > platform_device > directly. Can you rework your patch so as to eliminate that > of_find_device_by_ndoe() (and the companion put_device call)? Or just adopt the same construct in the MPM driver. At this stage, drivers requiring a platform_device are the minority. M.
On 12/2/21 11:10 AM, Marc Zyngier wrote: > On 2021-12-02 17:52, Florian Fainelli wrote: >> On 12/2/21 4:21 AM, Shawn Guo wrote: >>> It makes sense to just pass device_node for callback in IRQCHIP_DECLARE >>> case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because >>> platform_driver probe/init usually needs device pointer for various >>> purposes, e.g. resource allocation, service request, device prefixed >>> message output, etc. Create a new callback type irqchip_init_cb_t which >>> takes platform_device pointer as parameter, and update the existing >>> IRQCHIP_PLATFORM_DRIVER users accordingly. >>> >>> Cc: Florian Fainelli <f.fainelli@gmail.com> >>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> >>> Cc: Neil Armstrong <narmstrong@baylibre.com> >>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org> >> >> Could you copy all recipients on all 3 patches plus your cover letter >> next time so we have the full context? Thanks! >> >> [snip] >> >>> >>> -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, >>> +static int __init bcm7120_l2_intc_probe_7120(struct platform_device >>> *pdev, >>> struct device_node *parent) >>> { >>> - return bcm7120_l2_intc_probe(dn, parent, >>> bcm7120_l2_intc_iomap_7120, >>> + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, >>> + bcm7120_l2_intc_iomap_7120, >>> "BCM7120 L2"); >> >> If you look further into that driver, you will see that we do something >> like this in bcm7120_l2_intc_probe: >> >> pdev = of_find_device_by_node(dn); >> if (!pdev) { >> ret = -ENODEV; >> goto out_free_data; >> } >> >> which would be completely superfluous now that we pass a platform_device >> directly. Can you rework your patch so as to eliminate that >> of_find_device_by_ndoe() (and the companion put_device call)? > > Or just adopt the same construct in the MPM driver. At this stage, drivers > requiring a platform_device are the minority. Works for me.
On Thu, Dec 02, 2021 at 09:52:55AM -0800, Florian Fainelli wrote: > On 12/2/21 4:21 AM, Shawn Guo wrote: > > It makes sense to just pass device_node for callback in IRQCHIP_DECLARE > > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because > > platform_driver probe/init usually needs device pointer for various > > purposes, e.g. resource allocation, service request, device prefixed > > message output, etc. Create a new callback type irqchip_init_cb_t which > > takes platform_device pointer as parameter, and update the existing > > IRQCHIP_PLATFORM_DRIVER users accordingly. > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Could you copy all recipients on all 3 patches plus your cover letter > next time so we have the full context? Thanks! > > [snip] > > > > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, > > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev, > > struct device_node *parent) > > { > > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, > > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, > > + bcm7120_l2_intc_iomap_7120, > > "BCM7120 L2"); > > If you look further into that driver, you will see that we do something > like this in bcm7120_l2_intc_probe: > > pdev = of_find_device_by_node(dn); > if (!pdev) { > ret = -ENODEV; > goto out_free_data; > } > > which would be completely superfluous now that we pass a platform_device > directly. Can you rework your patch so as to eliminate that > of_find_device_by_ndoe() (and the companion put_device call)? Firstly, I do not see any companion put_device call in the driver. Secondly, the existing code seems to have some problem in the "out" order. The out_unmap should go before out_free_l1_data, right? @@ -329,13 +323,13 @@ static int __init bcm7120_l2_intc_probe(struct device_node *dn, out_free_domain: irq_domain_remove(data->domain); -out_free_l1_data: - kfree(data->l1_data); out_unmap: for (idx = 0; idx < MAX_MAPPINGS; idx++) { if (data->map_base[idx]) iounmap(data->map_base[idx]); } +out_free_l1_data: + kfree(data->l1_data); out_free_data: kfree(data); return ret; Shawn
On Thu, Dec 02, 2021 at 07:10:04PM +0000, Marc Zyngier wrote: > On 2021-12-02 17:52, Florian Fainelli wrote: > > On 12/2/21 4:21 AM, Shawn Guo wrote: > > > It makes sense to just pass device_node for callback in > > > IRQCHIP_DECLARE > > > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because > > > platform_driver probe/init usually needs device pointer for various > > > purposes, e.g. resource allocation, service request, device prefixed > > > message output, etc. Create a new callback type irqchip_init_cb_t > > > which > > > takes platform_device pointer as parameter, and update the existing > > > IRQCHIP_PLATFORM_DRIVER users accordingly. > > > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > > > Could you copy all recipients on all 3 patches plus your cover letter > > next time so we have the full context? Thanks! > > > > [snip] > > > > > > > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, > > > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device > > > *pdev, > > > struct device_node *parent) > > > { > > > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, > > > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, > > > + bcm7120_l2_intc_iomap_7120, > > > "BCM7120 L2"); > > > > If you look further into that driver, you will see that we do something > > like this in bcm7120_l2_intc_probe: > > > > pdev = of_find_device_by_node(dn); > > if (!pdev) { > > ret = -ENODEV; > > goto out_free_data; > > } > > > > which would be completely superfluous now that we pass a platform_device > > directly. Can you rework your patch so as to eliminate that > > of_find_device_by_ndoe() (and the companion put_device call)? > > Or just adopt the same construct in the MPM driver. At this stage, drivers > requiring a platform_device are the minority. Marc, I need to ensure I understand you comment. Are you suggesting that I keep IRQCHIP_MATCH() unchanged, and go back to the MPM driver construction I used in v2? Shawn
On Mon, 06 Dec 2021 06:40:05 +0000, Shawn Guo <shawn.guo@linaro.org> wrote: > > On Thu, Dec 02, 2021 at 07:10:04PM +0000, Marc Zyngier wrote: > > On 2021-12-02 17:52, Florian Fainelli wrote: > > > On 12/2/21 4:21 AM, Shawn Guo wrote: > > > > It makes sense to just pass device_node for callback in > > > > IRQCHIP_DECLARE > > > > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because > > > > platform_driver probe/init usually needs device pointer for various > > > > purposes, e.g. resource allocation, service request, device prefixed > > > > message output, etc. Create a new callback type irqchip_init_cb_t > > > > which > > > > takes platform_device pointer as parameter, and update the existing > > > > IRQCHIP_PLATFORM_DRIVER users accordingly. > > > > > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com> > > > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > > > > > Could you copy all recipients on all 3 patches plus your cover letter > > > next time so we have the full context? Thanks! > > > > > > [snip] > > > > > > > > > > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, > > > > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device > > > > *pdev, > > > > struct device_node *parent) > > > > { > > > > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, > > > > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, > > > > + bcm7120_l2_intc_iomap_7120, > > > > "BCM7120 L2"); > > > > > > If you look further into that driver, you will see that we do something > > > like this in bcm7120_l2_intc_probe: > > > > > > pdev = of_find_device_by_node(dn); > > > if (!pdev) { > > > ret = -ENODEV; > > > goto out_free_data; > > > } > > > > > > which would be completely superfluous now that we pass a platform_device > > > directly. Can you rework your patch so as to eliminate that > > > of_find_device_by_ndoe() (and the companion put_device call)? > > > > Or just adopt the same construct in the MPM driver. At this stage, drivers > > requiring a platform_device are the minority. > > Marc, > > I need to ensure I understand you comment. Are you suggesting that I > keep IRQCHIP_MATCH() unchanged, and go back to the MPM driver > construction I used in v2? No. I suggest that you leave the irqchip API as is (i.e. drop this patch) and use of_find_device_by_node() in the MPM driver, just like the Broadcom driver does. This should be enough for your use case. M.
diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c index a62b96237b82..d52a598f73df 100644 --- a/drivers/irqchip/irq-bcm7038-l1.c +++ b/drivers/irqchip/irq-bcm7038-l1.c @@ -396,9 +396,10 @@ static const struct irq_domain_ops bcm7038_l1_domain_ops = { .map = bcm7038_l1_map, }; -static int __init bcm7038_l1_of_init(struct device_node *dn, +static int __init bcm7038_l1_of_init(struct platform_device *pdev, struct device_node *parent) { + struct device_node *dn = pdev->dev.of_node; struct bcm7038_l1_chip *intc; int idx, ret; diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c index d80e67a6aad2..82a75eb11523 100644 --- a/drivers/irqchip/irq-bcm7120-l2.c +++ b/drivers/irqchip/irq-bcm7120-l2.c @@ -341,17 +341,19 @@ static int __init bcm7120_l2_intc_probe(struct device_node *dn, return ret; } -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn, +static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev, struct device_node *parent) { - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120, + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, + bcm7120_l2_intc_iomap_7120, "BCM7120 L2"); } -static int __init bcm7120_l2_intc_probe_3380(struct device_node *dn, +static int __init bcm7120_l2_intc_probe_3380(struct platform_device *pdev, struct device_node *parent) { - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_3380, + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent, + bcm7120_l2_intc_iomap_3380, "BCM3380 L2"); } diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c index e4efc08ac594..52886fbcea18 100644 --- a/drivers/irqchip/irq-brcmstb-l2.c +++ b/drivers/irqchip/irq-brcmstb-l2.c @@ -270,16 +270,18 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np, return ret; } -static int __init brcmstb_l2_edge_intc_of_init(struct device_node *np, +static int __init brcmstb_l2_edge_intc_of_init(struct platform_device *pdev, struct device_node *parent) { - return brcmstb_l2_intc_of_init(np, parent, &l2_edge_intc_init); + return brcmstb_l2_intc_of_init(pdev->dev.of_node, parent, + &l2_edge_intc_init); } -static int __init brcmstb_l2_lvl_intc_of_init(struct device_node *np, +static int __init brcmstb_l2_lvl_intc_of_init(struct platform_device *pdev, struct device_node *parent) { - return brcmstb_l2_intc_of_init(np, parent, &l2_lvl_intc_init); + return brcmstb_l2_intc_of_init(pdev->dev.of_node, parent, + &l2_lvl_intc_init); } IRQCHIP_PLATFORM_DRIVER_BEGIN(brcmstb_l2) diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c index c726a19837d2..1ab34af841f6 100644 --- a/drivers/irqchip/irq-mchp-eic.c +++ b/drivers/irqchip/irq-mchp-eic.c @@ -199,8 +199,10 @@ static const struct irq_domain_ops mchp_eic_domain_ops = { .free = irq_domain_free_irqs_common, }; -static int mchp_eic_init(struct device_node *node, struct device_node *parent) +static int mchp_eic_init(struct platform_device *pdev, + struct device_node *parent) { + struct device_node *node = pdev->dev.of_node; struct irq_domain *parent_domain = NULL; int ret, i; diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c index d90ff0b92480..1c841189defa 100644 --- a/drivers/irqchip/irq-meson-gpio.c +++ b/drivers/irqchip/irq-meson-gpio.c @@ -436,7 +436,8 @@ static const struct irq_domain_ops meson_gpio_irq_domain_ops = { .translate = meson_gpio_irq_domain_translate, }; -static int meson_gpio_irq_parse_dt(struct device_node *node, struct meson_gpio_irq_controller *ctl) +static int meson_gpio_irq_parse_dt(struct device_node *node, + struct meson_gpio_irq_controller *ctl) { const struct of_device_id *match; int ret; @@ -462,8 +463,10 @@ static int meson_gpio_irq_parse_dt(struct device_node *node, struct meson_gpio_i return 0; } -static int meson_gpio_irq_of_init(struct device_node *node, struct device_node *parent) +static int meson_gpio_irq_of_init(struct platform_device *pdev, + struct device_node *parent) { + struct device_node *node = pdev->dev.of_node; struct irq_domain *domain, *parent_domain; struct meson_gpio_irq_controller *ctl; int ret; diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index 3570f0a588c4..62e6dbc3c4d0 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -36,7 +36,7 @@ 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); + irqchip_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev); if (!irq_init_cb) return -EINVAL; @@ -55,6 +55,6 @@ int platform_irqchip_probe(struct platform_device *pdev) if (par_np && !irq_find_matching_host(par_np, DOMAIN_BUS_ANY)) return -EPROBE_DEFER; - return irq_init_cb(np, par_np); + return irq_init_cb(pdev, par_np); } EXPORT_SYMBOL_GPL(platform_irqchip_probe); diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 173e6520e06e..b66ac9dd46c3 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -359,8 +359,10 @@ static int pdc_setup_pin_mapping(struct device_node *np) return 0; } -static int qcom_pdc_init(struct device_node *node, struct device_node *parent) +static int qcom_pdc_init(struct platform_device *pdev, + struct device_node *parent) { + struct device_node *node = pdev->dev.of_node; struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain; int ret; diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h index 3a091d0710ae..da33a21c0297 100644 --- a/include/linux/irqchip.h +++ b/include/linux/irqchip.h @@ -36,13 +36,19 @@ extern of_irq_init_cb_t typecheck_irq_init_cb; #define IRQCHIP_DECLARE(name, compat, fn) \ OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn)) +typedef int (*irqchip_init_cb_t)(struct platform_device *, + struct device_node *); +extern irqchip_init_cb_t typecheck_irqchip_init_cb; +#define typecheck_irqchip_init_cb(fn) \ + (__typecheck(typecheck_irqchip_init_cb, &fn) ? fn : fn) + extern int platform_irqchip_probe(struct platform_device *pdev); #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \ static const struct of_device_id drv_name##_irqchip_match_table[] = { #define IRQCHIP_MATCH(compat, fn) { .compatible = compat, \ - .data = typecheck_irq_init_cb(fn), }, + .data = typecheck_irqchip_init_cb(fn), }, #define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \ {}, \
It makes sense to just pass device_node for callback in IRQCHIP_DECLARE case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because platform_driver probe/init usually needs device pointer for various purposes, e.g. resource allocation, service request, device prefixed message output, etc. Create a new callback type irqchip_init_cb_t which takes platform_device pointer as parameter, and update the existing IRQCHIP_PLATFORM_DRIVER users accordingly. Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Claudiu Beznea <claudiu.beznea@microchip.com> Cc: Neil Armstrong <narmstrong@baylibre.com> Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/irqchip/irq-bcm7038-l1.c | 3 ++- drivers/irqchip/irq-bcm7120-l2.c | 10 ++++++---- drivers/irqchip/irq-brcmstb-l2.c | 10 ++++++---- drivers/irqchip/irq-mchp-eic.c | 4 +++- drivers/irqchip/irq-meson-gpio.c | 7 +++++-- drivers/irqchip/irqchip.c | 4 ++-- drivers/irqchip/qcom-pdc.c | 4 +++- include/linux/irqchip.h | 8 +++++++- 8 files changed, 34 insertions(+), 16 deletions(-)