[v2,20/22] device property: Modify fwnode irq_get() to use resource

  The underlying ACPI and OF subsystems provide their own APIs which
provide IRQ information as a struct resource. This allows callers to get
more information about the IRQ by looking at the resource flags.  For
example, whether or not an IRQ is wake capable.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-New patch

 drivers/acpi/property.c  | 11 +++++------
 drivers/base/property.c  | 24 +++++++++++++++++++++---
 drivers/of/property.c    |  8 ++++----
 include/linux/fwnode.h   |  8 +++++---
 include/linux/property.h |  2 ++
 5 files changed, 37 insertions(+), 16 deletions(-)
  

Hi Mark,

Thank you for the patch.

On Wed, Dec 20, 2023 at 04:54:34PM -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags.  For
> example, whether or not an IRQ is wake capable.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
> Changes in v2:
> -New patch
> 
>  drivers/acpi/property.c  | 11 +++++------
>  drivers/base/property.c  | 24 +++++++++++++++++++++---
>  drivers/of/property.c    |  8 ++++----
>  include/linux/fwnode.h   |  8 +++++---
>  include/linux/property.h |  2 ++
>  5 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index a6ead5204046b..259869987ffff 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1627,17 +1627,16 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> -static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
> -			       unsigned int index)
> +static int acpi_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +					unsigned int index, struct resource *r)
>  {
> -	struct resource res;
>  	int ret;
>  
> -	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, &res);
> +	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, r);
>  	if (ret)
>  		return ret;
>  
> -	return res.start;
> +	return r->start;
>  }
>  
>  #define DECLARE_ACPI_FWNODE_OPS(ops) \
> @@ -1664,7 +1663,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
>  			acpi_graph_get_remote_endpoint,			\
>  		.graph_get_port_parent = acpi_fwnode_get_parent,	\
>  		.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint, \
> -		.irq_get = acpi_fwnode_irq_get,				\
> +		.irq_get_resource = acpi_fwnode_irq_get_resource,	\
>  	};								\
>  	EXPORT_SYMBOL_GPL(ops)
>  
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a1b01ab420528..4f5d5ab5ab8cf 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1047,23 +1047,41 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
>  EXPORT_SYMBOL(fwnode_iomap);
>  
>  /**
> - * fwnode_irq_get - Get IRQ directly from a fwnode
> + * fwnode_irq_get_resource - Get IRQ directly from a fwnode and populate
> + *			     the resource struct
>   * @fwnode:	Pointer to the firmware node
>   * @index:	Zero-based index of the IRQ
> + * @r:		Pointer to resource to populate with IRQ information.
>   *
>   * Return: Linux IRQ number on success. Negative errno on failure.
>   */
> -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r)
>  {
>  	int ret;
>  
> -	ret = fwnode_call_int_op(fwnode, irq_get, index);
> +	ret = fwnode_call_int_op(fwnode, irq_get_resource, index, r);
>  	/* We treat mapping errors as invalid case */
>  	if (ret == 0)
>  		return -EINVAL;
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(fwnode_irq_get_resource);

Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.

With this changed,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

In fact this should have always been the case for fwnode_irq_get(). I
wouldn't mind changing that, too, in a separate patch.

> +
> +/**
> + * fwnode_irq_get - Get IRQ directly from a fwnode
> + * @fwnode:	Pointer to the firmware node
> + * @index:	Zero-based index of the IRQ
> + *
> + * Return: Linux IRQ number on success. Negative errno on failure.
> + */
> +int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +{
> +	struct resource r;
> +
> +	return fwnode_irq_get_resource(fwnode, index, &r);
> +}
>  EXPORT_SYMBOL(fwnode_irq_get);
>  
>  /**
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index afdaefbd03f61..864ea5fa5702b 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1425,10 +1425,10 @@ static void __iomem *of_fwnode_iomap(struct fwnode_handle *fwnode, int index)
>  #endif
>  }
>  
> -static int of_fwnode_irq_get(const struct fwnode_handle *fwnode,
> -			     unsigned int index)
> +static int of_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +				      unsigned int index, struct resource *r)
>  {
> -	return of_irq_get(to_of_node(fwnode), index);
> +	return of_irq_to_resource(to_of_node(fwnode), index, r);
>  }
>  
>  static int of_fwnode_add_links(struct fwnode_handle *fwnode)
> @@ -1469,7 +1469,7 @@ const struct fwnode_operations of_fwnode_ops = {
>  	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
>  	.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
>  	.iomap = of_fwnode_iomap,
> -	.irq_get = of_fwnode_irq_get,
> +	.irq_get_resource = of_fwnode_irq_get_resource,
>  	.add_links = of_fwnode_add_links,
>  };
>  EXPORT_SYMBOL_GPL(of_fwnode_ops);
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 2a72f55d26eb8..716ed863acde0 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -9,10 +9,11 @@
>  #ifndef _LINUX_FWNODE_H_
>  #define _LINUX_FWNODE_H_
>  
> -#include <linux/types.h>
> -#include <linux/list.h>
>  #include <linux/bits.h>
>  #include <linux/err.h>
> +#include <linux/ioport.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
>  
>  struct fwnode_operations;
>  struct device;
> @@ -164,7 +165,8 @@ struct fwnode_operations {
>  	int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
>  				    struct fwnode_endpoint *endpoint);
>  	void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
> -	int (*irq_get)(const struct fwnode_handle *fwnode, unsigned int index);
> +	int (*irq_get_resource)(const struct fwnode_handle *fwnode,
> +				unsigned int index, struct resource *r);
>  	int (*add_links)(struct fwnode_handle *fwnode);
>  };
>  
> diff --git a/include/linux/property.h b/include/linux/property.h
> index e6516d0b7d52a..685ba72a8ce9e 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -190,6 +190,8 @@ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
>  void fwnode_handle_put(struct fwnode_handle *fwnode);
>  
>  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r);
>  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
>  
>  unsigned int device_get_child_node_count(const struct device *dev);
  

On Wed, Dec 20, 2023 at 04:54:34PM -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags.  For

Double space when other lines have a single space.

> example, whether or not an IRQ is wake capable.

Suggested-by?

...

> -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r)

It's perfectly fine to replace ) by , on the previous line, no need
to make it shorter.

...

> +int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +{
> +	struct resource r;

	struct resource r = {};

?

> +	return fwnode_irq_get_resource(fwnode, index, &r);
> +}
  

> > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > +                         unsigned int index, struct resource *r)
>
> It's perfectly fine to replace ) by , on the previous line, no need
> to make it shorter.

That puts the line at 115 chars? checkpatch.pl allows a maximum line
length of 100. I can bump the 'index' argument up a line and keep it
to a length of 95?
  

> Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
> instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.
>
> With this changed,
>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> In fact this should have always been the case for fwnode_irq_get(). I
> wouldn't mind changing that, too, in a separate patch.

Sure. It looks like fwnode_iomap(), fwnode_irq_get(),
fwnode_irq_get_byname(), and fwnode_graph_parse_endpoint() could all
be updated. I'll add another patch with these changes unless there's a
reason some of those functions shouldn't be GPL'd?
  

>> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
>> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
>> > > +                         unsigned int index, struct resource *r)
>> >
>> > It's perfectly fine to replace ) by , on the previous line, no need
>> > to make it shorter.
>>
>> That puts the line at 115 chars? checkpatch.pl allows a maximum line
>> length of 100. I can bump the 'index' argument up a line and keep it
>> to a length of 95?
>
>
> clang-format should do the right thing.

It formats the line as-is in the patch: with 'unsigned int index' on
the next line.
  

On Thu, Dec 21, 2023 at 04:46:11PM -0700, Mark Hasemeyer wrote:
> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > > +                         unsigned int index, struct resource *r)
> >
> > It's perfectly fine to replace ) by , on the previous line, no need
> > to make it shorter.
> 
> That puts the line at 115 chars? checkpatch.pl allows a maximum line
> length of 100. I can bump the 'index' argument up a line and keep it
> to a length of 95?

Yes, the point is to leave index on the previous line and add a new one with
the r.
  

On Thu, Dec 21, 2023 at 04:47:59PM -0700, Raul Rangel wrote:
> On Thu, Dec 21, 2023 at 4:46 PM Mark Hasemeyer <markhas@chromium.org> wrote:
> 
> > > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int
> > index)
> > > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > > > +                         unsigned int index, struct resource *r)
> > >
> > > It's perfectly fine to replace ) by , on the previous line, no need
> > > to make it shorter.
> >
> > That puts the line at 115 chars? checkpatch.pl allows a maximum line
> > length of 100. I can bump the 'index' argument up a line and keep it
> > to a length of 95?
> 
> clang-format should do the right thing.

Sorry, but no. It has some interesting results sometimes.
Like any other tool it has to be used with caution and
common sense. If it works in the particular case, fine.
  

On Thu, Dec 21, 2023 at 04:59:42PM -0700, Mark Hasemeyer wrote:
> >> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> >> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> >> > > +                         unsigned int index, struct resource *r)
> >> >
> >> > It's perfectly fine to replace ) by , on the previous line, no need
> >> > to make it shorter.
> >>
> >> That puts the line at 115 chars? checkpatch.pl allows a maximum line
> >> length of 100. I can bump the 'index' argument up a line and keep it
> >> to a length of 95?
> >
> > clang-format should do the right thing.
> 
> It formats the line as-is in the patch: with 'unsigned int index' on
> the next line.

Exactly, and I don't think we need that "smartness" in this case.
  

On Thu, Dec 21, 2023 at 04:52:15PM -0700, Mark Hasemeyer wrote:
> > Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
> > instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.
> >
> > With this changed,
> >
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > In fact this should have always been the case for fwnode_irq_get(). I
> > wouldn't mind changing that, too, in a separate patch.
> 
> Sure. It looks like fwnode_iomap(), fwnode_irq_get(),
> fwnode_irq_get_byname(), and fwnode_graph_parse_endpoint() could all
> be updated. I'll add another patch with these changes unless there's a
> reason some of those functions shouldn't be GPL'd?

Interesting... Once should look at the history of those changes.
I don't think anything prevents us from moving to GPLed versions.