Re: [PATCH v13 01/13] irqchip/sifive-plic: Convert PLIC driver into a platform driver

From: Thomas Gleixner <tglx@linutronix.de>
To: Anup Patel <apatel@ventanamicro.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Conor Dooley <conor+dt@kernel.org>
Cc: "Anup Patel" <apatel@ventanamicro.com>,
	devicetree@vger.kernel.org,
	"Saravana Kannan" <saravanak@google.com>,
	"Marc Zyngier" <maz@kernel.org>,
	"Anup Patel" <anup@brainfault.org>,
	linux-kernel@vger.kernel.org, "Björn Töpel" <bjorn@kernel.org>,
	"Atish Patra" <atishp@atishpatra.org>,
	linux-riscv@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	"Andrew Jones" <ajones@ventanamicro.com>
Subject: Re: [PATCH v13 01/13] irqchip/sifive-plic: Convert PLIC driver into a platform driver
Date: Tue, 20 Feb 2024 11:09:47 +0100	[thread overview]
Message-ID: <87il2jih04.ffs@tglx> (raw)
In-Reply-To: <20240220060718.823229-2-apatel@ventanamicro.com>

On Tue, Feb 20 2024 at 11:37, Anup Patel wrote:
> The PLIC driver does not require very early initialization so let
> us convert it into a platform driver.

s/let us convert/convert/

Please us passive voice and imperative mood all over the changelogs. No
we/us, let....

> As part of the conversion, the PLIC probing undergoes the following
> changes:
> 1. Use dev_info(), dev_err() and dev_warn() instead of pr_info(),
>    pr_err() and pr_warn()
> 2. Use devm_xyz() APIs wherever applicable
> 3. PLIC is now probed after CPUs are brought-up so we have to
>    setup cpuhp state after context handler of all online CPUs
>    are initialized otherwise we see crash on multi-socket systems

This patch is really doing too many things at once, which makes it hard
to review. Can you split this into digestable pieces please?

>  		if (unlikely(err))
> -			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
> +			dev_warn_ratelimited(handler->priv->dev,
> +					"can't find mapping for hwirq %lu\n",
>  					hwirq);

Nit. Please use brackets around the condition. See:

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

for reasoning.

Thanks,

        tglx

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv