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

[Lorenzo Pieralisi]

On Fri, Sep 25, 2026 at 01:54:26PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 25, 2026 at 12:30:07PM +0200, Lorenzo Pieralisi wrote:
> > On Fri, Sep 25, 2026 at 12:49:54PM +0300, Andy Shevchenko wrote:
> > > On Fri, Sep 25, 2026 at 09:48:01AM +0200, Lorenzo Pieralisi wrote:
> 
> ...
> 
> > > > +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];
> > > 
> > > Use standard pattern, id est
> > > 
> > > 	if (ret)
> > > 		...
> > > 
> > > > +	kfree(values);
> > > 
> > > You want to use __free()
> > > 
> > > > +	return ret;
> > > > +}
> > > 
> > > I believe the whole approach is suboptimal, if you wish get indexed value (but why?)
> > 
> > Why what (that's what the irq_get() interface requires ?) I agree it is
> > suboptimal - the whole point of the series is an RFC on using properties
> > to store GSI number/flags, then how to read them we will optimize it
> > when/if we agree that's the approach to be taken.
> 
> Why to have indexed APIs. The callers usually do not want a single item from an
> array, they want all of them or a big pile (exception is the array of strings,
> but we have matching functions for that).
> 
> As per approach, this patch is against device property and I don't think we are
> going to agree on the approach taken in *this* patch.
> 
> > > it needs to be retrieved as that in the guts of ACPI. Allocating memory for the whole
> > > array to retrieve a single element is simply wrong.
> 
> ...
> 
> > > > +#define ACPI_IRQ_PROP_GSI		"linux,acpi-gsi"
> > > > +#define ACPI_IRQ_PROP_GSI_TRIGGER	"linux,acpi-gsi-trigger"
> > > > +#define ACPI_IRQ_PROP_GSI_POLARITY	"linux,acpi-gsi-polarity"
> > > 
> > > Oh... This sounds like a big ugly hack.
> > 
> > That does not help much I am afraid.
> > 
> > What's a ugly hack ? Property names ? Using properties for this purpose ?
> 
> Yes, properties started with "linux," for some core functionality.

I added a name for those out of thin air, again - the question is on
the approch, properties I can call them whatever we like.

I am not a fan of those myself, as mentioned in the cover letter.

> Yes, using properties for this also doesn't sound right. I think the problem
> here is in the table specifications or somewhere that deep. That's why we
> ended up in this series.
> 
> > Again, it is an RFC for this specific reason and I mentioned that in the
> > cover letter, thank you for your inputs.
> 
> And here we have a discussion started :-)

Thanks !

> P.S. Looks like I was too quick to jump into this thread. I will wait for
> others to share their view on all this.

No, thank you for chiming in so promptly. The main aim is to keep most
of the drivers and API unchanged for a bunch of devices created out of
static tables - ie minimize changes as much as possible, that's the
purpose.

Thanks,
Lorenzo