[PATCH 0/2] Enable named interrupt smbus-alert for ACPI as well

[PATCH 0/2] Enable named interrupt smbus-alert for ACPI as well

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

Akhil R (2):
  device property: Add device_irq_get_byname
  i2c: smbus: Use device_ functions instead of of_

 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 +++
 6 files changed, 48 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/2] 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..7acf4fc 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
+ * @index:	IRQ name
+ *
+ * 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
+ * @index:	IRQ name
+ *
+ * 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

Re: [PATCH 1/2] device property: Add device_irq_get_byname

[Andy Shevchenko]

On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Get interrupt by name from ACPI table as well.

the interrupt resource

> 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..7acf4fc 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
> + * @index:     IRQ name
> + *

Needs a description to explain how the name is described.

> + * 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;

This property ise needs to be described in the ACPI documentation:
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html

Perhaps after the DMA section.

> +       return fwnode_irq_get(fwnode, index);
> +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] device property: Add device_irq_get_byname

[Akhil R]

> On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Get interrupt by name from ACPI table as well.
> 
> the interrupt resource
> 
> > 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..7acf4fc 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
> > + * @index:     IRQ name
> > + *
> 
> Needs a description to explain how the name is described.
> 
> > + * 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;
> 
> This property ise needs to be described in the ACPI documentation:
> https://www.kernel.org/doc/html/latest/firmware-
> guide/acpi/enumeration.html
> 
> Perhaps after the DMA section.
Do you mean to document the complete interrupt usage in ACPI
including getting interrupt by index or only the named interrupt part?

Also please share if anything on the discussion we had previously.

Thanks,
Akhil

[PATCH 2/2] 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 2/2] i2c: smbus: Use device_ functions instead of of_

[Andy Shevchenko]

On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Change of_ functions to device_ for firmware agnostic usage.

of_*()
device_*()

> This allows to have smbus_alert interrupt without any changes
> in the controller drivers using ACPI table.

...

> -       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");

Hmm... Adapter device node is not the same as the node for its parent.
Do you have some code that propagates of_node from parent to child?

I.o.w. I would expect to see

       irq = device_property_match_string(&adapter->dev, "interrupt-names",

here.

>         if (irq == -EINVAL || irq == -ENODATA)
>                 return 0;
>         else if (irq < 0)

TBH the entire code smells. "Interesting" way of getting an optional
named interrupt.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 2/2] i2c: smbus: Use device_ functions instead of of_

[Akhil R]

> On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Change of_ functions to device_ for firmware agnostic usage.
> 
> of_*()
> device_*()
> 
> > This allows to have smbus_alert interrupt without any changes in the
> > controller drivers using ACPI table.
> 
> ...
> 
> > -       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");
> 
> Hmm... Adapter device node is not the same as the node for its parent.
> Do you have some code that propagates of_node from parent to child?
Adapter device does not have an of_node unless the adapter driver
sets it, I guess. I see all the adapter drivers add the of_node and 
parent for adapter. Also, there are many places in i2c-core-base and 
i2c-core-acpi where adapter->dev.parent is referred to as the adapter 
driver device.

