On Tue, Dec 05 2023 at 18:45, Yoshinori Sato wrote:

+static void endisable_irq(struct irq_data *data, int enable)

bool enable?

+{
+	struct sh7751_intc_priv *priv;
+	unsigned int irq;
+
+	priv = irq_data_to_priv(data);
+
+	irq = irqd_to_hwirq(data);
+	if (!is_valid_irq(irq)) {
+		/* IRQ out of range */
+		pr_warn_once("%s: IRQ %u is out of range\n", __FILE__, irq);
+		return;
+	}
+
+	if (irq <= MAX_IRL && !priv->irlm)
+		/* IRL encoded external interrupt */
+		/* disable for SR.IMASK */
+		update_sr_imask(irq - IRQ_START, enable);
+	else
+		/* Internal peripheral interrupt */
+		/* mask for IPR priority 0 */
+		update_ipr(priv, irq, enable);

Lacks curly brackets on the if/else

+static int irq_sh7751_map(struct irq_domain *h, unsigned int virq,
+			  irq_hw_number_t hw_irq_num)
+{
+	irq_set_chip_and_handler(virq, &sh7751_irq_chip, handle_level_irq);
+	irq_get_irq_data(virq)->chip_data = h->host_data;
+	irq_modify_status(virq, IRQ_NOREQUEST, IRQ_NOPROBE);
+	return 0;
+}
+static const struct irq_domain_ops irq_ops = {

Newline before 'static ...'

+	.map    = irq_sh7751_map,
+	.xlate  = irq_domain_xlate_onecell,
+};
+
+static int __init load_ipr_map(struct device_node *intc,
+			       struct sh7751_intc_priv *priv)
+{
+	struct property *ipr_map;
+	unsigned int num_ipr, i;
+	struct ipr *ipr;
+	const __be32 *p;
+	u32 irq;
+
+	ipr_map = of_find_property(intc, "renesas,ipr-map", &num_ipr);
+	if (IS_ERR(ipr_map))
+		return PTR_ERR(ipr_map);
+	num_ipr /= sizeof(u32);
+	/* 3words per entry. */
+	if (num_ipr % 3)

Three words per ... But you can spare the comment by doing:

        if (num_ipr % WORDS_PER_ENTRY)

+		goto error1;
+	num_ipr /= 3;
+static int __init sh7751_intc_of_init(struct device_node *intc,
+				      struct device_node *parent)
+{
+	struct sh7751_intc_priv *priv;
+	void __iomem *base, *base2;
+	struct irq_domain *domain;
+	u16 icr;
+	int ret;
+
+	base = of_iomap(intc, 0);
+	base2 = of_iomap(intc, 1);
+	if (!base || !base2) {
+		pr_err("%pOFP: Invalid register definition\n", intc);

What unmaps 'base' if 'base' is valid and base2 == NULL?

+		return -EINVAL;
+	}
+
+	priv = kzalloc(sizeof(struct sh7751_intc_priv), GFP_KERNEL);
+	if (priv == NULL)
+		return -ENOMEM;

Leaks base[2] maps, no?

+	ret = load_ipr_map(intc, priv);
+	if (ret < 0) {
+		kfree(priv);
+		return ret;
+	}
+
+	priv->base = base;
+	priv->intpri00 = base2;
+
+	if (of_property_read_bool(intc, "renesas,irlm")) {
+		priv->irlm = true;
+		icr = __raw_readw(priv->base + R_ICR);
+		icr |= ICR_IRLM;
+		__raw_writew(icr, priv->base + R_ICR);
+	}
+
+	domain = irq_domain_add_linear(intc, NR_IRQS, &irq_ops, priv);
+	if (domain == NULL) {
+		pr_err("%pOFP: cannot initialize irq domain\n", intc);
+		kfree(priv);
+		return -ENOMEM;
+	}
+
+	irq_set_default_host(domain);
+	pr_info("%pOFP: SH7751 Interrupt controller (%s external IRQ)",
+		intc, priv->irlm ? "4 lines" : "15 level");
+	return 0;
+}
+
+IRQCHIP_DECLARE(sh_7751_intc,
+		"renesas,sh7751-intc", sh7751_intc_of_init);

One line please.

Thanks,

        tglx