Re: [PATCH 2/4] irqchip: irq-eyeq5-msi: add EyeQ5 PCIe MSI controller driver
From: Thomas Gleixner
Date: Wed Oct 28 2026 - 17:21:31 EST
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