Re: [PATCH v4 13/23] irqchip: Add driver for the RPMI system MSI service group

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, May 25, 2025 at 02:17:00PM +0530, Anup Patel wrote:
> The RPMI specification defines a system MSI service group which
> allows application processors to receive MSIs upon system events
> such as graceful shutdown/reboot request, CPU hotplug event, memory
> hotplug event, etc.
> 
> Add an irqchip driver for the RISC-V RPMI system MSI service group
> to directly receive system MSIs in Linux kernel.

...

> +#include <linux/device.h>

Perhaps I missed something, but devm_kzalloc() is in device/devres.h. Do you
need it for something else?

> +#include <linux/dev_printk.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/riscv-rpmi-message.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>

> +#include <vdso/bits.h>

Just make it linux/bits.h as vdso is for user space libvdso and related.

But

+ asm/byteorder.h

...

> +static void rpmi_sysmsi_irq_mask(struct irq_data *d)
> +{
> +	struct rpmi_sysmsi_priv *priv = irq_data_get_irq_chip_data(d);

Declare temporary variable for hwirq and do irqd_to_hwirq() only once.

> +	int ret;
> +
> +	ret = rpmi_sysmsi_set_msi_state(priv, irqd_to_hwirq(d), 0);
> +	if (ret) {
> +		dev_warn(priv->dev, "Failed to mask hwirq %lu (error %d)\n",
> +			 irqd_to_hwirq(d), ret);
> +	}
> +	irq_chip_mask_parent(d);
> +}

...

> +static void rpmi_sysmsi_irq_unmask(struct irq_data *d)

Ditto.

...

> +static void rpmi_sysmsi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +{
> +	arg->desc = desc;
> +	arg->hwirq = (u32)desc->data.icookie.value;

Hmm... Why do you need an explicit casting?

> +}

...

> +	if (WARN_ON(fwspec->param_count < 1))

+ bug.h

> +		return -EINVAL;

+ errno.h (but actually you need err.h due to PTR_ERR() et al.)

...

> +static int rpmi_sysmsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rpmi_sysmsi_priv *priv;
> +	int rc;

Be consistent with variable naming for the same (semantically) stuff.

> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->dev = dev;

> +	platform_set_drvdata(pdev, priv);

How is being used?

> +
> +	/* Setup mailbox client */
> +	priv->client.dev		= priv->dev;
> +	priv->client.rx_callback	= NULL;
> +	priv->client.tx_block		= false;
> +	priv->client.knows_txdone	= true;
> +	priv->client.tx_tout		= 0;
> +
> +	/* Request mailbox channel */
> +	priv->chan = mbox_request_channel(&priv->client, 0);
> +	if (IS_ERR(priv->chan))
> +		return PTR_ERR(priv->chan);
> +
> +	/* Get number of system MSIs */
> +	rc = rpmi_sysmsi_get_num_msi(priv);
> +	if (rc < 1) {
> +		mbox_free_channel(priv->chan);
> +		if (rc)
> +			return dev_err_probe(dev, rc, "Failed to get number of system MSIs\n");
> +		else
> +			return dev_err_probe(dev, -ENODEV, "No system MSIs found\n");
> +	}
> +	priv->nr_irqs = rc;
> +
> +	/* Set the device MSI domain if not available */
> +	if (!dev_get_msi_domain(dev)) {
> +		/*
> +		 * The device MSI domain for OF devices is only set at the
> +		 * time of populating/creating OF device. If the device MSI
> +		 * domain is discovered later after the OF device is created
> +		 * then we need to set it explicitly before using any platform
> +		 * MSI functions.
> +		 */

> +		if (is_of_node(dev_fwnode(dev)))
> +			of_msi_configure(dev, to_of_node(dev_fwnode(dev)));

		if (dev_of_node(dev))
			of_msi_configure(dev, dev_of_node(dev));

> +		if (!dev_get_msi_domain(dev)) {
> +			mbox_free_channel(priv->chan);
> +			return -EPROBE_DEFER;
> +		}
> +	}
> +
> +	if (!msi_create_device_irq_domain(dev, MSI_DEFAULT_DOMAIN,
> +					  &rpmi_sysmsi_template,
> +					  priv->nr_irqs, priv, priv)) {
> +		mbox_free_channel(priv->chan);
> +		return dev_err_probe(dev, -ENOMEM, "failed to create MSI irq domain\n");
> +	}
> +
> +	dev_info(dev, "%u system MSIs registered\n", priv->nr_irqs);
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko