[PATCH v2 0/2] device property: Adding fwnode_irq_get_byname()

[PATCH v2 0/2] device property: Adding fwnode_irq_get_byname()

[Puranjay Mohan]

The first patch in this series adds the fwnode_irq_get_byname() which is
the generic version of the of_irq_get_byname(). It is used to get the
IRQ number from name of the interrupt.

The second patch in this series uses the fwnode_irq_get_byname()
function in the IIO driver of the ADXL355 accelerometer. The driver has
been tested after applying this patch on a Raspberry PI. The ADXL355 was
connected to the Raspberry Pi using I2C and fwnode_irq_get_byname() was
used to get the IRQ number for the "DRDY" interrupt. Earlier this driver
was using of_irq_get_byname() to get this IRQ number.

Puranjay Mohan (2):
  device property: Add fwnode_irq_get_byname()
  iio: accel: adxl355: use fwnode_irq_get_byname()

 drivers/base/property.c          | 23 +++++++++++++++++++++++
 drivers/iio/accel/adxl355_core.c |  7 ++-----
 include/linux/property.h         |  2 ++
 3 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.30.1

[PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

[Puranjay Mohan]

The fwnode framework did not have means to obtain the IRQ number from
the name of a node.
Add that now, in form of the fwnode_irq_get_byname() function.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/base/property.c  | 23 +++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f1f35b48ab8b..0d685c79b0e8 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -958,6 +958,29 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
 }
 EXPORT_SYMBOL(fwnode_irq_get);
 
+/**
+ * fwnode_irq_get_byname - Get IRQ directly from its name.
+ * @fwnode:    Pointer to the firmware node
+ * @name:      IRQ Name
+ *
+ * Returns Linux IRQ number on success. Other values are determined
+ * accordingly to acpi_/of_ irq_get() operation.
+ */
+int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name)
+{
+	int index;
+
+	if (unlikely(!name))
+		return -EINVAL;
+
+	index = fwnode_property_match_string(fwnode, "interrupt-names", name);
+	if (index < 0)
+		return index;
+
+	return fwnode_irq_get(fwnode, index);
+}
+EXPORT_SYMBOL(fwnode_irq_get_byname);
+
 /**
  * fwnode_graph_get_next_endpoint - Get next endpoint firmware node
  * @fwnode: Pointer to the parent firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index 88fa726a76df..9c6177597ba8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -122,6 +122,8 @@ void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 
+int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name);
+
 unsigned int device_get_child_node_count(struct device *dev);
 
 static inline bool device_property_read_bool(struct device *dev,
-- 
2.30.1

[PATCH v2 2/2] iio: accel: adxl355: use fwnode_irq_get_byname()

[Puranjay Mohan]

Use the generic fwnode_irq_get_byname() in place of of_irq_get_byname()
to get the IRQ number from the interrupt pin.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/iio/accel/adxl355_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index 4f485909f459..7babb139bb92 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -18,7 +18,7 @@
 #include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
-#include <linux/of_irq.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <asm/unaligned.h>
 
@@ -746,10 +746,7 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
-	/*
-	 * TODO: Would be good to move it to the generic version.
-	 */
-	irq = of_irq_get_byname(dev->of_node, "DRDY");
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "DRDY");
 	if (irq > 0) {
 		ret = adxl355_probe_trigger(indio_dev, irq);
 		if (ret)
-- 
2.30.1

Re: [PATCH v2 0/2] device property: Adding fwnode_irq_get_byname()

[Saravana Kannan]

On Tue, Nov 9, 2021 at 12:09 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> The first patch in this series adds the fwnode_irq_get_byname() which is
> the generic version of the of_irq_get_byname(). It is used to get the
> IRQ number from name of the interrupt.
>
> The second patch in this series uses the fwnode_irq_get_byname()
> function in the IIO driver of the ADXL355 accelerometer. The driver has
> been tested after applying this patch on a Raspberry PI. The ADXL355 was
> connected to the Raspberry Pi using I2C and fwnode_irq_get_byname() was
> used to get the IRQ number for the "DRDY" interrupt. Earlier this driver
> was using of_irq_get_byname() to get this IRQ number.

Why do we need these changes though? Is there a non-OF device this
driver would ever probe?

-Saravana

Re: [PATCH v2 0/2] device property: Adding fwnode_irq_get_byname()

[Andy Shevchenko]

On Tue, Nov 09, 2021 at 02:42:35PM -0800, Saravana Kannan wrote:
> On Tue, Nov 9, 2021 at 12:09 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > The first patch in this series adds the fwnode_irq_get_byname() which is
> > the generic version of the of_irq_get_byname(). It is used to get the
> > IRQ number from name of the interrupt.
> >
> > The second patch in this series uses the fwnode_irq_get_byname()
> > function in the IIO driver of the ADXL355 accelerometer. The driver has
> > been tested after applying this patch on a Raspberry PI. The ADXL355 was
> > connected to the Raspberry Pi using I2C and fwnode_irq_get_byname() was
> > used to get the IRQ number for the "DRDY" interrupt. Earlier this driver
> > was using of_irq_get_byname() to get this IRQ number.
> 
> Why do we need these changes though? Is there a non-OF device this
> driver would ever probe?

Strange question, TBH. All discrete component drivers are subject to
be enumerated on any type of the systems. So, of course the answer
to it "Definitely yes".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] device property: Adding fwnode_irq_get_byname()

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 01:38:38AM +0530, Puranjay Mohan wrote:
> The first patch in this series adds the fwnode_irq_get_byname() which is
> the generic version of the of_irq_get_byname(). It is used to get the
> IRQ number from name of the interrupt.
> 
> The second patch in this series uses the fwnode_irq_get_byname()
> function in the IIO driver of the ADXL355 accelerometer. The driver has
> been tested after applying this patch on a Raspberry PI. The ADXL355 was
> connected to the Raspberry Pi using I2C and fwnode_irq_get_byname() was
> used to get the IRQ number for the "DRDY" interrupt. Earlier this driver
> was using of_irq_get_byname() to get this IRQ number.

