[PATCH v3 0/3] Enable named interrupt smbus-alert for ACPI

[PATCH v3 0/3] Enable named interrupt smbus-alert for ACPI

[Akhil R]

I2C - SMBus core drivers use named interrupts to support smbus_alert.
As named interrupts are not available for ACPI based systems, it was
required to change the i2c bus controller driver if to use smbus alert.
These patches provide option for named interrupts in ACPI and  make the
implementation similar to DT. This will enable use of interrupt named
'smbus-alert' in ACPI as well which will be taken during i2c adapter
register.

v2->v3:
  * Grammar/spelling corrections.
  * Added description in function comments.
  * Removed 'unlikely' from NULL check on 'name'
v1->v2:
  * Added firmware guide documentation for ACPI named interrupts
  * Updates in function description comments

Akhil R (3):
  device property: Add device_irq_get_byname
  docs: firmware-guide: ACPI: Add named interrupt doc
  i2c: smbus: Use device_*() functions instead of of_*()

 Documentation/firmware-guide/acpi/enumeration.rst | 38 +++++++++++++++++
 drivers/base/property.c                           | 51 +++++++++++++++++++++++
 drivers/i2c/i2c-core-base.c                       |  2 +-
 drivers/i2c/i2c-core-smbus.c                      | 10 ++---
 drivers/i2c/i2c-smbus.c                           |  2 +-
 include/linux/i2c-smbus.h                         |  6 +--
 include/linux/property.h                          |  3 ++
 7 files changed, 102 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] device property: Add device_irq_get_byname

[Akhil R]

Add device_irq_get_byname() to get an interrupt by name from both the
ACPI table and the Device Tree.

