Re: [PATCH 01/25] irqdomain: Add firmware info reporting interface

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Sep 08 2025 at 17:31, Marc Zyngier wrote:
> +/**
> + * struct irq_fwspec_info - firmware provided IRQ information structure
> + *
> + * @fwspec:		IRQ specifier

s/IRQ/interrupt/g

> + * @cpumask:		Affinity mask for this interrupt
> + * @flags:		Information validity flags
> + *
> + * This structure reports firmware-specific information about an
> + * interrupt. The only significant information is the affinity of a
> + * per-CPU interrupt, but this is designed to be extended as required.
> + */
> +struct irq_fwspec_info {
> +	struct irq_fwspec	fwspec;
> +	const struct cpumask	*affinity;
> +
> +#define IRQ_FWSPEC_INFO_FWSPEC_VALID	BIT(0)
> +#define IRQ_FWSPEC_INFO_AFFINITY_VALID	BIT(1)
> +
> +	unsigned long		flags;

Can you please not stick the defines into the struct. That makes my eyes
bleed.

Also the ordering looks strange as flags should be before the fields it
validates for readability sake, no?

> +};
> +
>  /* Conversion function from of_phandle_args fields to fwspec  */
>  void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
>  			       unsigned int count, struct irq_fwspec *fwspec);
> @@ -69,6 +90,8 @@ void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
>   * @translate:	Given @fwspec, decode the hardware irq number (@out_hwirq) and
>   *		linux irq type value (@out_type). This is a generalised @xlate
>   *		(over struct irq_fwspec) and is preferred if provided.
> + * @get_info: Given @info, report additional firmware-provided information.
> + *            Optional.

get_info() is pretty generic. Can we have some more descriptive name for
that? Also please keep the tabular formatting of the doc intact.

>   * @debug_show:	For domains to show specific data for an interrupt in debugfs.
>   *
>   * Functions below are provided by the driver and called whenever a new mapping
> @@ -96,6 +119,7 @@ struct irq_domain_ops {
>  	void	(*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
>  	int	(*translate)(struct irq_domain *d, struct irq_fwspec *fwspec,
>  			     unsigned long *out_hwirq, unsigned int *out_type);
> +	int (*get_info)(struct irq_fwspec_info *info);

TAB between int and '(*'

>  #endif
>  #ifdef CONFIG_GENERIC_IRQ_DEBUGFS
>  	void	(*debug_show)(struct seq_file *m, struct irq_domain *d,
> @@ -602,6 +626,8 @@ void irq_domain_free_irqs_parent(struct irq_domain *domain, unsigned int irq_bas
>  
>  int irq_domain_disconnect_hierarchy(struct irq_domain *domain, unsigned int virq);
>  
> +int irq_populate_fwspec_info(struct irq_fwspec_info *info);
> +
> +
> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY

Stray TAB

> +int irq_populate_fwspec_info(struct irq_fwspec_info *info)
> +{
> +	struct irq_domain *domain;
> +
> +	domain = fwspec_to_domain(&info->fwspec);

Just move that to the declaration line

> +	if (!domain || !domain->ops->get_info) {
> +		info->flags = 0;
> +		return 0;
> +	}
> +
> +	return domain->ops->get_info(info);
> +}
> +#endif
> +
> +unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> +{
> +	struct irq_domain *domain;
> +	struct irq_data *irq_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;

I know you moved it from the original function, but can you please
reorder it in reverse fir tree order?

> +	int virq;
> +
> +	domain = fwspec_to_domain(fwspec);
>  	if (!domain) {
>  		pr_warn("no irq domain found for %s !\n",
>  			of_node_full_name(to_of_node(fwspec->fwnode)));

Thanks,

        tglx