RE: [PATCH net-next] lan78xx: Use irq_domain for phy interrupt from USB Int. EP

quoted

+static void lan78xx_irq_mask(struct irq_data *irqd)
+{
+	struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+	struct lan78xx_net *dev =
+			container_of(data, struct lan78xx_net, domain_data);
+	u32 buf;
+
+	lan78xx_read_reg(dev, INT_EP_CTL, &buf);

lan78xx_read_reg() uses kmalloc() with GFP_KERNEL, while
irq_mask/irq_unmask can be called in atomic context AFAIR, you may need
to pass down a specific gfp_t to lan78xx_read_reg.

What about usb_submit_urb(), can that work in atomic context? Do you
need to have lan78xx_read_reg() acquire a raw spinlock or something to
serialize them?

quoted

+	buf &= ~INT_EP_PHY_INT_EN_;

Even though you may have only one interrupt line to deal with at the
moment, better make this bit derived from irqd->hwirq instead of hard
coding it here.

quoted

+	lan78xx_write_reg(dev, INT_EP_CTL, buf);
+}
+
+static void lan78xx_irq_unmask(struct irq_data *irqd)
+{
+	struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+	struct lan78xx_net *dev =
+			container_of(data, struct lan78xx_net, domain_data);
+	u32 buf;
+
+	lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+	buf |= INT_EP_PHY_INT_EN_;

Same here, this should come from irqd->hwirq.

quoted

+	lan78xx_write_reg(dev, INT_EP_CTL, buf);
+}
+
+static struct irq_chip lan78xx_irqchip = {
+	.name		= "lan78xx-phyirq",
+	.irq_mask	= lan78xx_irq_mask,
+	.irq_unmask	= lan78xx_irq_unmask,
+};
+
+static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
+{
+	struct device_node *of_node;
+	struct irq_domain *irqdomain;
+	unsigned int irq_base = 0;
+	int ret = 0;
+
+	of_node = dev->udev->dev.parent->of_node;
+
+	dev->domain_data.irqchip = &lan78xx_irqchip;
+	dev->domain_data.irq_handler = handle_simple_irq;
+
+	irqdomain = irq_domain_add_simple(of_node, 1, 0,

&chip_domain_ops,

quoted

+					  &dev->domain_data);

Is there really just one interrupt associated with this peripheral here?

quoted

+	if (lan78xx_setup_irq_domain(dev) < 0) {
+		netdev_warn(dev->net, "lan78xx_setup_irq_domain()

failed");

quoted

+		return -EIO;
+	}

Any reason not to propagate the error code from
lan78xx_setup_irq_domain() here?

Thanks for prompt review.
I'll look into it and submit update.

Thanks.
- Woojung