Re: [PATCH v2 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY

On Sat, Jan 07, 2023 at 07:23:59PM +0100, Andrew Lunn wrote:

quoted

+++ b/drivers/net/phy/Kconfig

@@ -264,6 +264,13 @@ config NATIONAL_PHY
 	help
 	  Currently supports the DP83865 PHY.
 
+config NCN26000_PHY
+	tristate "onsemi 10BASE-T1S Ethernet PHY"
+	help
+	  Adds support for the onsemi 10BASE-T1S Ethernet PHY.
+	  Currently supports the NCN26000 10BASE-T1S Industrial PHY
+	  with MII interface.
+
 config NXP_C45_TJA11XX_PHY
 	tristate "NXP C45 TJA11XX PHYs"

These are actually sorted by the tristate string, which is what you
see when you use

make menuconfig

So 'onsemi' should be after 'NXP TJA11xx PHYs support'. Also, all the
other entries capitalise the first word.

As for the order I fixed it. Thanks for noticing.

Regarding the capitalization, I have a little problem here. 'onsemi' is a
brand and according to company rules it MUST be written all in
lowercase. I know we're not obliged to follow any company directive here, but 
as wierd as it might sound, I'd rather keep it lowercase just not to get 
comments later on trying to fix this, if you agree...

quoted

 	depends on PTP_1588_CLOCK_OPTIONAL

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f7138d3c896b..b5138066ba04 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile

@@ -77,6 +77,7 @@ obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
 obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
+obj-$(CONFIG_NCN26000_PHY)	+= ncn26000.o
 obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o

This is sorted by CONFIG_ symbol, so is correct.

quoted

+
+// driver callbacks --------------------------------------------------------- //

Comments like this don't really add any value.

Sure, I can remove it.

quoted

+static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
+{
+	int ret;
+
+	// read and aknowledge the IRQ status register
+	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+	// check only link status changes
+	if (unlikely(ret < 0) || (ret & NCN26000_REG_IRQ_STATUS) == 0)
+		return IRQ_NONE;

More usage of unlikely(). If this was on the hot path, handling 10M
frames a second, then maybe unlikley() could be justified. But how
often do you get PHY interrupts? Once a day?

Right, it is my instinct to use unlikely on any sanity check which is
effectively unlikely to happen. But I understand it is not needed here.

      Andrew