diff mbox series

[06/13] media: davinci: vpif: Use platform_get_irq_optional() to get the interrupt

Message ID 20211223173015.22251-7-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series media: Use platform_get_irq*() variants to fetch IRQ's | expand

Commit Message

Prabhakar Mahadev Lad Dec. 23, 2021, 5:30 p.m. UTC
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().

Also this patch propagates error code in case devm_request_irq()
fails instead of returing -EINVAL.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/davinci/vpif.c         | 17 ++++++++++++++---
 drivers/media/platform/davinci/vpif_capture.c | 16 +++++++---------
 drivers/media/platform/davinci/vpif_display.c | 13 ++++++-------
 3 files changed, 27 insertions(+), 19 deletions(-)

Comments

Andy Shevchenko Dec. 25, 2021, 5:32 p.m. UTC | #1
On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> 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().
>
> Also this patch propagates error code in case devm_request_irq()
> fails instead of returing -EINVAL.

returning

...

> +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> +       res_irq->start = irq;
> +       res_irq->end = irq;
> +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;

If you convert DEFINE_RES_NAMED() to return a compound literal, then
you may use it here like

res_irq = DEFINE_RES_NAMED(...);

or even do like this

if (dev_of_node(...))
  res_irq = DEFINE_RES_IRQ_NAMED(...)
else
  res_irq = DEFINE_RES_IRQ(...);
res_irq->flags |= irq_get_trigger_type(irq);

I'm not sure why you can't simply use the NAMED variant in both cases
(yes, I see that of_node_full_name() will return something, not NULL).

...

