| Message ID | 20220111002314.15213-7-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Use platform_get_irq*() variants to fetch IRQ's | expand |
Hi Prabhakar, On 11/01/2022 01:23, Lad Prabhakar wrote: > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > allocation of IRQ resources in DT core code, this causes an issue > when using hierarchical interrupt domains using "interrupts" property > in the node as this bypasses the hierarchical setup and messes up the > irq chaining. > > In preparation for removal of static setup of IRQ resource from DT core > code use platform_get_irq_optional(). > > While at it, propagate error code in case devm_request_irq() fails > instead of returning -EINVAL in vpif_display.c. Please note that this patch clashes with [1], for which I just posted a PR [2]. So once [2] is merged you'll need to rebase this patch. Regards, Hans [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=7000 [2] https://patchwork.linuxtv.org/project/linux-media/patch/63d723aa-b6a3-ff42-c3e4-f1fcb979be11@xs4all.nl/ > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v1->v2 > * Used DEFINE_RES_IRQ_NAMED() macro > * Considered IRQ0 as no IRQ. > --- > drivers/media/platform/davinci/vpif.c | 18 ++++++++------ > drivers/media/platform/davinci/vpif_capture.c | 24 ++++++++++++------- > drivers/media/platform/davinci/vpif_display.c | 21 ++++++++++------ > 3 files changed, 40 insertions(+), 23 deletions(-) > > diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c > index 5a89d885d0e3..2dbb464399cc 100644 > --- a/drivers/media/platform/davinci/vpif.c > +++ b/drivers/media/platform/davinci/vpif.c > @@ -20,8 +20,10 @@ > #include <linux/err.h> > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/irq.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/spinlock.h> > @@ -425,9 +427,10 @@ EXPORT_SYMBOL(vpif_channel_getfid); > > static int vpif_probe(struct platform_device *pdev) > { > - static struct resource *res_irq; > + static struct resource res_irq; > struct platform_device *pdev_capture, *pdev_display; > struct device_node *endpoint = NULL; > + int irq; > > vpif_base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(vpif_base)) > @@ -453,19 +456,20 @@ static int vpif_probe(struct platform_device *pdev) > * For DT platforms, manually create platform_devices for > * capture/display drivers. > */ > - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > - if (!res_irq) { > - dev_warn(&pdev->dev, "Missing IRQ resource.\n"); > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > pm_runtime_put(&pdev->dev); > - return -EINVAL; > + return irq; > } > + res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); > + res_irq.flags |= irq_get_trigger_type(irq); > > pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture), > GFP_KERNEL); > if (pdev_capture) { > pdev_capture->name = "vpif_capture"; > pdev_capture->id = -1; > - pdev_capture->resource = res_irq; > + pdev_capture->resource = &res_irq; > pdev_capture->num_resources = 1; > pdev_capture->dev.dma_mask = pdev->dev.dma_mask; > pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; > @@ -480,7 +484,7 @@ static int vpif_probe(struct platform_device *pdev) > if (pdev_display) { > pdev_display->name = "vpif_display"; > pdev_display->id = -1; > - pdev_display->resource = res_irq; > + pdev_display->resource = &res_irq; > pdev_display->num_resources = 1; > pdev_display->dev.dma_mask = pdev->dev.dma_mask; > pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c > index 8fe55374c5a3..aaf8a0ec98ff 100644 > --- a/drivers/media/platform/davinci/vpif_capture.c > +++ b/drivers/media/platform/davinci/vpif_capture.c > @@ -1607,7 +1607,6 @@ static __init int vpif_probe(struct platform_device *pdev) > { > struct vpif_subdev_info *subdevdata; > struct i2c_adapter *i2c_adap; > - struct resource *res; > int subdev_count; > int res_idx = 0; > int i, err; > @@ -1632,15 +1631,22 @@ static __init int vpif_probe(struct platform_device *pdev) > goto vpif_free; > } > > - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { > - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, > - IRQF_SHARED, VPIF_DRIVER_NAME, > - (void *)(&vpif_obj.dev[res_idx]-> > - channel_id)); > - if (err) { > - err = -EINVAL; > + while (1) { > + int irq; > + > + err = platform_get_irq_optional(pdev, res_idx); > + if (err < 0 && err != -ENXIO) > + goto vpif_unregister; > + if (err > 0) > + irq = err; > + else > + break; > + > + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, > + IRQF_SHARED, VPIF_DRIVER_NAME, > + (void *)(&vpif_obj.dev[res_idx]->channel_id)); > + if (err) > goto vpif_unregister; > - } > res_idx++; > } > > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c > index 59f6b782e104..c4a0c4a4448c 100644 > --- a/drivers/media/platform/davinci/vpif_display.c > +++ b/drivers/media/platform/davinci/vpif_display.c > @@ -1221,7 +1221,6 @@ static __init int vpif_probe(struct platform_device *pdev) > { > struct vpif_subdev_info *subdevdata; > struct i2c_adapter *i2c_adap; > - struct resource *res; > int subdev_count; > int res_idx = 0; > int i, err; > @@ -1245,13 +1244,21 @@ static __init int vpif_probe(struct platform_device *pdev) > goto vpif_free; > } > > - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { > - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, > - IRQF_SHARED, VPIF_DRIVER_NAME, > - (void *)(&vpif_obj.dev[res_idx]-> > - channel_id)); > + while (1) { > + int irq; > + > + err = platform_get_irq_optional(pdev, res_idx); > + if (err < 0 && err != -ENXIO) > + goto vpif_unregister; > + if (err > 0) > + irq = err; > + else > + break; > + > + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, > + IRQF_SHARED, VPIF_DRIVER_NAME, > + (void *)(&vpif_obj.dev[res_idx]->channel_id)); > if (err) { > - err = -EINVAL; > vpif_err("VPIF IRQ request failed\n"); > goto vpif_unregister; > }
Hi Hans, On Tue, Jan 11, 2022 at 10:25 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Prabhakar, > > On 11/01/2022 01:23, Lad Prabhakar wrote: > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > > allocation of IRQ resources in DT core code, this causes an issue > > when using hierarchical interrupt domains using "interrupts" property > > in the node as this bypasses the hierarchical setup and messes up the > > irq chaining. > > > > In preparation for removal of static setup of IRQ resource from DT core > > code use platform_get_irq_optional(). > > > > While at it, propagate error code in case devm_request_irq() fails > > instead of returning -EINVAL in vpif_display.c. > > Please note that this patch clashes with [1], for which I just posted a PR [2]. > Ouch, I think I had a comment for patch#2 which needed to be addressed (I was nitpicking anyway) so I was hoping this will go in first. > So once [2] is merged you'll need to rebase this patch. > Ok, do you want me to just re-send this patch alone or the entire series? Cheers, Prabhakar > Regards, > > Hans > > [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=7000 > [2] https://patchwork.linuxtv.org/project/linux-media/patch/63d723aa-b6a3-ff42-c3e4-f1fcb979be11@xs4all.nl/ > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v1->v2 > > * Used DEFINE_RES_IRQ_NAMED() macro > > * Considered IRQ0 as no IRQ. > > --- > > drivers/media/platform/davinci/vpif.c | 18 ++++++++------ > > drivers/media/platform/davinci/vpif_capture.c | 24 ++++++++++++------- > > drivers/media/platform/davinci/vpif_display.c | 21 ++++++++++------ > > 3 files changed, 40 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c > > index 5a89d885d0e3..2dbb464399cc 100644 > > --- a/drivers/media/platform/davinci/vpif.c > > +++ b/drivers/media/platform/davinci/vpif.c > > @@ -20,8 +20,10 @@ > > #include <linux/err.h> > > #include <linux/init.h> > > #include <linux/io.h> > > +#include <linux/irq.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > +#include <linux/of.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > #include <linux/spinlock.h> > > @@ -425,9 +427,10 @@ EXPORT_SYMBOL(vpif_channel_getfid); > > > > static int vpif_probe(struct platform_device *pdev) > > { > > - static struct resource *res_irq; > > + static struct resource res_irq; > > struct platform_device *pdev_capture, *pdev_display; > > struct device_node *endpoint = NULL; > > + int irq; > > > > vpif_base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(vpif_base)) > > @@ -453,19 +456,20 @@ static int vpif_probe(struct platform_device *pdev) > > * For DT platforms, manually create platform_devices for > > * capture/display drivers. > > */ > > - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > - if (!res_irq) { > > - dev_warn(&pdev->dev, "Missing IRQ resource.\n"); > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > pm_runtime_put(&pdev->dev); > > - return -EINVAL; > > + return irq; > > } > > + res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); > > + res_irq.flags |= irq_get_trigger_type(irq); > > > > pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture), > > GFP_KERNEL); > > if (pdev_capture) { > > pdev_capture->name = "vpif_capture"; > > pdev_capture->id = -1; > > - pdev_capture->resource = res_irq; > > + pdev_capture->resource = &res_irq; > > pdev_capture->num_resources = 1; > > pdev_capture->dev.dma_mask = pdev->dev.dma_mask; > > pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; > > @@ -480,7 +484,7 @@ static int vpif_probe(struct platform_device *pdev) > > if (pdev_display) { > > pdev_display->name = "vpif_display"; > > pdev_display->id = -1; > > - pdev_display->resource = res_irq; > > + pdev_display->resource = &res_irq; > > pdev_display->num_resources = 1; > > pdev_display->dev.dma_mask = pdev->dev.dma_mask; > > pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; > > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c > > index 8fe55374c5a3..aaf8a0ec98ff 100644 > > --- a/drivers/media/platform/davinci/vpif_capture.c > > +++ b/drivers/media/platform/davinci/vpif_capture.c > > @@ -1607,7 +1607,6 @@ static __init int vpif_probe(struct platform_device *pdev) > > { > > struct vpif_subdev_info *subdevdata; > > struct i2c_adapter *i2c_adap; > > - struct resource *res; > > int subdev_count; > > int res_idx = 0; > > int i, err; > > @@ -1632,15 +1631,22 @@ static __init int vpif_probe(struct platform_device *pdev) > > goto vpif_free; > > } > > > > - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { > > - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, > > - IRQF_SHARED, VPIF_DRIVER_NAME, > > - (void *)(&vpif_obj.dev[res_idx]-> > > - channel_id)); > > - if (err) { > > - err = -EINVAL; > > + while (1) { > > + int irq; > > + > > + err = platform_get_irq_optional(pdev, res_idx); > > + if (err < 0 && err != -ENXIO) > > + goto vpif_unregister; > > + if (err > 0) > > + irq = err; > > + else > > + break; > > + > > + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, > > + IRQF_SHARED, VPIF_DRIVER_NAME, > > + (void *)(&vpif_obj.dev[res_idx]->channel_id)); > > + if (err) > > goto vpif_unregister; > > - } > > res_idx++; > > } > > > > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c > > index 59f6b782e104..c4a0c4a4448c 100644 > > --- a/drivers/media/platform/davinci/vpif_display.c > > +++ b/drivers/media/platform/davinci/vpif_display.c > > @@ -1221,7 +1221,6 @@ static __init int vpif_probe(struct platform_device *pdev) > > { > > struct vpif_subdev_info *subdevdata; > > struct i2c_adapter *i2c_adap; > > - struct resource *res; > > int subdev_count; > > int res_idx = 0; > > int i, err; > > @@ -1245,13 +1244,21 @@ static __init int vpif_probe(struct platform_device *pdev) > > goto vpif_free; > > } > > > > - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { > > - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, > > - IRQF_SHARED, VPIF_DRIVER_NAME, > > - (void *)(&vpif_obj.dev[res_idx]-> > > - channel_id)); > > + while (1) { > > + int irq; > > + > > + err = platform_get_irq_optional(pdev, res_idx); > > + if (err < 0 && err != -ENXIO) > > + goto vpif_unregister; > > + if (err > 0) > > + irq = err; > > + else > > + break; > > + > > + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, > > + IRQF_SHARED, VPIF_DRIVER_NAME, > > + (void *)(&vpif_obj.dev[res_idx]->channel_id)); > > if (err) { > > - err = -EINVAL; > > vpif_err("VPIF IRQ request failed\n"); > > goto vpif_unregister; > > } >
On 11/01/2022 11:43, Lad, Prabhakar wrote: > Hi Hans, > > On Tue, Jan 11, 2022 at 10:25 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Hi Prabhakar, >> >> On 11/01/2022 01:23, Lad Prabhakar wrote: >>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static >>> allocation of IRQ resources in DT core code, this causes an issue >>> when using hierarchical interrupt domains using "interrupts" property >>> in the node as this bypasses the hierarchical setup and messes up the >>> irq chaining. >>> >>> In preparation for removal of static setup of IRQ resource from DT core >>> code use platform_get_irq_optional(). >>> >>> While at it, propagate error code in case devm_request_irq() fails >>> instead of returning -EINVAL in vpif_display.c. >> >> Please note that this patch clashes with [1], for which I just posted a PR [2]. >> > Ouch, I think I had a comment for patch#2 which needed to be addressed > (I was nitpicking anyway) so I was hoping this will go in first. Patch 2 was fine since that change makes sense when looking at patch 3. > >> So once [2] is merged you'll need to rebase this patch. >> > Ok, do you want me to just re-send this patch alone or the entire series? Either works. Regards, Hans > > Cheers, > Prabhakar > >> Regards, >> >> Hans >> >> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=7000 >> [2] https://patchwork.linuxtv.org/project/linux-media/patch/63d723aa-b6a3-ff42-c3e4-f1fcb979be11@xs4all.nl/ >> >>> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> --- >>> v1->v2 >>> * Used DEFINE_RES_IRQ_NAMED() macro >>> * Considered IRQ0 as no IRQ. >>> --- >>> drivers/media/platform/davinci/vpif.c | 18 ++++++++------ >>> drivers/media/platform/davinci/vpif_capture.c | 24 ++++++++++++------- >>> drivers/media/platform/davinci/vpif_display.c | 21 ++++++++++------ >>> 3 files changed, 40 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c >>> index 5a89d885d0e3..2dbb464399cc 100644 >>> --- a/drivers/media/platform/davinci/vpif.c >>> +++ b/drivers/media/platform/davinci/vpif.c >>> @@ -20,8 +20,10 @@ >>> #include <linux/err.h> >>> #include <linux/init.h> >>> #include <linux/io.h> >>> +#include <linux/irq.h> >>> #include <linux/kernel.h> >>> #include <linux/module.h> >>> +#include <linux/of.h> >>> #include <linux/platform_device.h> >>> #include <linux/pm_runtime.h> >>> #include <linux/spinlock.h> >>> @@ -425,9 +427,10 @@ EXPORT_SYMBOL(vpif_channel_getfid); >>> >>> static int vpif_probe(struct platform_device *pdev) >>> { >>> - static struct resource *res_irq; >>> + static struct resource res_irq; >>> struct platform_device *pdev_capture, *pdev_display; >>> struct device_node *endpoint = NULL; >>> + int irq; >>> >>> vpif_base = devm_platform_ioremap_resource(pdev, 0); >>> if (IS_ERR(vpif_base)) >>> @@ -453,19 +456,20 @@ static int vpif_probe(struct platform_device *pdev) >>> * For DT platforms, manually create platform_devices for >>> * capture/display drivers. >>> */ >>> - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>> - if (!res_irq) { >>> - dev_warn(&pdev->dev, "Missing IRQ resource.\n"); >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) { >>> pm_runtime_put(&pdev->dev); >>> - return -EINVAL; >>> + return irq; >>> } >>> + res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); >>> + res_irq.flags |= irq_get_trigger_type(irq); >>> >>> pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture), >>> GFP_KERNEL); >>> if (pdev_capture) { >>> pdev_capture->name = "vpif_capture"; >>> pdev_capture->id = -1; >>> - pdev_capture->resource = res_irq; >>> + pdev_capture->resource = &res_irq; >>> pdev_capture->num_resources = 1; >>> pdev_capture->dev.dma_mask = pdev->dev.dma_mask; >>> pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; >>> @@ -480,7 +484,7 @@ static int vpif_probe(struct platform_device *pdev) >>> if (pdev_display) { >>> pdev_display->name = "vpif_display"; >>> pdev_display->id = -1; >>> - pdev_display->resource = res_irq; >>> + pdev_display->resource = &res_irq; >>> pdev_display->num_resources = 1; >>> pdev_display->dev.dma_mask = pdev->dev.dma_mask; >>> pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; >>> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c >>> index 8fe55374c5a3..aaf8a0ec98ff 100644 >>> --- a/drivers/media/platform/davinci/vpif_capture.c >>> +++ b/drivers/media/platform/davinci/vpif_capture.c >>> @@ -1607,7 +1607,6 @@ static __init int vpif_probe(struct platform_device *pdev) >>> { >>> struct vpif_subdev_info *subdevdata; >>> struct i2c_adapter *i2c_adap; >>> - struct resource *res; >>> int subdev_count; >>> int res_idx = 0; >>> int i, err; >>> @@ -1632,15 +1631,22 @@ static __init int vpif_probe(struct platform_device *pdev) >>> goto vpif_free; >>> } >>> >>> - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { >>> - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, >>> - IRQF_SHARED, VPIF_DRIVER_NAME, >>> - (void *)(&vpif_obj.dev[res_idx]-> >>> - channel_id)); >>> - if (err) { >>> - err = -EINVAL; >>> + while (1) { >>> + int irq; >>> + >>> + err = platform_get_irq_optional(pdev, res_idx); >>> + if (err < 0 && err != -ENXIO) >>> + goto vpif_unregister; >>> + if (err > 0) >>> + irq = err; >>> + else >>> + break; >>> + >>> + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, >>> + IRQF_SHARED, VPIF_DRIVER_NAME, >>> + (void *)(&vpif_obj.dev[res_idx]->channel_id)); >>> + if (err) >>> goto vpif_unregister; >>> - } >>> res_idx++; >>> } >>> >>> diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c >>> index 59f6b782e104..c4a0c4a4448c 100644 >>> --- a/drivers/media/platform/davinci/vpif_display.c >>> +++ b/drivers/media/platform/davinci/vpif_display.c >>> @@ -1221,7 +1221,6 @@ static __init int vpif_probe(struct platform_device *pdev) >>> { >>> struct vpif_subdev_info *subdevdata; >>> struct i2c_adapter *i2c_adap; >>> - struct resource *res; >>> int subdev_count; >>> int res_idx = 0; >>> int i, err; >>> @@ -1245,13 +1244,21 @@ static __init int vpif_probe(struct platform_device *pdev) >>> goto vpif_free; >>> } >>> >>> - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { >>> - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, >>> - IRQF_SHARED, VPIF_DRIVER_NAME, >>> - (void *)(&vpif_obj.dev[res_idx]-> >>> - channel_id)); >>> + while (1) { >>> + int irq; >>> + >>> + err = platform_get_irq_optional(pdev, res_idx); >>> + if (err < 0 && err != -ENXIO) >>> + goto vpif_unregister; >>> + if (err > 0) >>> + irq = err; >>> + else >>> + break; >>> + >>> + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, >>> + IRQF_SHARED, VPIF_DRIVER_NAME, >>> + (void *)(&vpif_obj.dev[res_idx]->channel_id)); >>> if (err) { >>> - err = -EINVAL; >>> vpif_err("VPIF IRQ request failed\n"); >>> goto vpif_unregister; >>> } >>
Hi Hans, On Tue, Jan 11, 2022 at 10:45 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 11/01/2022 11:43, Lad, Prabhakar wrote: > > Hi Hans, > > > > On Tue, Jan 11, 2022 at 10:25 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> Hi Prabhakar, > >> > >> On 11/01/2022 01:23, Lad Prabhakar wrote: > >>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > >>> allocation of IRQ resources in DT core code, this causes an issue > >>> when using hierarchical interrupt domains using "interrupts" property > >>> in the node as this bypasses the hierarchical setup and messes up the > >>> irq chaining. > >>> > >>> In preparation for removal of static setup of IRQ resource from DT core > >>> code use platform_get_irq_optional(). > >>> > >>> While at it, propagate error code in case devm_request_irq() fails > >>> instead of returning -EINVAL in vpif_display.c. > >> > >> Please note that this patch clashes with [1], for which I just posted a PR [2]. > >> > > Ouch, I think I had a comment for patch#2 which needed to be addressed > > (I was nitpicking anyway) so I was hoping this will go in first. > > Patch 2 was fine since that change makes sense when looking at patch 3. > Agreed. > > > >> So once [2] is merged you'll need to rebase this patch. > >> > > Ok, do you want me to just re-send this patch alone or the entire series? > > Either works. > Will just send this alone patch as v3. Cheers, Prabhakar
diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c index 5a89d885d0e3..2dbb464399cc 100644 --- a/drivers/media/platform/davinci/vpif.c +++ b/drivers/media/platform/davinci/vpif.c @@ -20,8 +20,10 @@ #include <linux/err.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/irq.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/spinlock.h> @@ -425,9 +427,10 @@ EXPORT_SYMBOL(vpif_channel_getfid); static int vpif_probe(struct platform_device *pdev) { - static struct resource *res_irq; + static struct resource res_irq; struct platform_device *pdev_capture, *pdev_display; struct device_node *endpoint = NULL; + int irq; vpif_base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(vpif_base)) @@ -453,19 +456,20 @@ static int vpif_probe(struct platform_device *pdev) * For DT platforms, manually create platform_devices for * capture/display drivers. */ - res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); - if (!res_irq) { - dev_warn(&pdev->dev, "Missing IRQ resource.\n"); + irq = platform_get_irq(pdev, 0); + if (irq < 0) { pm_runtime_put(&pdev->dev); - return -EINVAL; + return irq; } + res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); + res_irq.flags |= irq_get_trigger_type(irq); pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture), GFP_KERNEL); if (pdev_capture) { pdev_capture->name = "vpif_capture"; pdev_capture->id = -1; - pdev_capture->resource = res_irq; + pdev_capture->resource = &res_irq; pdev_capture->num_resources = 1; pdev_capture->dev.dma_mask = pdev->dev.dma_mask; pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; @@ -480,7 +484,7 @@ static int vpif_probe(struct platform_device *pdev) if (pdev_display) { pdev_display->name = "vpif_display"; pdev_display->id = -1; - pdev_display->resource = res_irq; + pdev_display->resource = &res_irq; pdev_display->num_resources = 1; pdev_display->dev.dma_mask = pdev->dev.dma_mask; pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask; diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c index 8fe55374c5a3..aaf8a0ec98ff 100644 --- a/drivers/media/platform/davinci/vpif_capture.c +++ b/drivers/media/platform/davinci/vpif_capture.c @@ -1607,7 +1607,6 @@ static __init int vpif_probe(struct platform_device *pdev) { struct vpif_subdev_info *subdevdata; struct i2c_adapter *i2c_adap; - struct resource *res; int subdev_count; int res_idx = 0; int i, err; @@ -1632,15 +1631,22 @@ static __init int vpif_probe(struct platform_device *pdev) goto vpif_free; } - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, - IRQF_SHARED, VPIF_DRIVER_NAME, - (void *)(&vpif_obj.dev[res_idx]-> - channel_id)); - if (err) { - err = -EINVAL; + while (1) { + int irq; + + err = platform_get_irq_optional(pdev, res_idx); + if (err < 0 && err != -ENXIO) + goto vpif_unregister; + if (err > 0) + irq = err; + else + break; + + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, + IRQF_SHARED, VPIF_DRIVER_NAME, + (void *)(&vpif_obj.dev[res_idx]->channel_id)); + if (err) goto vpif_unregister; - } res_idx++; } diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c index 59f6b782e104..c4a0c4a4448c 100644 --- a/drivers/media/platform/davinci/vpif_display.c +++ b/drivers/media/platform/davinci/vpif_display.c @@ -1221,7 +1221,6 @@ static __init int vpif_probe(struct platform_device *pdev) { struct vpif_subdev_info *subdevdata; struct i2c_adapter *i2c_adap; - struct resource *res; int subdev_count; int res_idx = 0; int i, err; @@ -1245,13 +1244,21 @@ static __init int vpif_probe(struct platform_device *pdev) goto vpif_free; } - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) { - err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr, - IRQF_SHARED, VPIF_DRIVER_NAME, - (void *)(&vpif_obj.dev[res_idx]-> - channel_id)); + while (1) { + int irq; + + err = platform_get_irq_optional(pdev, res_idx); + if (err < 0 && err != -ENXIO) + goto vpif_unregister; + if (err > 0) + irq = err; + else + break; + + err = devm_request_irq(&pdev->dev, irq, vpif_channel_isr, + IRQF_SHARED, VPIF_DRIVER_NAME, + (void *)(&vpif_obj.dev[res_idx]->channel_id)); if (err) { - err = -EINVAL; vpif_err("VPIF IRQ request failed\n"); goto vpif_unregister; }
platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static allocation of IRQ resources in DT core code, this causes an issue when using hierarchical interrupt domains using "interrupts" property in the node as this bypasses the hierarchical setup and messes up the irq chaining. In preparation for removal of static setup of IRQ resource from DT core code use platform_get_irq_optional(). While at it, propagate error code in case devm_request_irq() fails instead of returning -EINVAL in vpif_display.c. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- v1->v2 * Used DEFINE_RES_IRQ_NAMED() macro * Considered IRQ0 as no IRQ. --- drivers/media/platform/davinci/vpif.c | 18 ++++++++------ drivers/media/platform/davinci/vpif_capture.c | 24 ++++++++++++------- drivers/media/platform/davinci/vpif_display.c | 21 ++++++++++------ 3 files changed, 40 insertions(+), 23 deletions(-)