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

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(-)

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
>

> 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

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.

> 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

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.