Re: [PATCH 2/5] irqchip: Add Qualcomm MSM VIC driver

[Thomas Gleixner <tglx@kernel.org>]

On Sun, Mar 15 2026 at 12:17, j0sh1x wrote:

That's a lot of void in this change log. And you have to provide a real
name.

> +++ b/drivers/irqchip/irq-msm-vic.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2007 Google, Inc.
> + * Copyright (c) 2009, Code Aurora Forum. All rights reserved.
> + * Copyright (c) 2024, Htc Leo Revival Project
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

You have the SPDX license identifier, so no need for the boiler plate.

https://www.kernel.org/doc/Documentation/process/license-rules.rst

> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/interrupt.h>
> +#include <linux/ptrace.h>
> +#include <linux/timer.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/cacheflush.h>

Please order the includes alphabetically and remove all unneeded
ones. There are quite some pointless ones.

> +#include <asm/exception.h>
> +#include <asm/irq.h>
> +
> +#include <dt-bindings/interrupt-controller/qcom-vic.h>
> +
> +static u32 msm_irq_smsm_wake_enable[2];
> +struct msm_irq_shadow {

Please put a new line between the variable and the structure definition.

> +	u32 int_en[2];
> +	u32 int_type;
> +	u32 int_polarity;
> +	u32 int_select;
> +};
> +
> +static struct msm_irq_shadow *msm_irq_shadow_reg;
> +static u32 *msm_irq_idle_disable;
> +
> +struct msm_irq_map {
> +	u32 irq;
> +	u8 smsm;
> +};
> +
> +struct vic_device {
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	u32				msm_nr_irqs;
> +	u32				nr_vic_irqs;
> +	u32				nr_gpio_irqs;
> +	u32				vic_num_regs;
> +	struct msm_irq_map *irq_map;
> +	int irq_map_count;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

Please read and follow it.

> +};
> +
> +static struct vic_device vic_data;

Please have the struct declarations first and then define all the static
variables.