This will allow to use 'interrupt-names' in _DSD which can be mapped to
Interrupt() resource by index. The implementation is similar to
'interrupt-names' in the Device Tree.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 drivers/base/property.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 54 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e6497f6..962a645 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -936,6 +936,57 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
 EXPORT_SYMBOL(fwnode_iomap);
 
 /**
+ * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
+ * @fwnode:	Pointer to the firmware node
+ * @name:	IRQ name
+ *
+ * Description:
+ * Find a match to the string 'name' in the 'interrupt-names' string array
+ * in _DSD for ACPI, or of_node for device tree. Then get the Linux IRQ
+ * number of the IRQ resource corresponding to the index of the matched
+ * string.
+ *
+ * Return:
+ * Linux IRQ number on success
+ * Negative errno otherwise.
+ */
+int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
+{
+	int index;
+
+	if (!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);
+
+/**
+ * device_irq_get_byname - Get IRQ of a device using interrupt name
+ * @dev: Device to get the interrupt
+ * @name: IRQ name
+ *
+ * Description:
+ * Find a match to the string 'name' in the 'interrupt-names' string array
+ * in _DSD for ACPI, or of_node for device tree. Then get the Linux IRQ
+ * number of the IRQ resource corresponding to the index of the matched
+ * string.
+ *
+ * Return:
+ * Linux IRQ number on success
+ * Negative errno otherwise.
+ */
+int device_irq_get_byname(struct device *dev, const char *name)
+{
+	return fwnode_irq_get_byname(dev_fwnode(dev), name);
+}
+EXPORT_SYMBOL_GPL(device_irq_get_byname);
+
+/**
  * fwnode_graph_get_next_endpoint - Get next endpoint firmware node
  * @fwnode: Pointer to the parent firmware node
  * @prev: Previous endpoint node or %NULL to get the first
diff --git a/include/linux/property.h b/include/linux/property.h
index 7399a0b..3123b75 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -121,6 +121,9 @@ 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_byname(const struct fwnode_handle *fwnode, const char *name);
+
+int device_irq_get_byname(struct device *dev, const char *name);
 
 void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
 
-- 
2.7.4

[PATCH v3 2/3] docs: firmware-guide: ACPI: Add named interrupt doc

[Akhil R]

Added details and example for named interrupts in the ACPI table.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 Documentation/firmware-guide/acpi/enumeration.rst | 38 +++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/enumeration.rst b/Documentation/firmware-guide/acpi/enumeration.rst
index 74b830b2..595deba 100644
--- a/Documentation/firmware-guide/acpi/enumeration.rst
+++ b/Documentation/firmware-guide/acpi/enumeration.rst
@@ -143,6 +143,44 @@ In robust cases the client unfortunately needs to call
 acpi_dma_request_slave_chan_by_index() directly and therefore choose the
 specific FixedDMA resource by its index.
 
+Named Interrupts
+================
+
+Drivers enumerated via ACPI can have names to interrupts in the ACPI table
+which can be used to get the IRQ number in the driver.
+
+The interrupt name can be listed in _DSD as 'interrupt-names'. The names
+should be listed as an array of strings which will map to the Interrupt()
+resource in the ACPI table corresponding to its index.
+
+The table below shows an example of its usage::
+
+    Device (DEV0) {
+        ...
+        Name (_CRS, ResourceTemplate() {
+            ...
+            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+                0x20,
+                0x24
+            }
+        })
+
+        Name (_DSD, Package () {
+            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+            Package () {
+                Package () {"interrupt-names",
+                Package (2) {"default", "alert"}},
+            }
+        ...
+        })
+    }
+
+The interrupt name 'default' will correspond to 0x20 in Interrupt()
+resource and 'alert' to 0x24.
+
+The driver can call the function - device_irq_get_byname() with the device
+and interrupt name as arguments to get the corresponding IRQ number.
+
 SPI serial bus support
 ======================
 
-- 
2.7.4

[PATCH v3 3/3] i2c: smbus: Use device_*() functions instead of of_*()

[Akhil R]

Change of_*() functions to device_*() for firmware agnostic usage.
This allows to have the smbus_alert interrupt without any changes
in the controller drivers using the ACPI table.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 drivers/i2c/i2c-core-base.c  |  2 +-
 drivers/i2c/i2c-core-smbus.c | 10 +++++-----
 drivers/i2c/i2c-smbus.c      |  2 +-
 include/linux/i2c-smbus.h    |  6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2c59dd7..32a4526 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1479,7 +1479,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_list;
 	}
 
-	res = of_i2c_setup_smbus_alert(adap);
+	res = i2c_setup_smbus_alert(adap);
 	if (res)
 		goto out_reg;
 
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index e5b2d14..4c24c84 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -701,13 +701,13 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter,
 }
 EXPORT_SYMBOL_GPL(i2c_new_smbus_alert_device);
 
-#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
-int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+int i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 {
 	int irq;
 
-	irq = of_property_match_string(adapter->dev.of_node, "interrupt-names",
-				       "smbus_alert");
+	irq = device_property_match_string(adapter->dev.parent, "interrupt-names",
+					   "smbus_alert");
 	if (irq == -EINVAL || irq == -ENODATA)
 		return 0;
 	else if (irq < 0)
@@ -715,5 +715,5 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 
 	return PTR_ERR_OR_ZERO(i2c_new_smbus_alert_device(adapter, NULL));
 }
-EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
+EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
 #endif
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index d3d06e3..fdd6d97 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -128,7 +128,7 @@ static int smbalert_probe(struct i2c_client *ara,
 	if (setup) {
 		irq = setup->irq;
 	} else {
-		irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
+		irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert");
 		if (irq <= 0)
 			return irq;
 	}
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 1ef4218..95cf902 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -30,10 +30,10 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter,
 					      struct i2c_smbus_alert_setup *setup);
 int i2c_handle_smbus_alert(struct i2c_client *ara);
 
-#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
-int of_i2c_setup_smbus_alert(struct i2c_adapter *adap);
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+int i2c_setup_smbus_alert(struct i2c_adapter *adap);
 #else
-static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
+static inline int i2c_setup_smbus_alert(struct i2c_adapter *adap)
 {
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v3 1/3] device property: Add device_irq_get_byname

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 3:45 PM Akhil R <akhilrajeev@nvidia.com> wrote:

Thanks, my comments below.

> Add device_irq_get_byname() to get an interrupt by name from both the
> ACPI table and the Device Tree.

This needs to be clarified (it's not and, but or), what about:

  Add device_irq_get_byname() to get an interrupt by name from either
  ACPI table or Device Tree whichever has it.

> This will allow to use 'interrupt-names' in _DSD which can be mapped to

In the ACPI case this
allow us to

> Interrupt() resource by index. The implementation is similar to
> 'interrupt-names' in the Device Tree.

...

>  /**
> + * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
> + * @fwnode:    Pointer to the firmware node
> + * @name:      IRQ name
> + *
> + * Description:
> + * Find a match to the string 'name' in the 'interrupt-names' string array

'name' --> @name

> + * in _DSD for ACPI, or of_node for device tree. Then get the Linux IRQ

Device Tree

> + * number of the IRQ resource corresponding to the index of the matched
> + * string.
> + *
> + * Return:

> + * Linux IRQ number on success
> + * Negative errno otherwise.

 * Linux IRQ number on success, or negative errno otherwise.

> + */
> +int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
> +{
> +       int index;
> +
> +       if (!name)
> +               return -EINVAL;
> +
> +       index = fwnode_property_match_string(fwnode, "interrupt-names",  name);
> +       if (index < 0)
> +               return index;
> +
> +       return fwnode_irq_get(fwnode, index);
> +}

...

> +/**
> + * device_irq_get_byname - Get IRQ of a device using interrupt name
> + * @dev: Device to get the interrupt
> + * @name: IRQ name
> + *
> + * Description:
> + * Find a match to the string 'name' in the 'interrupt-names' string array
> + * in _DSD for ACPI, or of_node for device tree. Then get the Linux IRQ
> + * number of the IRQ resource corresponding to the index of the matched
> + * string.
> + *
> + * Return:
> + * Linux IRQ number on success
> + * Negative errno otherwise.
> + */

As per above.

...

> +int device_irq_get_byname(struct device *dev, const char *name);

Since we don't have device_irq_get() perhaps we don't need this one
right now (just open code it in the caller). This will satisfy
Rafael's request.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] docs: firmware-guide: ACPI: Add named interrupt doc

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 3:45 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Added details and example for named interrupts in the ACPI table.

Added details and example for --> Add a detailed example of the

...

> +            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
> +                0x20,

I would think of splitting this to two separate entries in between of
which the GpioInt() resource is provided. It will explicitly show that
you describe the case only for Interrupt(). Something like

  Interrupt (...) { 0x20 }
  GpioInt(...) { ... }
  Interrupt (...) { 0x24 }

But it's up to you.

> +                0x24
> +            }

