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

[Gregory CLEMENT]

Hello Thomas,

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

OK
>
>> + */
>> +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.

Thanks for the pointer! Indeed, once I used
generic_handle_demux_domain_irq(), it was possible to use affinity, I
managed to test it on the hardware.


>
>> +			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".
>

I took into account this remarks and all the other ones about style and
formatting.

Thanks,

Gregory


-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com