From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 39C9637416C for ; Fri, 20 Mar 2026 09:51:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774000292; cv=none; b=pRI9rbZovONRvCa6gsBWtfTQGuKmAkMeufLpVNPGY+POQgxQi7zwWMmiflUk0BEQKNP4p0uSd4qIL8skBuksIHRumxe4BE2cMEGTeN8Wq5zx2EKIU9HiUTRR/gf+/+Bs9ieWF8T/wRPRY66/QiygB6CALf+FWonUcs//BwtAIYs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774000292; c=relaxed/simple; bh=gloz+vj4OM5k6cGlVremWffdd+Ypm5maMAJnIHGDHDs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=muhFtGYXleHQc8jEmYkVLi9lj2dnx7QLci5lVtefPTsKOgQsrWwGLzafKrkP/YG0XElxQfwjs74rKVf7S6MPdtEpfiw60TyP979DcnROm05niiyzl6ImstbN7ktlzew7LJyg4RJ4ZPrta14bQ/PvEtcaLaLX+aZDx8Vx79CAR9k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=I13HyS/8; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="I13HyS/8" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 81B1AC2BC9E; Fri, 20 Mar 2026 09:51:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1774000292; bh=gloz+vj4OM5k6cGlVremWffdd+Ypm5maMAJnIHGDHDs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=I13HyS/8zT/1U8YDgiiutMbBybrEqUYHVJ2Cg/oMutSUJUSkX4jQhw79qJt4qGMnD DQqJuFRBMOtz3GBRDJCHEfr1MmZgiQA5/iuIGOTJtpTAodBzZ5LFxKC0Ixwa+r7xFH +OI6VPAvh5A0nZHFNs6pOOD8eOwPgH4cgeljIvyxWNs+zHWcaZT9eeYM1yzfTJ8KpZ eeTKwBzAnXDqvey6PVdkpdhvVEJ067zeZz9xGmrDkg/RSiijXMPFbnXPnz+3hSEDRQ maSrsb8pKVEXZ8P6TSX2NCDO3iXpIVYdOJivF41ZE+9XaxQCI9pA844l0EWuyPO+DO 8rSzjJgK5o0hg== From: Thomas Gleixner To: j0sh1x Cc: linux-kernel@vger.kernel.org, subsystem@vger.kernel.org, dominikkobinski314@gmail.com, j0sh1x Subject: Re: [PATCH 2/5] irqchip: Add Qualcomm MSM VIC driver In-Reply-To: <20260315111705.118544-2-aljoshua.hell@gmail.com> References: <20260315111705.118544-1-aljoshua.hell@gmail.com> <20260315111705.118544-2-aljoshua.hell@gmail.com> Date: Fri, 20 Mar 2026 10:51:28 +0100 Message-ID: <871pheltgf.ffs@tglx> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Please order the includes alphabetically and remove all unneeded ones. There are quite some pointless ones. > +#include > +#include > + > +#include > + > +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