...

> +The driver can call the function - device_irq_get_byname() with the device
> +and interrupt name as arguments to get the corresponding IRQ number.

Needs switch to fwnode as per comment against the previous patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] i2c: smbus: Use device_*() functions instead of of_*()

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 3:45 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Change of_*() functions to device_*() for firmware agnostic usage.
> This allows to have the smbus_alert interrupt without any changes
> in the controller drivers using the ACPI table.

This patch LGTM.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

The 0 check needs a separate discussion and fixing, which is out of scope here.

> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  drivers/i2c/i2c-core-base.c  |  2 +-
>  drivers/i2c/i2c-core-smbus.c | 10 +++++-----
>  drivers/i2c/i2c-smbus.c      |  2 +-
>  include/linux/i2c-smbus.h    |  6 +++---
>  4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 2c59dd7..32a4526 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1479,7 +1479,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>                 goto out_list;
>         }
>
> -       res = of_i2c_setup_smbus_alert(adap);
> +       res = i2c_setup_smbus_alert(adap);
>         if (res)
>                 goto out_reg;
>
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index e5b2d14..4c24c84 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -701,13 +701,13 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter,
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_smbus_alert_device);
>
> -#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
> -int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +int i2c_setup_smbus_alert(struct i2c_adapter *adapter)
>  {
>         int irq;
>
> -       irq = of_property_match_string(adapter->dev.of_node, "interrupt-names",
> -                                      "smbus_alert");
> +       irq = device_property_match_string(adapter->dev.parent, "interrupt-names",
> +                                          "smbus_alert");
>         if (irq == -EINVAL || irq == -ENODATA)
>                 return 0;
>         else if (irq < 0)
> @@ -715,5 +715,5 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
>
>         return PTR_ERR_OR_ZERO(i2c_new_smbus_alert_device(adapter, NULL));
>  }
> -EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
> +EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
>  #endif
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index d3d06e3..fdd6d97 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -128,7 +128,7 @@ static int smbalert_probe(struct i2c_client *ara,
>         if (setup) {
>                 irq = setup->irq;
>         } else {
> -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> +               irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert");
>                 if (irq <= 0)
>                         return irq;
>         }
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 1ef4218..95cf902 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -30,10 +30,10 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter,
>                                               struct i2c_smbus_alert_setup *setup);
>  int i2c_handle_smbus_alert(struct i2c_client *ara);
>
> -#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
> -int of_i2c_setup_smbus_alert(struct i2c_adapter *adap);
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +int i2c_setup_smbus_alert(struct i2c_adapter *adap);
>  #else
> -static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
> +static inline int i2c_setup_smbus_alert(struct i2c_adapter *adap)
>  {
>         return 0;
>  }
> --
> 2.7.4
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 1/3] device property: Add device_irq_get_byname