> +static int msm_irq_to_smsm(u32 irq)
> +{
> +	int i;
> +
> +	if (!vic_data.irq_map)
> +		return -EINVAL;
> +
> +	for (i = 0; i < vic_data.irq_map_count; i++) {

  for (int i = 0;

> +		if (vic_data.irq_map[i].irq == irq)
> +			return vic_data.irq_map[i].smsm;
> +		}
> +
> +	return -ENOENT;
> +}
> +
> +void set_irq_flags(unsigned int irq, unsigned int iflags);

What?

> +void set_irq_flags(unsigned int irq, unsigned int iflags)
> +{
> +	unsigned long clr = 0, set = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> +
> +	if (irq >= vic_data.msm_nr_irqs) {
> +		pr_err("Trying to set irq flags for IRQ%d\n", irq);
> +		return;
> +	}
> +
> +	if (iflags & IRQF_VALID)
> +		clr |= IRQ_NOREQUEST;
> +	if (iflags & IRQF_PROBE)
> +		clr |= IRQ_NOPROBE;
> +	if (!(iflags & IRQF_NOAUTOEN))
> +		clr |= IRQ_NOAUTOEN;
> +	/* Order is clear bits in "clr" then set bits in "set" */
> +	irq_modify_status(irq, clr, set & ~clr);
> +}
> +EXPORT_SYMBOL_GPL(set_irq_flags);

Not going to happen. There is existing infrastructure for that and there
is nothing VIC specific about this. set_irq_flags() has been removed
from the kernel long ago for a reason.

> +static inline void msm_irq_write_all_regs(void __iomem *base, unsigned int val)
> +{
> +	for (int i = 0; i < vic_data.vic_num_regs; i++)
> +		writel(val, base + (i * 4));
> +}
> +
> +static void msm_irq_ack(struct irq_data *d)
> +{
> +	void __iomem *reg = VIC_INT_TO_REG_ADDR(vic_data.base + VIC_INT_CLEAR0, d->irq);
> +
> +	writel(1 << (d->irq & 31), reg);
> +}
> +
> +static void msm_irq_mask(struct irq_data *d)
> +{
> +	void __iomem *reg = VIC_INT_TO_REG_ADDR(vic_data.base + VIC_INT_ENCLEAR0, d->irq);
> +	unsigned int index = VIC_INT_TO_REG_INDEX(d->irq);
> +	u32 mask = 1UL << (d->irq & 31);
> +	int smsm_irq = msm_irq_to_smsm(d->irq);

Variable declaration ordering.

> +
> +	msm_irq_shadow_reg[index].int_en[0] &= ~mask;
> +	writel(mask, reg);
> +	if (smsm_irq == 0) {
> +		msm_irq_idle_disable[index] &= ~mask;
> +	} else {
> +		mask = 1UL << (smsm_irq - 1);
> +		msm_irq_smsm_wake_enable[0] &= ~mask;
> +	}
> +}
> +static inline void msm_vic_handle_irq(void __iomem *base_addr, struct pt_regs
> +		*regs)

Bogus line break

> +{
> +	u32 irqnr;
> +
> +	do {
> +		/* VIC_IRQ_VEC_RD has irq# or old irq# if the irq has been handled

Bogus comment style.

> +		 * VIC_IRQ_VEC_PEND_RD has irq# or -1 if none pending *but* if you
> +		 * just read VIC_IRQ_VEC_PEND_RD you never get the first irq for some reason
> +		 */
> +		irqnr = readl_relaxed(base_addr + VIC_IRQ_VEC_RD);
> +		irqnr = readl_relaxed(base_addr + VIC_IRQ_VEC_PEND_RD);
> +		if (irqnr == -1)

-1 on a u32? That's just sloppy. Use a proper define like:

#define NO_PENDING_IRQ	(~0U)

		if (irqnr == NO_PENDING_IRQ)


> +			break;
> +		handle_IRQ(irqnr, regs);
> +	} while (1);
> +}
> +
> +/* enable imprecise aborts */
> +static inline void local_cpsie_enable(void)
> +{
> +	asm volatile("cpsie a" : : : "memory");
> +}
> +
> +static void __exception_irq_entry vic_handle_irq(struct pt_regs *regs)
> +{
> +	local_cpsie_enable();// local_abt_enable()?

No tail comments please.

> +	msm_vic_handle_irq(vic_data.base, regs);
> +}
> +
> +static struct irq_chip msm_irq_chip = {
> +	.name          = "msm",
> +	.irq_disable   = msm_irq_mask,
> +	.irq_ack       = msm_irq_ack,
> +	.irq_mask      = msm_irq_mask,
> +	.irq_unmask    = msm_irq_unmask,
> +	.irq_set_wake  = msm_irq_set_wake,
> +	.irq_set_type  = msm_irq_set_type,
> +};
> +
> +static int msm_vic_parse_irq_mapping(struct device_node *np, struct msm_irq_map **map, int *count)
> +{
> +	const __be32 *prop;
> +	int len, i;
> +
> +	prop = of_get_property(np, "irq-mapping", &len);
> +	if (!prop) {
> +		pr_err("%s: No irq-mapping property in device tree\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +    /* Each entry is 2 u32 values: irq + smsm */
> +	*count = len / (2 * sizeof(u32));
> +
> +	*map = kzalloc((*count) * sizeof(**map), GFP_KERNEL);
> +	if (!*map) {
> +		pr_err("%s: Failed to allocate memory for irq_map\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < *count; i++) {
> +		(*map)[i].irq = be32_to_cpu(prop[i * 2]);
> +		(*map)[i].smsm = be32_to_cpu(prop[i * 2 + 1]);
> +	}

Why is anything in the device tree big endian on a little endian system?

Also this code is unreadable and incomprehensible. 

> +
> +	return 0;
> +}
> +
> +static int __init msm_init_irq(struct device_node *intc, struct device_node *parent)
> +{
> +	int ret;
> +
> +	vic_data.base = of_iomap(intc, 0);
> +	if (WARN_ON(!vic_data.base))
> +		return -EIO;
> +	ret = of_property_read_u32(intc, "num-irqs", &vic_data.nr_vic_irqs);
> +	if (ret) {
> +		pr_err("%s: failed to read num-irqs ret=%d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(intc, "num-gpio-irqs", &vic_data.nr_gpio_irqs);
> +	if (ret) {
> +		pr_err("%s: failed to read num-gpio-irqs ret=%d\n", __func__, ret);
> +		return ret;
> +	}

Read all the required information into local variables first and then
allocate the vic_device data including the mapping table in one go.

> +	msm_irq_shadow_reg = kcalloc(vic_data.vic_num_regs,
> +				     sizeof(*msm_irq_shadow_reg),
> +					 GFP_KERNEL);

You have 100 characters.

> +	msm_irq_idle_disable = kcalloc(vic_data.vic_num_regs,
> +				       sizeof(*msm_irq_idle_disable),
> +				       GFP_KERNEL);
> +
> +	if (!msm_irq_shadow_reg || !msm_irq_idle_disable)
> +		return -ENOMEM;

That leaks the previous allocations.

> +	vic_data.msm_nr_irqs = vic_data.nr_vic_irqs * 2 + vic_data.nr_gpio_irqs;
> +
> +	ret = irq_alloc_descs(-1, 0, vic_data.nr_vic_irqs, 0);

No. That's not how any of this works. A interrupt driver has no business
to allocate interrupt descriptors. Use proper interrupt domains and not
this cobbled together gunk collected from some out of tree garbage can.

There are enough recent examples of properly written interrupt chip
drivers in drivers/irqchip to learn from.

> +IRQCHIP_DECLARE(qcom_msm_vic, "qcom,msm-vic", msm_init_irq);
> \ No newline at end of file

Git gave you a hint right there....

Thanks,

        tglx