Received-SPF: pass (google.com: domain of linux-kernel+bounces-36751-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <31bbe5787708b7de770cab96a9072fe6db39347b.camel@pengutronix.de>
Subject: Re: [PATCH v1 2/2] irqchip: Add StarFive external interrupt
 controller
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Changhuang Liang <changhuang.liang@starfivetech.com>, Thomas Gleixner
 <tglx@linutronix.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>
Cc: Ley Foon Tan <leyfoon.tan@starfivetech.com>, Jack Zhu
	 <jack.zhu@starfivetech.com>, linux-kernel@vger.kernel.org, 
	devicetree@vger.kernel.org
Date: Wed, 24 Jan 2024 10:53:53 +0100
In-Reply-To: <20240111023201.6187-3-changhuang.liang@starfivetech.com>
References: <20240111023201.6187-1-changhuang.liang@starfivetech.com>
	 <20240111023201.6187-3-changhuang.liang@starfivetech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4-2 
Precedence: bulk
MIME-Version: 1.0

On Mi, 2024-01-10 at 18:32 -0800, Changhuang Liang wrote:
[...]
> diff --git a/drivers/irqchip/irq-starfive-jh8100-intc.c b/drivers/irqchip=
/irq-starfive-jh8100-intc.c
> new file mode 100644
> index 000000000000..be8224111cf2
> --- /dev/null
> +++ b/drivers/irqchip/irq-starfive-jh8100-intc.c
> @@ -0,0 +1,180 @@
[...]
> +struct starfive_irq_chip {
> +	void __iomem *base;
> +	struct irq_domain *root_domain;
> +	struct clk *clk;
> +	struct reset_control *rst;

Since rst is never used outside of starfive_intc_init, there is no need
to store it here.

[...]
> +static int __init starfive_intc_init(struct device_node *intc,
> +				     struct device_node *parent)
> +{
> +	struct starfive_irq_chip *irqc;
> +	int ret;
> +	int parent_irq;
> +
> +	irqc =3D kzalloc(sizeof(*irqc), GFP_KERNEL);
> +	if (!irqc)
> +		return -ENOMEM;
> +
> +	irqc->base =3D of_iomap(intc, 0);
> +	if (!irqc->base) {
> +		pr_err("Unable to map IC registers\n");
> +		ret =3D -ENXIO;
> +		goto err_free;
> +	}
> +
> +	irqc->rst =3D of_reset_control_get_by_index(intc, 0);

Please use of_reset_control_get_exclusive(intc, NULL) instead.

> +	if (IS_ERR(irqc->rst)) {
> +		pr_err("Unable to get reset control\n");
> +		ret =3D PTR_ERR(irqc->rst);

Consider printing the error code via %pe.


regards
Philipp