[Akhil R]

> Thanks, my comments below.
Thanks for the inputs. 
> 
> > Add device_irq_get_byname() to get an interrupt by name from both the
> > ACPI table and the Device Tree.
> 
> This needs to be clarified (it's not and, but or), what about:
> 
>   Add device_irq_get_byname() to get an interrupt by name from either
>   ACPI table or Device Tree whichever has it.
> 
> > This will allow to use 'interrupt-names' in _DSD which can be mapped
> > to
> 
> In the ACPI case this
> allow us to
> 
> > Interrupt() resource by index. The implementation is similar to
> > 'interrupt-names' in the Device Tree.
> 
> ...
> 
> >  /**
> > + * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
> > + * @fwnode:    Pointer to the firmware node
> > + * @name:      IRQ name
> > + *
> > + * Description:
> > + * Find a match to the string 'name' in the 'interrupt-names' string
> > + array
> 
> 'name' --> @name
> 
> > + * in _DSD for ACPI, or of_node for device tree. Then get the Linux
> > + IRQ
> 
> Device Tree
> 
> > + * number of the IRQ resource corresponding to the index of the
> > + matched
> > + * string.
> > + *
> > + * Return:
> 
> > + * Linux IRQ number on success
> > + * Negative errno otherwise.
> 
>  * Linux IRQ number on success, or negative errno otherwise.
> 
> > + */
> > +int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const
> > +char *name) {
> > +       int index;
> > +
> > +       if (!name)
> > +               return -EINVAL;
> > +
> > +       index = fwnode_property_match_string(fwnode, "interrupt-names",
> name);
> > +       if (index < 0)
> > +               return index;
> > +
> > +       return fwnode_irq_get(fwnode, index); }
> 
> ...
> 
> > +/**
> > + * device_irq_get_byname - Get IRQ of a device using interrupt name
> > + * @dev: Device to get the interrupt
> > + * @name: IRQ name
> > + *
> > + * Description:
> > + * Find a match to the string 'name' in the 'interrupt-names' string
> > +array
> > + * in _DSD for ACPI, or of_node for device tree. Then get the Linux
> > +IRQ
> > + * number of the IRQ resource corresponding to the index of the
> > +matched
> > + * string.
> > + *
> > + * Return:
> > + * Linux IRQ number on success
> > + * Negative errno otherwise.
> > + */
> 
> As per above.
> 
> ...
> 
> > +int device_irq_get_byname(struct device *dev, const char *name);
> 
> Since we don't have device_irq_get() perhaps we don't need this one right now
> (just open code it in the caller). This will satisfy Rafael's request.

If to code the same in caller, I guess, it would look like this -
	 irq = fwnode_irq_get_byname(dev_fwnode(adapter->dev.parent),
					 "smbus_alert");

Looks okay to me, but if given an option I would go with device_irq_get_byname().

Thanks,
Akhil

Re: [PATCH v3 1/3] device property: Add device_irq_get_byname

[Andy Shevchenko]

On Fri, Jan 21, 2022 at 11:18 AM Akhil R <akhilrajeev@nvidia.com> wrote:

...

> > > +int device_irq_get_byname(struct device *dev, const char *name);
> >
> > Since we don't have device_irq_get() perhaps we don't need this one right now
> > (just open code it in the caller). This will satisfy Rafael's request.
>
> If to code the same in caller, I guess, it would look like this -
>          irq = fwnode_irq_get_byname(dev_fwnode(adapter->dev.parent),
>                                          "smbus_alert");

Yep, I meant how you point to it in the documentation, e.g.

  The user may call fwnode_irq_get_byname() with the firmware node and
name of the IRQ it wants to retrieve.


> Looks okay to me, but if given an option I would go with device_irq_get_byname().

You see, there was a query from Rafael and I haven't seen an answer
yet. On top there is no such function for fwnode_irq_get() (I mean
device_irq_get() API). It would be harder to push without good
justification why one has the device_ counterpart and the other does
not. Easiest way, as I see it, is to drop it for now.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 1/3] device property: Add device_irq_get_byname

[Akhil R]