This is marked as v2, can we have a link / changelog from v1, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:
> The fwnode framework did not have means to obtain the IRQ number from
> the name of a node.
> Add that now, in form of the fwnode_irq_get_byname() function.

...

> +int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name)
> +{
> +	int index;
> +
> +	if (unlikely(!name))
> +		return -EINVAL;

> +	index = fwnode_property_match_string(fwnode, "interrupt-names", name);
> +	if (index < 0)
> +		return index;

It won't work like this. The ACPI table quite likely won't have this in them.
Also it doesn't cover the GPIO interrupts in ACPI case.

> +	return fwnode_irq_get(fwnode, index);

Neither this covers GPIO IRQs.

> +}
> +EXPORT_SYMBOL(fwnode_irq_get_byname);

So, first you need to provide a design for this how ACPI cases can be handled.

Imagine these cases (at least) for _CRS method in ACPI:
  1/ Single GSI

	Interrupt()

  2/ Single GPIO IRQ

	GpioInt()

  3/ Both in different orders
    a)
	Interrupt()
	GpioInt()

    b)
	GpioInt()
	Interrupt()

  4/ Mixed (complicated cases)

	Interrupt()
	Interrupt()
	GpioInt()
	Interrupt()
	GpioInt()

Obvious question, what does the index mean in all these cases?

Next one is, how can we quirk out the platforms with the old ACPI tables
where no properties are provided? For GPIO there is struct acpi_gpio_params
which goes deep into ACPI glue layer.

Luckily, the GPIO IRQ case has already available APIs for indexing and naming
match: acpi_dev_gpio_irq_get_by().

Hence, the main task is to define index in cases like 4 and see what can be
done for the GSI cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

[Puranjay Mohan]

On Wed, Nov 10, 2021 at 2:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:
> > The fwnode framework did not have means to obtain the IRQ number from
> > the name of a node.
> > Add that now, in form of the fwnode_irq_get_byname() function.
>
> ...
>
> > +int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name)
> > +{
> > +     int index;
> > +
> > +     if (unlikely(!name))
> > +             return -EINVAL;
>
> > +     index = fwnode_property_match_string(fwnode, "interrupt-names", name);
> > +     if (index < 0)
> > +             return index;
>
> It won't work like this. The ACPI table quite likely won't have this in them.
> Also it doesn't cover the GPIO interrupts in ACPI case.
>
> > +     return fwnode_irq_get(fwnode, index);
>
> Neither this covers GPIO IRQs.
>
> > +}
> > +EXPORT_SYMBOL(fwnode_irq_get_byname);
>
> So, first you need to provide a design for this how ACPI cases can be handled.
>
> Imagine these cases (at least) for _CRS method in ACPI:
>   1/ Single GSI
>
>         Interrupt()
>
>   2/ Single GPIO IRQ
>
>         GpioInt()
>
>   3/ Both in different orders
>     a)
>         Interrupt()
>         GpioInt()
>
>     b)
>         GpioInt()
>         Interrupt()
>
>   4/ Mixed (complicated cases)
>
>         Interrupt()
>         Interrupt()
>         GpioInt()
>         Interrupt()
>         GpioInt()
>
> Obvious question, what does the index mean in all these cases?
>
> Next one is, how can we quirk out the platforms with the old ACPI tables
> where no properties are provided? For GPIO there is struct acpi_gpio_params
> which goes deep into ACPI glue layer.
>
> Luckily, the GPIO IRQ case has already available APIs for indexing and naming
> match: acpi_dev_gpio_irq_get_by().
>
> Hence, the main task is to define index in cases like 4 and see what can be
> done for the GSI cases.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Andy,
I wrote this function keeping the device tree in mind. I will have to
look into ACPI and see how the cases you mentioned can be implemented.
Let's see how far I can get with understanding the ACPI.

-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan

Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 10:34:43PM +0530, Puranjay Mohan wrote:
> On Wed, Nov 10, 2021 at 2:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:

> I wrote this function keeping the device tree in mind. I will have to
> look into ACPI and see how the cases you mentioned can be implemented.
> Let's see how far I can get with understanding the ACPI.

Yeah.

What you need to have is
1) expand fwnode_irq_get() to support ACPI GPIO IRQ resources.
2) provide a simple version of the fwnode_irq_get_by_name() like

	if (is_of_node())
		return of_...();

	return acpi_dev_gpio_irq_get_by();

3) establish understanding about naming for ACPI and
4) extend fwnode_irq_get_by_name() by it

	if (is_of_node())
		return of_...();

	ret = acpi_irq_get_by_name();
	if (ret > 0)
		return ret;

	return acpi_dev_gpio_irq_get_by();

As I mentioned, items 1 and 2 are easy to achieve with currently existing APIs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: accel: adxl355: use fwnode_irq_get_byname()

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 01:38:40AM +0530, Puranjay Mohan wrote:
> Use the generic fwnode_irq_get_byname() in place of of_irq_get_byname()
> to get the IRQ number from the interrupt pin.

Heads up, the fwnode_irq_get_byname() is in I2C tree and if Jonathan wants to
apply this one, the PR [1] can be used.

[1]: https://lore.kernel.org/linux-i2c/YfRiGR3AT8tzyweG@shikoro/T/#u

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: accel: adxl355: use fwnode_irq_get_byname()

[Jonathan Cameron]

On Tue, 1 Feb 2022 18:25:54 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Nov 10, 2021 at 01:38:40AM +0530, Puranjay Mohan wrote:
> > Use the generic fwnode_irq_get_byname() in place of of_irq_get_byname()
> > to get the IRQ number from the interrupt pin.  
> 
> Heads up, the fwnode_irq_get_byname() is in I2C tree and if Jonathan wants to
> apply this one, the PR [1] can be used.
> 
> [1]: https://lore.kernel.org/linux-i2c/YfRiGR3AT8tzyweG@shikoro/T/#u
> 

Thanks Andy. 

I'm going to let this go the slow way and pick up this patch after the
merge window when I can avoid pulling in that PR.

Basically I'm feeling lazy today :)

Jonathan

Re: [PATCH v2 2/2] iio: accel: adxl355: use fwnode_irq_get_byname()

[Jonathan Cameron]

On Sun, 27 Feb 2022 12:41:34 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 1 Feb 2022 18:25:54 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Wed, Nov 10, 2021 at 01:38:40AM +0530, Puranjay Mohan wrote:  
> > > Use the generic fwnode_irq_get_byname() in place of of_irq_get_byname()
> > > to get the IRQ number from the interrupt pin.    
> > 
> > Heads up, the fwnode_irq_get_byname() is in I2C tree and if Jonathan wants to
> > apply this one, the PR [1] can be used.
> > 
> > [1]: https://lore.kernel.org/linux-i2c/YfRiGR3AT8tzyweG@shikoro/T/#u
> >   
> 
> Thanks Andy. 
> 
> I'm going to let this go the slow way and pick up this patch after the
> merge window when I can avoid pulling in that PR.
> 
> Basically I'm feeling lazy today :)

Now applied to the togreg branch of iio.git and pushed out as testing to
see if 0-day can find any problems.

Thanks,

Jonathan

> 
> Jonathan

Re: [PATCH v2 2/2] iio: accel: adxl355: use fwnode_irq_get_byname()

[Andy Shevchenko]

On Sun, Apr 10, 2022 at 06:13:06PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Feb 2022 12:41:34 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 1 Feb 2022 18:25:54 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > On Wed, Nov 10, 2021 at 01:38:40AM +0530, Puranjay Mohan wrote:  
> > > > Use the generic fwnode_irq_get_byname() in place of of_irq_get_byname()
> > > > to get the IRQ number from the interrupt pin.    
> > > 
> > > Heads up, the fwnode_irq_get_byname() is in I2C tree and if Jonathan wants to
> > > apply this one, the PR [1] can be used.
> > > 
> > > [1]: https://lore.kernel.org/linux-i2c/YfRiGR3AT8tzyweG@shikoro/T/#u
> > >   
> > 
> > Thanks Andy. 
> > 
> > I'm going to let this go the slow way and pick up this patch after the
> > merge window when I can avoid pulling in that PR.
> > 
> > Basically I'm feeling lazy today :)
> 
> Now applied to the togreg branch of iio.git and pushed out as testing to
> see if 0-day can find any problems.

Thanks for keeping an eye on it!

-- 
With Best Regards,
Andy Shevchenko