Re: [RFC PATCH v2 11/17] irqchip: Add driver for the RISC-V RPMI system MSI service group

From: Thomas Gleixner <tglx@linutronix.de>
To: "Anup Patel" <apatel@ventanamicro.com>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Jassi Brar" <jassisinghbrar@gmail.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <brgl@bgdev.pl>,
	"Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Len Brown <lenb@kernel.org>, Sunil V L <sunilvl@ventanamicro.com>,
	Rahul Pathak <rpathak@ventanamicro.com>,
	Leyfoon Tan <leyfoon.tan@starfivetech.com>,
	Atish Patra <atishp@atishpatra.org>,
	Andrew Jones <ajones@ventanamicro.com>,
	Samuel Holland <samuel.holland@sifive.com>,
	Anup Patel <anup@brainfault.org>,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	Anup Patel <apatel@ventanamicro.com>
Subject: Re: [RFC PATCH v2 11/17] irqchip: Add driver for the RISC-V RPMI system MSI service group
Date: Mon, 03 Feb 2025 14:50:25 +0100	[thread overview]
Message-ID: <871pwfp272.ffs@tglx> (raw)
In-Reply-To: <20250203084906.681418-12-apatel@ventanamicro.com>

On Mon, Feb 03 2025 at 14:19, Anup Patel wrote:
> +
> +struct rpmi_sysmsi_priv {
> +	struct device *dev;
> +	struct mbox_client client;
> +	struct mbox_chan *chan;
> +	u32 nr_irqs;
> +	u32 gsi_base;
> +};

AS requested before please use tabular layout for structs:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +static int rpmi_sysmsi_set_msi_state(struct rpmi_sysmsi_priv *priv,
> +				     u32 sys_msi_index, u32 sys_msi_state)
> +{
> +	struct rpmi_sysmsi_set_msi_state_tx tx;
> +	struct rpmi_sysmsi_set_msi_state_rx rx;
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	tx.sys_msi_index = sys_msi_index;
> +	tx.sys_msi_state = sys_msi_state;
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_SYSMSI_SRV_SET_MSI_STATE,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +	ret = rpmi_mbox_send_message(priv->chan, &msg);
> +	if (ret)
> +		return ret;
> +	if (rx.status)
> +		return rpmi_to_linux_error(rx.status);
> +
> +	return 0;
> +}
> +
> +#define rpmi_sysmsi_mask(__priv, __msi_index)		\
> +	rpmi_sysmsi_set_msi_state(__priv, __msi_index, 0)
> +#define rpmi_sysmsi_unmask(__priv, __msi_index)		\
> +	rpmi_sysmsi_set_msi_state(__priv, __msi_index, RPMI_SYSMSI_MSI_STATE_ENABLE)

These macros are not really providing any value.

> +static void rpmi_sysmsi_irq_mask(struct irq_data *d)
> +{
> +	struct rpmi_sysmsi_priv *priv = irq_data_get_irq_chip_data(d);
> +	int ret;
> +
> +	ret = rpmi_sysmsi_mask(priv, d->hwirq);
> +	if (ret)
> +		dev_warn(priv->dev, "Failed to mask hwirq %d (error %d)\n",
> +			 (u32)d->hwirq, ret);

        if (ret) {
        	....
        }

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

> +	irq_chip_mask_parent(d);
> +}

Other than those nits, this looks reasonable.

Thanks,

        tglx