> Thanks, my comments below.
> 
> > Add device_irq_get_byname() to get an interrupt by name from both the
> > ACPI table and the Device Tree.
> 
> This needs to be clarified (it's not and, but or), what about:
> 
>   Add device_irq_get_byname() to get an interrupt by name from either
>   ACPI table or Device Tree whichever has it.
Does 'whichever has it' mean a bit different here? Would it be good like this -?

    Add fwnode_irq_get_byname() to get an interrupt by name from either
    ACPI table or Device Tree, whichever is used for enumeration.

Okay with the other comments.

Thanks,
Akhil

RE: [PATCH v3 2/3] docs: firmware-guide: ACPI: Add named interrupt doc

[Akhil R]

> > Added details and example for named interrupts in the ACPI table.
> 
> Added details and example for --> Add a detailed example of the
> 
> ...
> 
> > +            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
> > +                0x20,
> 
> I would think of splitting this to two separate entries in between of which the
> GpioInt() resource is provided. It will explicitly show that you describe the case
> only for Interrupt(). Something like
> 
>   Interrupt (...) { 0x20 }
>   GpioInt(...) { ... }
>   Interrupt (...) { 0x24 }
> 
> But it's up to you.
Instead, would it be good to add a statement mentioning this explicitly. Something 
like -

    The interrupt name 'default' will correspond to 0x20 in Interrupt()
    resource and 'alert' to 0x24. Note that only the Interrupt() resource
    is mapped and not GpioInt() or similar.

I feel mixing these in the example would add a bit of confusion to the reader.

Thanks,
Akhil

Re: [PATCH v3 1/3] device property: Add device_irq_get_byname

[Andy Shevchenko]

On Fri, Jan 21, 2022 at 2:29 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> > Thanks, my comments below.
> >
> > > Add device_irq_get_byname() to get an interrupt by name from both the
> > > ACPI table and the Device Tree.
> >
> > This needs to be clarified (it's not and, but or), what about:
> >
> >   Add device_irq_get_byname() to get an interrupt by name from either
> >   ACPI table or Device Tree whichever has it.
> Does 'whichever has it' mean a bit different here? Would it be good like this -?
>
>     Add fwnode_irq_get_byname() to get an interrupt by name from either
>     ACPI table or Device Tree, whichever is used for enumeration.

Yes, your variant is good.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] docs: firmware-guide: ACPI: Add named interrupt doc

[Andy Shevchenko]

On Fri, Jan 21, 2022 at 2:50 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> > > Added details and example for named interrupts in the ACPI table.
> >
> > Added details and example for --> Add a detailed example of the
> >
> > ...
> >
> > > +            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
> > > +                0x20,
> >
> > I would think of splitting this to two separate entries in between of which the
> > GpioInt() resource is provided. It will explicitly show that you describe the case
> > only for Interrupt(). Something like
> >
> >   Interrupt (...) { 0x20 }
> >   GpioInt(...) { ... }
> >   Interrupt (...) { 0x24 }
> >
> > But it's up to you.
> Instead, would it be good to add a statement mentioning this explicitly. Something
> like -
>
>     The interrupt name 'default' will correspond to 0x20 in Interrupt()
>     resource and 'alert' to 0x24. Note that only the Interrupt() resource
>     is mapped and not GpioInt() or similar.
>
> I feel mixing these in the example would add a bit of confusion to the reader.

That's why I added "up to you" in my previous reply. I also thought
about the example being a bit confusing for a reader who is
non-familiar with the ACPI.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/3] docs: firmware-guide: ACPI: Add named interrupt doc

[Akhil R]

Fri, Jan 21, 2022 at 2:50 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > > > Added details and example for named interrupts in the ACPI table.
> > >
> > > Added details and example for --> Add a detailed example of the
> > >
> > > ...
> > >
> > > > +            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
> > > > +                0x20,
> > >
> > > I would think of splitting this to two separate entries in between
> > > of which the
> > > GpioInt() resource is provided. It will explicitly show that you
> > > describe the case only for Interrupt(). Something like
> > >
> > >   Interrupt (...) { 0x20 }
> > >   GpioInt(...) { ... }
> > >   Interrupt (...) { 0x24 }
> > >
> > > But it's up to you.
> > Instead, would it be good to add a statement mentioning this
> > explicitly. Something like -
> >
> >     The interrupt name 'default' will correspond to 0x20 in Interrupt()
> >     resource and 'alert' to 0x24. Note that only the Interrupt() resource
> >     is mapped and not GpioInt() or similar.
> >
> > I feel mixing these in the example would add a bit of confusion to the reader.
> 
> That's why I added "up to you" in my previous reply. I also thought about the
> example being a bit confusing for a reader who is non-familiar with the ACPI.

Thanks. Will send out an updated patch.

Thanks,
Akhil