> +       while ((err = platform_get_irq_optional(pdev, res_idx)) != -ENXIO) {
> +               if (err < 0)
> +                       goto vpif_unregister;

Needs a better error checking, i.e. consider 0 as no-IRQ (equivalent
to -ENXIO (note, OF code never returns 0 as valid vIRQ).

>                 res_idx++;
>         }

...

> +       while ((err = platform_get_irq_optional(pdev, res_idx)) != -ENXIO) {
> +               if (err < 0)
> +                       goto vpif_unregister;

Ditto.
Andy Shevchenko Jan. 5, 2022, 9:42 a.m. UTC | #2
On Tue, Jan 4, 2022 at 7:23 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Sat, Dec 25, 2021 at 5:32 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

...

> > > +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > > +       res_irq->start = irq;
> > > +       res_irq->end = irq;
> > > +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
> >
> > If you convert DEFINE_RES_NAMED() to return a compound literal, then
> > you may use it here like
> >
> > res_irq = DEFINE_RES_NAMED(...);
> >
> > or even do like this
> >
> > if (dev_of_node(...))
> >   res_irq = DEFINE_RES_IRQ_NAMED(...)
> > else
> >   res_irq = DEFINE_RES_IRQ(...);
> > res_irq->flags |= irq_get_trigger_type(irq);
> >
> There are quite a few users of DEFINE_RES_IRQ_NAMED()/DEFINE_RES_IRQ()
> changing this macos just for this single user tree wide doesn't make
> sense. Let me know if you think otherwise.

Converting them to produce compound literal is straightforward and
does not require changes in the users. But on the other hand it allows
you to use it and convert existing users to use that form directly.
You may conduct research on how macros in the property.h were morphing
towards that.

> > I'm not sure why you can't simply use the NAMED variant in both cases
> > (yes, I see that of_node_full_name() will return something, not NULL).
Lad, Prabhakar Jan. 5, 2022, 5:41 p.m. UTC | #3
Hi Andy,

On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 4, 2022 at 7:23 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Sat, Dec 25, 2021 at 5:32 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> ...
>
> > > > +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > > > +       res_irq->start = irq;
> > > > +       res_irq->end = irq;
> > > > +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
> > >
> > > If you convert DEFINE_RES_NAMED() to return a compound literal, then
> > > you may use it here like
> > >
> > > res_irq = DEFINE_RES_NAMED(...);
> > >
> > > or even do like this
> > >
> > > if (dev_of_node(...))
> > >   res_irq = DEFINE_RES_IRQ_NAMED(...)
> > > else
> > >   res_irq = DEFINE_RES_IRQ(...);
> > > res_irq->flags |= irq_get_trigger_type(irq);
> > >
> > There are quite a few users of DEFINE_RES_IRQ_NAMED()/DEFINE_RES_IRQ()
> > changing this macos just for this single user tree wide doesn't make
> > sense. Let me know if you think otherwise.
>
> Converting them to produce compound literal is straightforward and
> does not require changes in the users. But on the other hand it allows
> you to use it and convert existing users to use that form directly.
> You may conduct research on how macros in the property.h were morphing
> towards that.
>
Thank you for the pointer. I did the below change for this.

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..da1208e8f164 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -153,7 +153,7 @@ enum {

 /* helpers to define resources */
 #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
-       {                                                               \
+       (struct resource) {                                     \
                .start = (_start),                                      \
                .end = (_start) + (_size) - 1,                          \
                .name = (_name),                                        \

But there are some instances which need to be touched, for example
vexpress-sysreg.c [1]. Are you OK with files to be changed?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/vexpress-sysreg.c?h=v5.16-rc8#n65

Cheers,
Prabhakar
Andy Shevchenko Jan. 6, 2022, 1:43 p.m. UTC | #4
On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jan 4, 2022 at 7:23 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Sat, Dec 25, 2021 at 5:32 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > ...
> >
> > > > > +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > > > > +       res_irq->start = irq;
> > > > > +       res_irq->end = irq;
> > > > > +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
> > > >
> > > > If you convert DEFINE_RES_NAMED() to return a compound literal, then
> > > > you may use it here like
> > > >
> > > > res_irq = DEFINE_RES_NAMED(...);
> > > >
> > > > or even do like this
> > > >
> > > > if (dev_of_node(...))
> > > >   res_irq = DEFINE_RES_IRQ_NAMED(...)
> > > > else
> > > >   res_irq = DEFINE_RES_IRQ(...);
> > > > res_irq->flags |= irq_get_trigger_type(irq);
> > > >
> > > There are quite a few users of DEFINE_RES_IRQ_NAMED()/DEFINE_RES_IRQ()
> > > changing this macos just for this single user tree wide doesn't make
> > > sense. Let me know if you think otherwise.
> >
> > Converting them to produce compound literal is straightforward and
> > does not require changes in the users. But on the other hand it allows
> > you to use it and convert existing users to use that form directly.
> > You may conduct research on how macros in the property.h were morphing
> > towards that.
> >
> Thank you for the pointer. I did the below change for this.
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 8359c50f9988..da1208e8f164 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -153,7 +153,7 @@ enum {
>
>  /* helpers to define resources */
>  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> -       {                                                               \
> +       (struct resource) {                                     \

Yep, that's it.

>                 .start = (_start),                                      \
>                 .end = (_start) + (_size) - 1,                          \
>                 .name = (_name),                                        \
>
> But there are some instances which need to be touched, for example
> vexpress-sysreg.c [1]. Are you OK with files to be changed?

Nice! That's exactly my point and you can sell it to the community
because there are already users of it like this.

Yes, I'm fine, but it seems it needs to be done treewide in one patch.
Btw, how many of those already in use?


> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/vexpress-sysreg.c?h=v5.16-rc8#n65
Andy Shevchenko Jan. 6, 2022, 2:14 p.m. UTC | #5
On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jan 4, 2022 at 7:23 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Sat, Dec 25, 2021 at 5:32 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

...

> > > > > > +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > > > > > +       res_irq->start = irq;
> > > > > > +       res_irq->end = irq;
> > > > > > +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
> > > > >
> > > > > If you convert DEFINE_RES_NAMED() to return a compound literal, then
> > > > > you may use it here like
> > > > >
> > > > > res_irq = DEFINE_RES_NAMED(...);
> > > > >
> > > > > or even do like this
> > > > >
> > > > > if (dev_of_node(...))
> > > > >   res_irq = DEFINE_RES_IRQ_NAMED(...)
> > > > > else
> > > > >   res_irq = DEFINE_RES_IRQ(...);
> > > > > res_irq->flags |= irq_get_trigger_type(irq);
> > > > >
> > > > There are quite a few users of DEFINE_RES_IRQ_NAMED()/DEFINE_RES_IRQ()
> > > > changing this macos just for this single user tree wide doesn't make
> > > > sense. Let me know if you think otherwise.
> > >
> > > Converting them to produce compound literal is straightforward and
> > > does not require changes in the users. But on the other hand it allows
> > > you to use it and convert existing users to use that form directly.
> > > You may conduct research on how macros in the property.h were morphing
> > > towards that.
> > >
> > Thank you for the pointer. I did the below change for this.
> >
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index 8359c50f9988..da1208e8f164 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -153,7 +153,7 @@ enum {
> >
> >  /* helpers to define resources */
> >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > -       {                                                               \
> > +       (struct resource) {                                     \
>
> Yep, that's it.
>
> >                 .start = (_start),                                      \
> >                 .end = (_start) + (_size) - 1,                          \
> >                 .name = (_name),                                        \
> >
> > But there are some instances which need to be touched, for example
> > vexpress-sysreg.c [1]. Are you OK with files to be changed?
>
> Nice! That's exactly my point and you can sell it to the community
> because there are already users of it like this.
>
> Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> Btw, how many of those already in use?

Actually you don't need to change that. It's an array of resources and
everything should be kept as is there.

> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/vexpress-sysreg.c?h=v5.16-rc8#n65
Lad, Prabhakar Jan. 6, 2022, 3:27 p.m. UTC | #6
Hi Andy,

On Thu, Jan 6, 2022 at 2:15 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jan 4, 2022 at 7:23 PM Lad, Prabhakar
> > > > <prabhakar.csengg@gmail.com> wrote:
> > > > > On Sat, Dec 25, 2021 at 5:32 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Sat, Dec 25, 2021 at 3:04 AM Lad Prabhakar
> > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> ...
>
> > > > > > > +       res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > > > > > > +       res_irq->start = irq;
> > > > > > > +       res_irq->end = irq;
> > > > > > > +       res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
> > > > > >
> > > > > > If you convert DEFINE_RES_NAMED() to return a compound literal, then
> > > > > > you may use it here like
> > > > > >
> > > > > > res_irq = DEFINE_RES_NAMED(...);
> > > > > >
> > > > > > or even do like this
> > > > > >
> > > > > > if (dev_of_node(...))
> > > > > >   res_irq = DEFINE_RES_IRQ_NAMED(...)
> > > > > > else
> > > > > >   res_irq = DEFINE_RES_IRQ(...);
> > > > > > res_irq->flags |= irq_get_trigger_type(irq);
> > > > > >
> > > > > There are quite a few users of DEFINE_RES_IRQ_NAMED()/DEFINE_RES_IRQ()
> > > > > changing this macos just for this single user tree wide doesn't make
> > > > > sense. Let me know if you think otherwise.
> > > >
> > > > Converting them to produce compound literal is straightforward and
> > > > does not require changes in the users. But on the other hand it allows
> > > > you to use it and convert existing users to use that form directly.
> > > > You may conduct research on how macros in the property.h were morphing
> > > > towards that.
> > > >
> > > Thank you for the pointer. I did the below change for this.
> > >
> > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > > index 8359c50f9988..da1208e8f164 100644
> > > --- a/include/linux/ioport.h
> > > +++ b/include/linux/ioport.h
> > > @@ -153,7 +153,7 @@ enum {
> > >
> > >  /* helpers to define resources */
> > >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > > -       {                                                               \
> > > +       (struct resource) {                                     \
> >
> > Yep, that's it.
> >
> > >                 .start = (_start),                                      \
> > >                 .end = (_start) + (_size) - 1,                          \
> > >                 .name = (_name),                                        \
> > >
> > > But there are some instances which need to be touched, for example
> > > vexpress-sysreg.c [1]. Are you OK with files to be changed?
> >
> > Nice! That's exactly my point and you can sell it to the community
> > because there are already users of it like this.
> >
> > Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> > Btw, how many of those already in use?
>
> Actually you don't need to change that. It's an array of resources and
> everything should be kept as is there.
>
I do get below build failures, with the above literal change for
vexpress-sysreg.c.

drivers/mfd/vexpress-sysreg.c: At top level:
drivers/mfd/vexpress-sysreg.c:64:37: error: initialiser element is not constant
   64 |   .resources = (struct resource []) {
      |                                     ^
drivers/mfd/vexpress-sysreg.c:64:37: note: (near initialisation for
‘vexpress_sysreg_cells[0]’)
drivers/mfd/vexpress-sysreg.c:73:37: error: initialiser element is not constant
   73 |   .resources = (struct resource []) {
      |                                     ^
drivers/mfd/vexpress-sysreg.c:73:37: note: (near initialisation for
‘vexpress_sysreg_cells[1]’)
drivers/mfd/vexpress-sysreg.c:82:37: error: initialiser element is not constant
   82 |   .resources = (struct resource []) {
      |                                     ^
drivers/mfd/vexpress-sysreg.c:82:37: note: (near initialisation for
‘vexpress_sysreg_cells[2]’)
drivers/mfd/vexpress-sysreg.c:90:37: error: initialiser element is not constant
   90 |   .resources = (struct resource []) {
      |                                     ^
drivers/mfd/vexpress-sysreg.c:90:37: note: (near initialisation for
‘vexpress_sysreg_cells[3]’)
drivers/mfd/vexpress-sysreg.c:93:2: warning: missing initialiser for
field ‘ignore_resource_conflicts’ of ‘struct mfd_cell’
[-Wmissing-field-initializers]
   93 |  }

Cheers,
Prabhakar
Andy Shevchenko Jan. 6, 2022, 4:01 p.m. UTC | #7
On Thu, Jan 6, 2022 at 5:27 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Jan 6, 2022 at 2:15 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko

...

> > > >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > > > -       {                                                               \
> > > > +       (struct resource) {                                     \
> > >
> > > Yep, that's it.
> > >
> > > >                 .start = (_start),                                      \
> > > >                 .end = (_start) + (_size) - 1,                          \
> > > >                 .name = (_name),                                        \
> > > >
> > > > But there are some instances which need to be touched, for example
> > > > vexpress-sysreg.c [1]. Are you OK with files to be changed?
> > >
> > > Nice! That's exactly my point and you can sell it to the community
> > > because there are already users of it like this.
> > >
> > > Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> > > Btw, how many of those already in use?
> >
> > Actually you don't need to change that. It's an array of resources and
> > everything should be kept as is there.
> >
> I do get below build failures, with the above literal change for
> vexpress-sysreg.c.
>
> drivers/mfd/vexpress-sysreg.c: At top level:
> drivers/mfd/vexpress-sysreg.c:64:37: error: initialiser element is not constant
>    64 |   .resources = (struct resource []) {
>       |                                     ^
> drivers/mfd/vexpress-sysreg.c:64:37: note: (near initialisation for
> ‘vexpress_sysreg_cells[0]’)
> drivers/mfd/vexpress-sysreg.c:73:37: error: initialiser element is not constant
>    73 |   .resources = (struct resource []) {
>       |                                     ^
> drivers/mfd/vexpress-sysreg.c:73:37: note: (near initialisation for
> ‘vexpress_sysreg_cells[1]’)
> drivers/mfd/vexpress-sysreg.c:82:37: error: initialiser element is not constant
>    82 |   .resources = (struct resource []) {
>       |                                     ^
> drivers/mfd/vexpress-sysreg.c:82:37: note: (near initialisation for
> ‘vexpress_sysreg_cells[2]’)
> drivers/mfd/vexpress-sysreg.c:90:37: error: initialiser element is not constant
>    90 |   .resources = (struct resource []) {
>       |                                     ^
> drivers/mfd/vexpress-sysreg.c:90:37: note: (near initialisation for
> ‘vexpress_sysreg_cells[3]’)
> drivers/mfd/vexpress-sysreg.c:93:2: warning: missing initialiser for
> field ‘ignore_resource_conflicts’ of ‘struct mfd_cell’
> [-Wmissing-field-initializers]
>    93 |  }

Hmm... Interesting, so I suppose the fix is to drop (struct resource
[]) parts from the driver?
Lad, Prabhakar Jan. 6, 2022, 4:10 p.m. UTC | #8
Hi Andy,

On Thu, Jan 6, 2022 at 4:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jan 6, 2022 at 5:27 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Thu, Jan 6, 2022 at 2:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> > > > <prabhakar.csengg@gmail.com> wrote:
> > > > > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
>
> ...
>
> > > > >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > > > > -       {                                                               \
> > > > > +       (struct resource) {                                     \
> > > >
> > > > Yep, that's it.
> > > >
> > > > >                 .start = (_start),                                      \
> > > > >                 .end = (_start) + (_size) - 1,                          \
> > > > >                 .name = (_name),                                        \
> > > > >
> > > > > But there are some instances which need to be touched, for example
> > > > > vexpress-sysreg.c [1]. Are you OK with files to be changed?
> > > >
> > > > Nice! That's exactly my point and you can sell it to the community
> > > > because there are already users of it like this.
> > > >
> > > > Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> > > > Btw, how many of those already in use?
> > >
> > > Actually you don't need to change that. It's an array of resources and
> > > everything should be kept as is there.
> > >
> > I do get below build failures, with the above literal change for
> > vexpress-sysreg.c.
> >
> > drivers/mfd/vexpress-sysreg.c: At top level:
> > drivers/mfd/vexpress-sysreg.c:64:37: error: initialiser element is not constant
> >    64 |   .resources = (struct resource []) {
> >       |                                     ^
> > drivers/mfd/vexpress-sysreg.c:64:37: note: (near initialisation for
> > ‘vexpress_sysreg_cells[0]’)
> > drivers/mfd/vexpress-sysreg.c:73:37: error: initialiser element is not constant
> >    73 |   .resources = (struct resource []) {
> >       |                                     ^
> > drivers/mfd/vexpress-sysreg.c:73:37: note: (near initialisation for
> > ‘vexpress_sysreg_cells[1]’)
> > drivers/mfd/vexpress-sysreg.c:82:37: error: initialiser element is not constant
> >    82 |   .resources = (struct resource []) {
> >       |                                     ^
> > drivers/mfd/vexpress-sysreg.c:82:37: note: (near initialisation for
> > ‘vexpress_sysreg_cells[2]’)
> > drivers/mfd/vexpress-sysreg.c:90:37: error: initialiser element is not constant
> >    90 |   .resources = (struct resource []) {
> >       |                                     ^
> > drivers/mfd/vexpress-sysreg.c:90:37: note: (near initialisation for
> > ‘vexpress_sysreg_cells[3]’)
> > drivers/mfd/vexpress-sysreg.c:93:2: warning: missing initialiser for
> > field ‘ignore_resource_conflicts’ of ‘struct mfd_cell’
> > [-Wmissing-field-initializers]
> >    93 |  }
>
> Hmm... Interesting, so I suppose the fix is to drop (struct resource
> []) parts from the driver?
>
A bit more than that like something below:

diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index aaf24af287dd..eab82619ec31 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -61,35 +61,27 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
  .name = "basic-mmio-gpio",
  .of_compatible = "arm,vexpress-sysreg,sys_led",
  .num_resources = 1,
- .resources = (struct resource []) {
- DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),
- },
+ .resources = &DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),
  .platform_data = &vexpress_sysreg_sys_led_pdata,
  .pdata_size = sizeof(vexpress_sysreg_sys_led_pdata),
  }, {
  .name = "basic-mmio-gpio",
  .of_compatible = "arm,vexpress-sysreg,sys_mci",
  .num_resources = 1,
- .resources = (struct resource []) {
- DEFINE_RES_MEM_NAMED(SYS_MCI, 0x4, "dat"),
- },
+ .resources = &DEFINE_RES_MEM_NAMED(SYS_MCI, 0x4, "dat"),
  .platform_data = &vexpress_sysreg_sys_mci_pdata,
  .pdata_size = sizeof(vexpress_sysreg_sys_mci_pdata),
  }, {
  .name = "basic-mmio-gpio",
  .of_compatible = "arm,vexpress-sysreg,sys_flash",
  .num_resources = 1,
- .resources = (struct resource []) {
- DEFINE_RES_MEM_NAMED(SYS_FLASH, 0x4, "dat"),
- },
+ .resources = &DEFINE_RES_MEM_NAMED(SYS_FLASH, 0x4, "dat"),
  .platform_data = &vexpress_sysreg_sys_flash_pdata,
  .pdata_size = sizeof(vexpress_sysreg_sys_flash_pdata),
  }, {
  .name = "vexpress-syscfg",
  .num_resources = 1,
- .resources = (struct resource []) {
- DEFINE_RES_MEM(SYS_MISC, 0x4c),
- },
+ .resources = &DEFINE_RES_MEM(SYS_MISC, 0x4c),
  }
 };

Cheers,
Prabhakar
Andy Shevchenko Jan. 6, 2022, 4:28 p.m. UTC | #9
On Thu, Jan 6, 2022 at 6:11 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Jan 6, 2022 at 4:02 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jan 6, 2022 at 5:27 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Thu, Jan 6, 2022 at 2:15 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> > > > > <prabhakar.csengg@gmail.com> wrote:
> > > > > > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
> >
> > ...
> >
> > > > > >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > > > > > -       {                                                               \
> > > > > > +       (struct resource) {                                     \
> > > > >
> > > > > Yep, that's it.
> > > > >
> > > > > >                 .start = (_start),                                      \
> > > > > >                 .end = (_start) + (_size) - 1,                          \
> > > > > >                 .name = (_name),                                        \
> > > > > >
> > > > > > But there are some instances which need to be touched, for example
> > > > > > vexpress-sysreg.c [1]. Are you OK with files to be changed?
> > > > >
> > > > > Nice! That's exactly my point and you can sell it to the community
> > > > > because there are already users of it like this.
> > > > >
> > > > > Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> > > > > Btw, how many of those already in use?
> > > >
> > > > Actually you don't need to change that. It's an array of resources and
> > > > everything should be kept as is there.
> > > >
> > > I do get below build failures, with the above literal change for
> > > vexpress-sysreg.c.
> > >
> > > drivers/mfd/vexpress-sysreg.c: At top level:
> > > drivers/mfd/vexpress-sysreg.c:64:37: error: initialiser element is not constant
> > >    64 |   .resources = (struct resource []) {
> > >       |                                     ^
> > > drivers/mfd/vexpress-sysreg.c:64:37: note: (near initialisation for
> > > ‘vexpress_sysreg_cells[0]’)

> > Hmm... Interesting, so I suppose the fix is to drop (struct resource
> > []) parts from the driver?
> >
> A bit more than that like something below:

> - .resources = (struct resource []) {
> - DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),
> - },
> + .resources = &DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),

This is not an equivalent change.
The warning is about const qualifier. Can it rather be  const struct
resource [] ?
Lad, Prabhakar Jan. 6, 2022, 4:46 p.m. UTC | #10
Hi Andy,

On Thu, Jan 6, 2022 at 4:28 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jan 6, 2022 at 6:11 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Thu, Jan 6, 2022 at 4:02 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jan 6, 2022 at 5:27 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Thu, Jan 6, 2022 at 2:15 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Jan 6, 2022 at 3:43 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Wed, Jan 5, 2022 at 7:41 PM Lad, Prabhakar
> > > > > > <prabhakar.csengg@gmail.com> wrote:
> > > > > > > On Wed, Jan 5, 2022 at 9:43 AM Andy Shevchenko
> > >
> > > ...
> > >
> > > > > > >  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)                 \
> > > > > > > -       {                                                               \
> > > > > > > +       (struct resource) {                                     \
> > > > > >
> > > > > > Yep, that's it.
> > > > > >
> > > > > > >                 .start = (_start),                                      \
> > > > > > >                 .end = (_start) + (_size) - 1,                          \
> > > > > > >                 .name = (_name),                                        \
> > > > > > >
> > > > > > > But there are some instances which need to be touched, for example
> > > > > > > vexpress-sysreg.c [1]. Are you OK with files to be changed?
> > > > > >
> > > > > > Nice! That's exactly my point and you can sell it to the community
> > > > > > because there are already users of it like this.
> > > > > >
> > > > > > Yes, I'm fine, but it seems it needs to be done treewide in one patch.
> > > > > > Btw, how many of those already in use?
> > > > >
> > > > > Actually you don't need to change that. It's an array of resources and
> > > > > everything should be kept as is there.
> > > > >
> > > > I do get below build failures, with the above literal change for
> > > > vexpress-sysreg.c.
> > > >
> > > > drivers/mfd/vexpress-sysreg.c: At top level:
> > > > drivers/mfd/vexpress-sysreg.c:64:37: error: initialiser element is not constant
> > > >    64 |   .resources = (struct resource []) {
> > > >       |                                     ^
> > > > drivers/mfd/vexpress-sysreg.c:64:37: note: (near initialisation for
> > > > ‘vexpress_sysreg_cells[0]’)
>
> > > Hmm... Interesting, so I suppose the fix is to drop (struct resource
> > > []) parts from the driver?
> > >
> > A bit more than that like something below:
>
> > - .resources = (struct resource []) {
> > - DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),
> > - },
> > + .resources = &DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),
>
> This is not an equivalent change.
> The warning is about const qualifier. Can it rather be  const struct
> resource [] ?
>
No, since it's just a single resource, just the below should be OK.

.resources = &DEFINE_RES_MEM_NAMED(SYS_LED, 0x4, "dat"),

[1] https://elixir.bootlin.com/linux/v5.16-rc8/source/include/linux/mfd/core.h#L108

On the other note I could use the below without changing the macro:

if (dev_of_node(...))
  res_irq = (struct resource) DEFINE_RES_IRQ_NAMED(...)
else
  res_irq =  (struct resource) DEFINE_RES_IRQ(...);
res_irq->flags |= irq_get_trigger_type(irq);

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 5a89d885d0e3..c3c78e1afdda 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>
@@ -428,6 +430,7 @@  static int vpif_probe(struct platform_device *pdev)
 	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,12 +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);
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		pm_runtime_put(&pdev->dev);
+		return irq;
+	}
+	res_irq = devm_kzalloc(&pdev->dev, sizeof(*res_irq), GFP_KERNEL);
 	if (!res_irq) {
-		dev_warn(&pdev->dev, "Missing IRQ resource.\n");
 		pm_runtime_put(&pdev->dev);
-		return -EINVAL;
+		return -ENOMEM;
 	}
+	res_irq->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
+	res_irq->start = irq;
+	res_irq->end = irq;
+	res_irq->name = dev_of_node(&pdev->dev) ? of_node_full_name(pdev->dev.of_node) : NULL;
 
 	pdev_capture = devm_kzalloc(&pdev->dev, sizeof(*pdev_capture),
 				    GFP_KERNEL);
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 8fe55374c5a3..c1a67a221743 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,14 @@  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 ((err = platform_get_irq_optional(pdev, res_idx)) != -ENXIO) {
+		if (err < 0)
+			goto vpif_unregister;
+		err = devm_request_irq(&pdev->dev, err, 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..9c552ea3787e 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,13 @@  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 ((err = platform_get_irq_optional(pdev, res_idx)) != -ENXIO) {
+		if (err < 0)
+			goto vpif_unregister;
+		err = devm_request_irq(&pdev->dev, err, 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;
 		}