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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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

Same here, please avoid polluting commit message with the Cc list.

...

> +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?)
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.

-- 
With Best Regards,
Andy Shevchenko