On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:

Interrupts are required for TX time stamping. Probably they could also
be used for PHY connection status.

Do the KSZ switches have an internal PHY? And there's a single interrupt
line, shared between the PTP timestamping engine, and the internal PHY
that is driver by phylib?

This patch only adds the basic infrastructure for interrupts, no
interrupts are actually enabled nor handled.

ksz9477_reset_switch() must be called before requesting the IRQ (in
ksz9477_init() instead of ksz9477_setup()).

A patch can never be "too simple". Maybe you could factor out that code
movement into a separate patch.

Please keep local variable declaration sorted in the reverse order of
line length. But....

+
+	/* Read global port interrupt status register */
+	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
+	if (ret)
+		return result;

...Is there any point at all in keeping the "result" variable?

+
+	for (port = 0; port < dev->port_cnt; port++) {
+		if (data & BIT(port)) {

You can reduce the indentation level by 1 here using:

		if (!(data & BIT(port)))
			continue;

+			u8 data8;
+
+			/* Read port interrupt status register */
+			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
+					&data8);
+			if (ret)
+				return result;
+
+			/* ToDo: Add specific handling of port interrupts */

Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
this commit exactly, will emit interrupts and complain that nobody cared.

+		}
+	}
+
+	return result;
+}
+
+static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Enable port interrupts (0 means enabled) */
+	data &= ~((1 << dev->port_cnt) - 1);

And what's the " - 1" for?

+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;

	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);

+}
+
+static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Disable port interrupts (1 means disabled) */
+	data |= ((1 << dev->port_cnt) - 1);
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;

same comments as above.

Also, it's almost as if you want to implement these in the same
function, with a "bool enable"?

What is irqd_get_trigger_type and what does it have to do with the
"irqflags" argument of request_threaded_irq? Where else have you even
seen this?

+
+		irqflags |= IRQF_ONESHOT;

And shared maybe?

+		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
+						ksz9477_switch_irq_thread,
+						irqflags,
+						dev_name(dev->dev),
+						dev);
+		if (ret) {
+			dev_err(dev->dev, "failed to request IRQ.\n");
+			return ret;
+		}
+
+		ret = ksz9477_enable_port_interrupts(dev);
+		if (ret)
+			return ret;

Could you also clear pending interrupts before enabling the line?

+	}

 	return 0;
 }

 static void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (dev->irq > 0)
+		ksz9477_disable_port_interrupts(dev);

I think it'd look a bit nicer if you moved this condition into
ksz9477_disable_port_interrupts:

	if (!dev->irq)
		return;

 	ksz9477_reset_switch(dev);
 }