Re: [PATCH 2/4] irqchip: irq-eyeq5-msi: add EyeQ5 PCIe MSI controller driver

[Thomas Gleixner <tglx@kernel.org>]

On Wed, Oct 28 2026 at 16:58, Gregory CLEMENT wrote:
> +/*
> + * The caller must hold msi->lock.

Instead of a non-enforcable comment can you please put a
lockdep_assert_held() into the code.

> + */
> +static void __eyeq_msi_mask_irq(struct eyeq_msi *msi, unsigned int bit)
> +{
> +	u16 value;
> +
> +	value = readw(msi->gcsr_base + GCSR_IRQMASK);
> +	value &= ~FIELD_PREP(GCSR_IRQMASK_MASK, BIT(bit));
> +	writew(value, msi->gcsr_base + GCSR_IRQMASK);
> +}

> +static struct irq_chip eyeq_msi_irq_chip = {
> +	.name			= "EyeQ5-MSI",
> +	.irq_mask		= eyeq_msi_mask_irq,
> +	.irq_unmask		= eyeq_msi_unmask_irq,
> +	.irq_compose_msi_msg	= eyeq_msi_compose_msg,
> +};
> +
> +static void eyeq_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *parent_chip = irq_desc_get_chip(desc);
> +	struct eyeq_msi *msi = irq_desc_get_handler_data(desc);
> +	void __iomem *status_reg;
> +	unsigned long pending;
> +	unsigned int bit;
> +	int ret;
> +
> +	chained_irq_enter(parent_chip, desc);
> +
> +	status_reg = msi->gcsr_base + GCSR_IRQSTATUS;
> +
> +	while (true) {
> +		pending = FIELD_GET(GCSR_IRQSTATUS_MASK,
> +				    readw(status_reg));
> +		if (!pending)
> +			break;
> +
> +		/*
> +		 * Write-one-to-clear status register.
> +		 */
> +		writew(FIELD_PREP(GCSR_IRQSTATUS_MASK, pending),
> +		       status_reg);
> +
> +		for_each_set_bit(bit, &pending, msi->num_vectors) {
> +			ret = generic_handle_domain_irq(msi->msi_domain, bit);

You want to look at generic_handle_demux_domain_irq() and related
functionality so you can actually provide a irq_set_affinity() callback
for these demultiplexed interrupts. 

> +			if (ret)
> +				pr_err_ratelimited("unmapped MSI interrupt %u\n",
> +						   bit);

No line break required. You have 100 characters. Otherwise this needs
curly brackets around the "two-liner".

> +		}
> +	}
> +
> +	chained_irq_exit(parent_chip, desc);
> +}
> +
> +static int eyeq_msi_domain_alloc(struct irq_domain *domain,
> +				 unsigned int virq,
> +				 unsigned int nr_irqs,
> +				 void *args)

100 characters... All over the place

> +{
> +	struct eyeq_msi *msi = domain->host_data;
> +	DECLARE_BITMAP(reserved, EYEQ_MSI_MAX_VECTORS);
> +	unsigned long bit = 0;
> +	unsigned int i;
> +
> +	bitmap_zero(reserved, EYEQ_MSI_MAX_VECTORS);
> +
> +	scoped_guard(raw_spinlock_irqsave, &msi->lock) {
> +		for (i = 0; i < nr_irqs; i++) {
> +			bit = find_next_zero_bit(msi->used, msi->num_vectors, bit);
> +			if (bit >= msi->num_vectors) {
> +				/*
> +				 * Roll back this call's allocations: on error the
> +				 * core does not call .free().
> +				 */
> +				for_each_set_bit(bit, reserved,
> +						 EYEQ_MSI_MAX_VECTORS) {
> +					clear_bit(bit, msi->used);
> +					__eyeq_msi_mask_irq(msi, bit);
> +				}
> +
> +				return -ENOSPC;
> +			}
> +
> +			set_bit(bit, msi->used);
> +			__eyeq_msi_unmask_irq(msi, bit);
> +			set_bit(bit, reserved);
> +			bit++;
> +		}
> +	}
> +
> +	i = 0;
> +	for_each_set_bit(bit, reserved, EYEQ_MSI_MAX_VECTORS)

  for....() {

> +		irq_domain_set_info(domain, virq + i++, bit,
> +				    &eyeq_msi_irq_chip, msi,
> +				    handle_simple_irq, NULL, NULL);
> +

  }

See documentation.

> +	return 0;
> +}
> +
> +static void eyeq_msi_domain_free(struct irq_domain *domain,
> +				 unsigned int virq,
> +				 unsigned int nr_irqs)

....

> +{
> +	struct irq_data *data;
> +	struct eyeq_msi *msi;
> +	irq_hw_number_t hwirq;

Reverse fir tree ordering please. See documentation. And please move
these declarations into the scope where they are used.

> +	unsigned int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {

  for (unsigned int i = 0; ...)

> +		data = irq_domain_get_irq_data(domain, virq + i);
> +		if (WARN_ON_ONCE(!data))
> +			continue;
> +


> +static int eyeq_msi_parse_dt(struct eyeq_msi *msi,
> +			     struct device_node *node)
> +{
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "num-vectors",
> +				   &msi->num_vectors);

        int ret = of....();

> +	if (ret) {
> +		pr_err("missing or invalid num-vectors property\n");
> +		return ret;
> +	}
> +

Thanks,

        tglx