Basically, adapter->dev.parent and adapter->dev.of_node would 
ultimately refer to the same device (or the of_node of that device), 
as far as I understand.
> 
> I.o.w. I would expect to see
> 
>        irq = device_property_match_string(&adapter->dev, "interrupt-names",
> 
> here.
It would then require adding the fw_node as well from the adapter driver.
I felt it made more sense to refer adapter->dev.parent here as most of the
(or rather all of the) adapter drivers already sets it.
> 
> >         if (irq == -EINVAL || irq == -ENODATA)
> >                 return 0;
> >         else if (irq < 0)
> 
> TBH the entire code smells. "Interesting" way of getting an optional named
> interrupt.
I felt it useful to have it this way as it would remain agnostic to device tree and 
the ACPI. We would not have to add redundant codes in adapter drivers that
are using ACPI table.

Named interrupts for the ACPI as well, I feel would be a useful addition that can
prove to be of value more than this change; I believe.

Thanks,
Akhil

Re: [PATCH 2/2] i2c: smbus: Use device_ functions instead of of_

[Andy Shevchenko]

On Thu, Dec 16, 2021 at 6:08 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> > On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:

...

> > > -       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");
> >
> > Hmm... Adapter device node is not the same as the node for its parent.
> > Do you have some code that propagates of_node from parent to child?
> Adapter device does not have an of_node unless the adapter driver
> sets it, I guess. I see all the adapter drivers add the of_node and
> parent for adapter. Also, there are many places in i2c-core-base and
> i2c-core-acpi where adapter->dev.parent is referred to as the adapter
> driver device.
>
> Basically, adapter->dev.parent and adapter->dev.of_node would
> ultimately refer to the same device (or the of_node of that device),
> as far as I understand.
> >
> > I.o.w. I would expect to see
> >
> >        irq = device_property_match_string(&adapter->dev, "interrupt-names",
> >
> > here.
> It would then require adding the fw_node as well from the adapter driver.
> I felt it made more sense to refer adapter->dev.parent here as most of the
> (or rather all of the) adapter drivers already sets it.

Is this
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1047
what you are looking for?

...

> > >         if (irq == -EINVAL || irq == -ENODATA)
> > >                 return 0;
> > >         else if (irq < 0)
> >
> > TBH the entire code smells. "Interesting" way of getting an optional named
> > interrupt.
> I felt it useful to have it this way as it would remain agnostic to device tree and
> the ACPI. We would not have to add redundant codes in adapter drivers that
> are using ACPI table.
>
> Named interrupts for the ACPI as well, I feel would be a useful addition that can
> prove to be of value more than this change; I believe.

Me too. My comment was about current state of affairs, and not to your change.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 2/2] i2c: smbus: Use device_ functions instead of of_

[Akhil R]

> On Thu, Dec 16, 2021 at 6:08 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> > > On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> 
> ...
> 
> > > > -       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");
> > >
> > > Hmm... Adapter device node is not the same as the node for its parent.
> > > Do you have some code that propagates of_node from parent to child?
> > Adapter device does not have an of_node unless the adapter driver sets
> > it, I guess. I see all the adapter drivers add the of_node and parent
> > for adapter. Also, there are many places in i2c-core-base and
> > i2c-core-acpi where adapter->dev.parent is referred to as the adapter
> > driver device.
> >
> > Basically, adapter->dev.parent and adapter->dev.of_node would
> > ultimately refer to the same device (or the of_node of that device),
> > as far as I understand.
> > >
> > > I.o.w. I would expect to see
> > >
> > >        irq = device_property_match_string(&adapter->dev,
> > > "interrupt-names",
> > >
> > > here.
> > It would then require adding the fw_node as well from the adapter driver.
> > I felt it made more sense to refer adapter->dev.parent here as most of
> > the (or rather all of the) adapter drivers already sets it.
> 
> Is this
> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1047
>
> what you are looking for?
This, I suppose, is for the i2c client driver.
I meant the individual adapter drivers.
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-tegra.c#L1786
similar is there in all drivers.
If to use adapter->dev for interrupt-names, I assume, it would require to add

	adapter->dev.fwnode = i2c_dev->dev->fwnode;

in all drivers (or at least in the drivers which does not use devicetree).
I thought it would be decent to use adapter->dev.parent as all the drivers already
set it.

> 
> ...
> 
> > > >         if (irq == -EINVAL || irq == -ENODATA)
> > > >                 return 0;
> > > >         else if (irq < 0)
> > >
> > > TBH the entire code smells. "Interesting" way of getting an optional
> > > named interrupt.
> > I felt it useful to have it this way as it would remain agnostic to
> > device tree and the ACPI. We would not have to add redundant codes in
> > adapter drivers that are using ACPI table.
> >
> > Named interrupts for the ACPI as well, I feel would be a useful
> > addition that can prove to be of value more than this change; I believe.
> 
> Me too. My comment was about current state of affairs, and not to your
> change.
Got it :)

Thanks,
Akhil