Re: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Nov 21 2025 at 13:14, Cosmin Tanislav wrote:
> +static inline int rzt2h_icu_irq_to_offset(struct irq_data *d, void __iomem **base,
> +					  unsigned int *offset)
> +{
> +	struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> +	unsigned int hwirq = irqd_to_hwirq(d);
> +
> +	if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_NS)) {
> +		*offset = hwirq - RZT2H_ICU_IRQ_NS_START;
> +		*base = priv->base_ns;
> +	} else if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_S) ||
> +		   /* SEI follows safety IRQs in registers and in IRQ numbers. */
> +		   RZT2H_ICU_IRQ_IN_RANGE(hwirq, SEI)) {

This nested commend in the condition is really unreadable.

> +		*offset = hwirq - RZT2H_ICU_IRQ_S_START;
> +		*base = priv->base_s;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzt2h_icu_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> +	unsigned int parent_type;
> +	unsigned int offset;

Combine same data types into one line please.

> +	void __iomem *base;
> +	u32 val, md;
> +	int ret;


> +	guard(raw_spinlock)(&priv->lock);
> +	val = readl_relaxed(base + RZT2H_ICU_PORTNF_MD);
> +	val &= ~RZT2H_ICU_PORTNF_MDi_MASK(offset);
> +	val |= RZT2H_ICU_PORTNF_MDi_PREP(offset, md);
> +	writel_relaxed(val, base + RZT2H_ICU_PORTNF_MD);
> +

This looks wrong. guard() holds the lock across the set_parent()
call. If you really need that then this needs a comment explaining the
why. Otherwise use scoped_guard().

> +	return irq_chip_set_type_parent(d, parent_type);
> +}
> +static const struct irq_chip rzt2h_icu_chip = {
> +	.name = "rzt2h-icu",
> +	.irq_mask = irq_chip_mask_parent,
> +	.irq_unmask = irq_chip_unmask_parent,
> +	.irq_eoi = irq_chip_eoi_parent,
> +	.irq_set_type = rzt2h_icu_set_type,
> +	.irq_set_wake = irq_chip_set_wake_parent,
> +	.irq_set_affinity = irq_chip_set_affinity_parent,
> +	.irq_retrigger = irq_chip_retrigger_hierarchy,
> +	.irq_get_irqchip_state = irq_chip_get_parent_state,
> +	.irq_set_irqchip_state = irq_chip_set_parent_state,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SET_TYPE_MASKED |
> +		 IRQCHIP_SKIP_SET_WAKE,

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

And please read and follow the rest of the documentation too.

> +};
> +
> +static int rzt2h_icu_alloc(struct irq_domain *domain, unsigned int virq,
> +			   unsigned int nr_irqs, void *arg)
> +{
> +	struct rzt2h_icu_priv *priv = domain->host_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int ret;
> +
> +	ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &rzt2h_icu_chip,
> +					    NULL);

Get rid of the line breaks all over the place. You have 100 characters.

> +	if (ret)
> +		return ret;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +					    &priv->fwspec[hwirq]);
> +}


> +static int rzt2h_icu_init(struct platform_device *pdev,
> +			  struct device_node *parent)
> +{
> +	struct irq_domain *irq_domain, *parent_domain;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct rzt2h_icu_priv *priv;
> +	int ret;
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain)
> +		return dev_err_probe(dev, -ENODEV, "cannot find parent domain\n");
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->base_ns = devm_of_iomap(dev, dev->of_node, 0, NULL);
> +	if (IS_ERR(priv->base_ns))
> +		return PTR_ERR(priv->base_ns);
> +
> +	priv->base_s = devm_of_iomap(dev, dev->of_node, 1, NULL);
> +	if (IS_ERR(priv->base_s))
> +		return PTR_ERR(priv->base_s);
> +
> +	ret = rzt2h_icu_parse_interrupts(priv, node);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "cannot parse interrupts: %d\n", ret);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "devm_pm_runtime_enable failed: %d\n", ret);
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "pm_runtime_resume_and_get failed: %d\n", ret);
> +
> +	raw_spin_lock_init(&priv->lock);
> +
> +	irq_domain = irq_domain_create_hierarchy(parent_domain, 0, RZT2H_ICU_NUM_IRQ,
> +						 dev_fwnode(dev),
> +						 &rzt2h_icu_domain_ops, priv);
> +	if (!irq_domain) {
> +		pm_runtime_put(dev);
> +		return -ENOMEM;
> +	}

The mix of 'return $ERR' and 'return dev_err_probe()' is confusing at best.

Thanks,

        tglx