Re: [PATCH RFC 02/11] ACPI: Introduce irq_get() for static fwnodes

[Ashok Raj <ashok.raj@oss.qualcomm.com>]

On Tue, Sep 29, 2026 at 10:40:59AM +0200, Lorenzo Pieralisi wrote:
> On Mon, Sep 28, 2026 at 04:24:18PM -0700, Ashok Raj wrote:
> > 
> > On Fri, Sep 25, 2026 at 09:48:01AM +0200, Lorenzo Pieralisi wrote:
> > > To describe and map GSIs for firmware nodes created out of ACPI static
> > > table entries in a uniform way it is required to define some standard
> > > properties and attach them to ACPI static fwnode as secondary nodes.
> > > 
> > > Define properties names to describe GSIs and their trigger-mode/polarity,
> > > and implement an irq_get() callback for static fwnodes so that core code
> > > can retrieve and map IRQs for ACPI static fwnodes in standard manner.
> > > 
> > > An empty stub for property_read_string_array() is also added, so that
> > > the fwnode_irq_get_byname() interface falls back (through
> > > fwnode_property_read_string_array()) to the secondary
> > > fwnode to grab the "interrupt-names" property.
> > > 
> > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > Cc: Bartosz Golaszewski <brgl@kernel.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > ---
> > >  drivers/acpi/property.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  include/linux/acpi.h    |  4 +++
> > >  2 files changed, 72 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index 8ee5a1f0eb48..c609100c08db 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -1766,7 +1766,74 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
> > >  
> > >  DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);
> > >  DECLARE_ACPI_FWNODE_OPS(acpi_data_fwnode_ops);
> > > -const struct fwnode_operations acpi_static_fwnode_ops;
> > > +
> > > +static int acpi_static_fwnode_read_u32_prop_index(const struct fwnode_handle *fwnode,
> > > +						  const char *propname,
> > > +						  unsigned int index, u32 *value)
> > > +{
> > > +	u32 *values;
> > > +	int ret, count;
> > > +
> > > +	count = fwnode_property_count_u32(fwnode, propname);
> > > +	if (count < 0)
> > > +		return count;
> > > +
> > > +	if (index >= count)
> > > +		return -ENOENT;
> > > +
> > > +	values = kcalloc(count, sizeof(*values), GFP_KERNEL);
> > > +	if (!values)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = fwnode_property_read_u32_array(fwnode, propname, values, count);
> > > +	if (!ret)
> > > +		*value = values[index];
> > > +
> > 
> > You lookup with a propname, and then qualify with and index? is it
> > possible to have the different index but same propname? 
> 
> What is the question :) ? It is to retrieve a property value at a specific
> index.

Gah... I'm wondering what was my question as well :-)

See below: acpi_static_fwnode_irq_get() calls this read_string_array() multiple
times from within the same function.. 

Sorry for the terse message, apologies.
> 
> > Alternately you can send the list to caller and they can use the ones
> > they need?
> > 
> > 
> > > +	kfree(values);
> > > +	return ret;
> > > +}
> > > +
> > > +static int acpi_static_fwnode_read_string_array(const struct fwnode_handle *fwnode,
> > > +						const char *propname,
> > > +						const char **val, size_t nval)
> > > +{
> > > +	/* Route string handling to secondary software nodes */
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int acpi_static_fwnode_irq_get(const struct fwnode_handle *fwnode,
> > > +				      unsigned int index)
> > > +{
> > > +	u32 gsi, trigger, polarity;
> > > +	int ret;
> > > +
> > > +	if (!fwnode->secondary)
> > > +		return -ENODEV;
> > > +
> > > +	fwnode = fwnode->secondary;
> > > +
> > > +	ret = acpi_static_fwnode_read_u32_prop_index(fwnode, ACPI_IRQ_PROP_GSI,
> > > +						index, &gsi);
> > > +	if (ret)
> > > +		return ret == -ENOENT ? -ENXIO : ret;
> > > +
> > > +	ret = acpi_static_fwnode_read_u32_prop_index(fwnode, ACPI_IRQ_PROP_GSI_TRIGGER,
> > > +						index, &trigger);
> > > +	if (ret)
> > > +		return ret == -ENOENT ? -ENXIO : ret;
> > > +
> > > +	ret = acpi_static_fwnode_read_u32_prop_index(fwnode, ACPI_IRQ_PROP_GSI_POLARITY,
> > > +						index, &polarity);

For each of the above calls for read_u32_prop_index() the allocation,
copy a value and does free it. 

Instead you could read the whole array, and just get each value and
discard it once? 

> > > +	if (ret)
> > > +		return ret == -ENOENT ? -ENXIO : ret;
> > 
> > Consolidate return to one place? 
> 
> Yes it can (and trigger and polarity can be just one property "flags", FWIW),
> if we agree that's what will do, which I doubt.
> 
> Thanks,
> Lorenzo

-- 
/ashok.raj
ashok.raj@oss.qualcomm.com
Qualcomm Inc