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

[PATCH v2 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.

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                           | 35 +++++++++++++++++++++
 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, 86 insertions(+), 10 deletions(-)

-- 
2.7.4

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

[Akhil R]

Get interrupt by name from ACPI table as well.

Add option to use 'interrupt-names' in _DSD which can map to interrupt by
index. The implementation is similar to 'interrupt-names' in devicetree.
Also add a common routine to get irq by name from devicetree and ACPI
table.

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

diff --git a/drivers/base/property.c b/drivers/base/property.c
index cbe4fa2..414c316 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -920,6 +920,41 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
 EXPORT_SYMBOL(fwnode_irq_get);
 
 /**
+ * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
+ * @fwnode:	Pointer to the firmware node
+ * @name:	IRQ name in interrupt-names property in fwnode
+ *
+ * Returns Linux IRQ number on success, errno otherwise.
+ */
+int fwnode_irq_get_byname(const 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);
+
+/**
+ * device_irq_get_byname - Get IRQ of a device using interrupt name
+ * @dev:	Device to get the interrupt
+ * @name:	IRQ name in interrupt-names property in fwnode
+ *
+ * Returns Linux IRQ number on success, 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 16f736c..bc49350 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);
 
 unsigned int device_get_child_node_count(struct device *dev);
 
-- 
2.7.4

[PATCH v2 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..30ae41c 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 with ACPI node can have names to interrupts in 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
+property in 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 property
+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 v2 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 smbus_alert interrupt without any changes
in the controller drivers using 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 1072a47..8e6c7a1 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1574,7 +1574,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 v2 3/3] i2c: smbus: Use device_*() functions instead of of_*()

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 4:15 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Change of_*() functions to device_*() for firmware agnostic usage.
> This allows to have smbus_alert interrupt without any changes

the smbus_alert

> in the controller drivers using ACPI table.

the ACPI

...

This change reveals potential issue:

> -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> +               irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert");

>                 if (irq <= 0)

I guess this '= 0' part should be fixed first.

>                         return irq;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 4:15 PM Akhil R <akhilrajeev@nvidia.com> wrote:

Thanks for doing this, very helpful! My comments below.

> Added details and example for named interrupts in the ACPI Table

Table.

...

> +Named Interrupts
> +================
> +
> +Drivers with ACPI node can have names to interrupts in ACPI table which
> +can be used to get the irq number in the driver.

IRQ

> +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
> +property in ACPI table corresponding to its index.

'Interrupt property' --> 'Interrupt() resource'

the ACPI

> +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"}},
> +                       }

                       Package () {
                               Package () {
                                        "interrupt-names", Package ()
{"default", "alert"}
                               },
                       }

> +                       ...
> +               })
> +       }

Please, drop the indentation to just 4 spaces.

> +The interrupt name 'default' will correspond to 0x20 in Interrupt property

Interrupt() resource

> +and 'alert' to 0x24.
> +
> +The driver can call the function - device_irq_get_byname with the device

device_irq_get_byname()

> +and interrupt name as arguments to get the corresponding irq number.

IRQ

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 4:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:

In the subject line: device_irq_get_byname()

> Get interrupt by name from ACPI table as well.

an interrupt
the ACPI

> Add option to use 'interrupt-names' in _DSD which can map to interrupt by

can be mapped
Interrupt() resource

(The last one is very important to point out this is only about
Interrupt() resources for now).

> index. The implementation is similar to 'interrupt-names' in devicetree.

the Device Tree

> Also add a common routine to get irq by name from devicetree and ACPI

IRQ
Device Tree

> table.

...

>  /**
> + * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
> + * @fwnode:    Pointer to the firmware node
> + * @name:      IRQ name in interrupt-names property in fwnode
> + *
> + * Returns Linux IRQ number on success, errno otherwise.

negative errno

> + */
> +int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
> +{
> +       int index;

> +       if (unlikely(!name))

Don't use unlikely() here.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] Enable named interrupt smbus-alert for ACPI

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 4:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> 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.

Most of my comments are regarding spelling and documentation.
The code looks almost good enough. That said, if maintainers will be
okay, I'm sure the next version will be upstream-ready.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Wed, Jan 12, 2022 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Get interrupt by name from ACPI table as well.
>
> Add option to use 'interrupt-names' in _DSD which can map to interrupt by
> index. The implementation is similar to 'interrupt-names' in devicetree.
> Also add a common routine to get irq by name from devicetree and ACPI
> table.
>
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  drivers/base/property.c  | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  3 +++
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index cbe4fa2..414c316 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -920,6 +920,41 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
>  EXPORT_SYMBOL(fwnode_irq_get);
>
>  /**
> + * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
> + * @fwnode:    Pointer to the firmware node
> + * @name:      IRQ name in interrupt-names property in fwnode
> + *
> + * Returns Linux IRQ number on success, errno otherwise.
> + */
> +int fwnode_irq_get_byname(const 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);
> +
> +/**
> + * device_irq_get_byname - Get IRQ of a device using interrupt name
> + * @dev:       Device to get the interrupt
> + * @name:      IRQ name in interrupt-names property in fwnode

Which fwnode?

> + *
> + * Returns Linux IRQ number on success, 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);

This can be confusing, because it pretends to be super-generic and in
fact it depends on an fwnode to be there.

I guess I'd rather not have it at all, or use a more precise name for it.

> +
> +/**
>   * 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 16f736c..bc49350 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);
>
>  unsigned int device_get_child_node_count(struct device *dev);
>
> --
> 2.7.4
>

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

[Akhil R]

> On Wed, Jan 12, 2022 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Get interrupt by name from ACPI table as well.
> >
> > Add option to use 'interrupt-names' in _DSD which can map to interrupt
> > by index. The implementation is similar to 'interrupt-names' in devicetree.
> > Also add a common routine to get irq by name from devicetree and ACPI
> > table.
> >
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > ---
> >  drivers/base/property.c  | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  3 +++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c index
> > cbe4fa2..414c316 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -920,6 +920,41 @@ int fwnode_irq_get(const struct fwnode_handle
> > *fwnode, unsigned int index)  EXPORT_SYMBOL(fwnode_irq_get);
> >
> >  /**
> > + * fwnode_irq_get_byname - Get IRQ from a fwnode using its name
> > + * @fwnode:    Pointer to the firmware node
> > + * @name:      IRQ name in interrupt-names property in fwnode
> > + *
> > + * Returns Linux IRQ number on success, errno otherwise.
> > + */
> > +int fwnode_irq_get_byname(const 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);
> > +
> > +/**
> > + * device_irq_get_byname - Get IRQ of a device using interrupt name
> > + * @dev:       Device to get the interrupt
> > + * @name:      IRQ name in interrupt-names property in fwnode
> 
> Which fwnode?
> 
> > + *
> > + * Returns Linux IRQ number on success, 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);
> 
> This can be confusing, because it pretends to be super-generic and in fact it
> depends on an fwnode to be there.
> 
> I guess I'd rather not have it at all, or use a more precise name for it.
But, I suppose, the other device_*() functions also depend on the fwnode.
Wouldn't it make the naming inconsistent if we add a different one here?
Would it be better if I add more details in the description comment?

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

[Akhil R]

> On Wed, Jan 12, 2022 at 4:15 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Change of_*() functions to device_*() for firmware agnostic usage.
> > This allows to have smbus_alert interrupt without any changes
> 
> the smbus_alert
> 
> > in the controller drivers using ACPI table.
> 
> the ACPI
> 
> ...
> 
> This change reveals potential issue:
> 
> > -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> > +               irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert");
> 
> >                 if (irq <= 0)
> 
> I guess this '= 0' part should be fixed first.

'0' is a failure as per the documentation of of_irq_get_byname() as well as
of_irq_get(). The case is different for acpi_irq_get(), but it is handled in
fwnode_irq_get(). If I understood it right, a return value of '0' should be 
considered a failure here.

Thanks,
Akhil

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

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 11:48 AM Akhil R <akhilrajeev@nvidia.com> wrote:
> > On Wed, Jan 12, 2022 at 4:15 PM Akhil R <akhilrajeev@nvidia.com> wrote:

...

> > This change reveals potential issue:
> >
> > > -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> > > +               irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert");
> >
> > >                 if (irq <= 0)
> >
> > I guess this '= 0' part should be fixed first.
>
> '0' is a failure as per the documentation of of_irq_get_byname() as well as
> of_irq_get(). The case is different for acpi_irq_get(), but it is handled in
> fwnode_irq_get(). If I understood it right, a return value of '0' should be
> considered a failure here.

Depends. I have no idea what the original code does here. But
returning an error or 0 from this function seems confusing to me.


-- 
With Best Regards,
Andy Shevchenko

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

[Akhil R]

> ...
> 
> > > This change reveals potential issue:
> > >
> > > > -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> > > > +               irq = device_irq_get_byname(adapter->dev.parent,
> "smbus_alert");
> > >
> > > >                 if (irq <= 0)
> > >
> > > I guess this '= 0' part should be fixed first.
> >
> > '0' is a failure as per the documentation of of_irq_get_byname() as well as
> > of_irq_get(). The case is different for acpi_irq_get(), but it is handled in
> > fwnode_irq_get(). If I understood it right, a return value of '0' should be
> > considered a failure here.
> 
> Depends. I have no idea what the original code does here. But
> returning an error or 0 from this function seems confusing to me.
> 
The description in of_irq_get*() says - 
/* Return: Linux IRQ number on success, or 0 on the IRQ mapping failure, or
 * -EPROBE_DEFER if the IRQ domain is not yet created, or error code in case
 * of any other failure.
 */
As I see from the code of fwnode_irq_get(), which is used in this case, returns 
either the return value of of_irq_get() or error code from acpi_irq_get() when
it fails, or res.start if it didn't fail. I guess, any of these would not be 0 unless
there is an error.

Thanks,
Akhil

 

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

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 12:29 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> > ...
> >
> > > > This change reveals potential issue:
> > > >
> > > > > -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> > > > > +               irq = device_irq_get_byname(adapter->dev.parent,
> > "smbus_alert");
> > > >
> > > > >                 if (irq <= 0)
> > > >
> > > > I guess this '= 0' part should be fixed first.
> > >
> > > '0' is a failure as per the documentation of of_irq_get_byname() as well as
> > > of_irq_get(). The case is different for acpi_irq_get(), but it is handled in
> > > fwnode_irq_get(). If I understood it right, a return value of '0' should be
> > > considered a failure here.
> >
> > Depends. I have no idea what the original code does here. But
> > returning an error or 0 from this function seems confusing to me.
> >
> The description in of_irq_get*() says -
> /* Return: Linux IRQ number on success, or 0 on the IRQ mapping failure, or
>  * -EPROBE_DEFER if the IRQ domain is not yet created, or error code in case
>  * of any other failure.
>  */
> As I see from the code of fwnode_irq_get(), which is used in this case, returns
> either the return value of of_irq_get() or error code from acpi_irq_get() when
> it fails, or res.start if it didn't fail. I guess, any of these would not be 0 unless
> there is an error.

of_irq_get*() seems inconsistent...

Uwe, what do you think?

-- 
With Best Regards,
Andy Shevchenko

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

[Akhil R]

> > > ...
> > >
> > > > > This change reveals potential issue:
> > > > >
> > > > > > -               irq = of_irq_get_byname(adapter->dev.of_node,
> "smbus_alert");
> > > > > > +               irq =
> > > > > > + device_irq_get_byname(adapter->dev.parent,
> > > "smbus_alert");
> > > > >
> > > > > >                 if (irq <= 0)
> > > > >
> > > > > I guess this '= 0' part should be fixed first.
> > > >
> > > > '0' is a failure as per the documentation of of_irq_get_byname()
> > > > as well as of_irq_get(). The case is different for acpi_irq_get(),
> > > > but it is handled in fwnode_irq_get(). If I understood it right, a
> > > > return value of '0' should be considered a failure here.
> > >
> > > Depends. I have no idea what the original code does here. But
> > > returning an error or 0 from this function seems confusing to me.
> > >
> > The description in of_irq_get*() says -
> > /* Return: Linux IRQ number on success, or 0 on the IRQ mapping
> > failure, or
> >  * -EPROBE_DEFER if the IRQ domain is not yet created, or error code
> > in case
> >  * of any other failure.
> >  */
> > As I see from the code of fwnode_irq_get(), which is used in this
> > case, returns either the return value of of_irq_get() or error code
> > from acpi_irq_get() when it fails, or res.start if it didn't fail. I
> > guess, any of these would not be 0 unless there is an error.
> 
> of_irq_get*() seems inconsistent...
> 
> Uwe, what do you think?
> 
A bit tricky. You are right, as we don't often see a return value of '0' as
an error in Linux. But here since it is a number which is expected, it might
be reasonable to allot 0 to an error as well. Not sure of the exact rationale
in those functions though.

Thanks,
Akhil

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

On Thu, Jan 20, 2022 at 12:43:02PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 12:29 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > > ...
> > >
> > > > > This change reveals potential issue:
> > > > >
> > > > > > -               irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert");
> > > > > > +               irq = device_irq_get_byname(adapter->dev.parent,
> > > "smbus_alert");
> > > > >
> > > > > >                 if (irq <= 0)
> > > > >
> > > > > I guess this '= 0' part should be fixed first.
> > > >
> > > > '0' is a failure as per the documentation of of_irq_get_byname() as well as
> > > > of_irq_get(). The case is different for acpi_irq_get(), but it is handled in
> > > > fwnode_irq_get(). If I understood it right, a return value of '0' should be
> > > > considered a failure here.
> > >
> > > Depends. I have no idea what the original code does here. But
> > > returning an error or 0 from this function seems confusing to me.
> > >
> > The description in of_irq_get*() says -
> > /* Return: Linux IRQ number on success, or 0 on the IRQ mapping failure, or
> >  * -EPROBE_DEFER if the IRQ domain is not yet created, or error code in case
> >  * of any other failure.
> >  */
> > As I see from the code of fwnode_irq_get(), which is used in this case, returns
> > either the return value of of_irq_get() or error code from acpi_irq_get() when
> > it fails, or res.start if it didn't fail. I guess, any of these would not be 0 unless
> > there is an error.
> 
> of_irq_get*() seems inconsistent...
> 
> Uwe, what do you think?

Yeah, this is something I stumbled over during the platform_get_irq*()
discussion. But I don't feel like investing any more energy there.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]