microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs

drivers/net/phy/microchipT1phy.c new file +31 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/microchipT1phy.h>
+
+#define DRIVER_AUTHOR	"Nisar Sayed <[email protected]>"
+#define DRIVER_DESC	"Microchip LAN87XX T1 PHY driver"
+
+struct lan87xx_priv {
+	int	chip_id;
+	int	chip_rev;
+};
+
+static int lan87xx_phy_config_intr(struct phy_device *phydev)
+{
+	int rc = 0;

drivers/net/phy/microchipT1phy.c new file +13 -0

+
+	return rc < 0 ? rc : 0;
+}
+
+static int lan87xx_phy_ack_interrupt(struct phy_device *phydev)
+{
+	int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
+
+	return rc < 0 ? rc : 0;
+}
+
+static int lan87xx_probe(struct phy_device *phydev)
+{

drivers/net/phy/microchipT1phy.c new file +12 -0

+
+	phydev->priv = priv;
+
+	return 0;
+}
+
+static void lan87xx_remove(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct lan87xx_priv *priv = phydev->priv;
+
+	if (priv)

drivers/net/phy/microchipT1phy.c new file +28 -0

+		.flags          = PHY_HAS_INTERRUPT,
+
+		.probe          = lan87xx_probe,
+		.remove         = lan87xx_remove,
+
+		.config_init    = genphy_config_init,
+		.config_aneg    = genphy_config_aneg,
+
+		.ack_interrupt  = lan87xx_phy_ack_interrupt,
+		.config_intr    = lan87xx_phy_config_intr,
+
+		.suspend        = genphy_suspend,
+		.resume         = genphy_resume,
+	}
+};
+
+module_phy_driver(microchip_T1_phy_driver);
+
+static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
+	{ 0x0007c150, 0xfffffff0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");

include/linux/microchipT1phy.h new file +0 -0

new file mode 100644
index 0000000..7353e7c

include/linux/microchipT1phy.h new file +14 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */

include/linux/microchipT1phy.h new file +2 -0

+
+#endif /* _MICROCHIPT1PHY_H_ */

Quoted 7 lines

Add driver for Microchip LAN87XX T1 PHYs

This patch support driver for Microchp T1 PHYs.
There will be followup patches to this driver to support T1 PHY features such as cable diagnostics, signal quality indicator(SQI), sleep and wakeup (TC10) support.

Quoted 5 lines

Signed-off-by Nisar Sayed <[email protected]>

---
 drivers/net/phy/Kconfig          |   5 ++
 drivers/net/phy/Makefile         |   1 +
 drivers/net/phy/microchipT1phy.c | 116

+++++++++++++++++++++++++++++++++++++++

drivers/net/phy/Makefile obj-$(CONFIG_MESON_GXL_PHY) += meson-

Quoted 26 lines

 include/linux/microchipT1phy.h   |  28 ++++++++++
 4 files changed, 150 insertions(+)
 create mode 100644 drivers/net/phy/microchipT1phy.c  create mode

100644 include/linux/microchipT1phy.h

drivers/net/phy/Kconfig index renamed from drivers/net/phy/Kconfig +0 -0

bdfbabb..7b0b351 100644

drivers/net/phy/Kconfig +5 -0

@@ -354,6 +354,11 @@ config MICROCHIP_PHY
 	help
 	  Supports the LAN88XX PHYs.
 
+config MICROCHIP_T1_PHY
+	tristate "Microchip T1 PHYs"
+	---help---
+	  Supports the LAN87XX PHYs.
+
 config MICROSEMI_PHY
 	tristate "Microsemi PHYs"
 	---help---

drivers/net/phy/Makefile index renamed from drivers/net/phy/Makefile +0 -0

01acbcb..5706613 100644

drivers/net/phy/Makefile +0 -0

@@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY)	+= meson-

gxl.o

drivers/net/phy/microchipT1phy.c

Quoted 13 lines

drivers/net/phy/Makefile +1 -0

 obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
+obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchipT1phy.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o

drivers/net/phy/microchipT1phy.c new file +0 -0

new file mode 100644
index 0000000..15fbb04

Can we avoid CamelCase file names? Any reasons why this is not part of microchip.c?

drivers/net/phy/microchipT1phy.c line 2

Quoted 3 lines

drivers/net/phy/microchipT1phy.c new file +2 -0

@@ -0,0 +1,116 @@
+/*
+ * Copyright (C) 2018 Microchip Technology

Please use a SPDX license tag.

Quoted 24 lines

drivers/net/phy/microchipT1phy.c new file +24 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/microchipT1phy.h>
+
+#define DRIVER_AUTHOR	"Nisar Sayed <[email protected]>"
+#define DRIVER_DESC	"Microchip LAN87XX T1 PHY driver"
+
+struct lan87xx_priv {

drivers/net/phy/microchipT1phy.c line 39

Quoted 10 lines

drivers/net/phy/microchipT1phy.c new file +10 -0

+	int	chip_id;
+	int	chip_rev;
+};
+
+static int lan87xx_phy_config_intr(struct phy_device *phydev) {
+	int rc = 0;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* unmask all source and clear them before enable */
+		rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,

0x7FFF);

drivers/net/phy/microchipT1phy.c

Quoted 3 lines

drivers/net/phy/microchipT1phy.c new file +3 -0

+		rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
+		rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
+			       LAN87XX_MASK_LINK_UP |

LAN87XX_MASK_LINK_DOWN);

drivers/net/phy/microchipT1phy.c line 43

Quoted 3 lines

drivers/net/phy/microchipT1phy.c new file +3 -0

+	} else {
+		rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
+	}

Since the last thing you do in both branches is write to LAN87XX_INTERRUPT_MASK, just move that write outside of the branches and make sure the value you will set is correct in both cases.

Quoted 11 lines

drivers/net/phy/microchipT1phy.c new file +11 -0

+
+	return rc < 0 ? rc : 0;
+}
+
+static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) {
+	int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
+
+	return rc < 0 ? rc : 0;
+}
+
+static int lan87xx_probe(struct phy_device *phydev) {

drivers/net/phy/microchipT1phy.c line 66

Quoted 10 lines

drivers/net/phy/microchipT1phy.c new file +10 -0

+	struct device *dev = &phydev->mdio.dev;
+	struct lan87xx_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* these values can be used to identify internal PHY */
+	priv->chip_id = phy_read(phydev, MII_PHYSID1);
+	priv->chip_rev = phy_read(phydev, MII_PHYSID2);

This is absolutely not necessary, phydev->phy_id stores that information and you can use that at any point in your driver's lifetime. Besides, the revision is not the full MII_PHYSID2, it's only the lower 4 bits of it.

Your probe() and remove() function then become unnecessary.

Quoted 17 lines

drivers/net/phy/microchipT1phy.c new file +17 -0

+
+	phydev->priv = priv;
+
+	return 0;
+}
+
+static void lan87xx_remove(struct phy_device *phydev) {
+	struct device *dev = &phydev->mdio.dev;
+	struct lan87xx_priv *priv = phydev->priv;
+
+	if (priv)
+		devm_kfree(dev, priv);
+}
+
+static struct phy_driver microchip_T1_phy_driver[] = {
+	{
+		.phy_id         = 0x0007c150,

drivers/net/phy/microchipT1phy.c line 88

Quoted 4 lines

drivers/net/phy/microchipT1phy.c new file +4 -0

+		.phy_id_mask    = 0xfffffff0,
+		.name           = "Microchip LAN87xx",
+
+		.features       = SUPPORTED_100baseT_Full,

PHY_BASIC_FEATURES maybe? You cannot possibly be supporting just 100BaseT full duplex, 100BaseT half duplex, 10BaseT full and half duplex are also supported, right?

Quoted 30 lines

drivers/net/phy/microchipT1phy.c new file +28 -0

+		.flags          = PHY_HAS_INTERRUPT,
+
+		.probe          = lan87xx_probe,
+		.remove         = lan87xx_remove,
+
+		.config_init    = genphy_config_init,
+		.config_aneg    = genphy_config_aneg,
+
+		.ack_interrupt  = lan87xx_phy_ack_interrupt,
+		.config_intr    = lan87xx_phy_config_intr,
+
+		.suspend        = genphy_suspend,
+		.resume         = genphy_resume,
+	}
+};
+
+module_phy_driver(microchip_T1_phy_driver);
+
+static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
+	{ 0x0007c150, 0xfffffff0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");

(quoted diff) +0 -0

b/include/linux/microchipT1phy.h new file mode 100644 index

include/linux/microchipT1phy.h line 2

Quoted 6 lines

0000000..7353e7c

include/linux/microchipT1phy.h new file +2 -0

@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2018 Microchip Technology

Same comment as before, please use SPDX license identifiers.

Quoted 19 lines

include/linux/microchipT1phy.h new file +19 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _MICROCHIPT1PHY_H_
+#define _MICROCHIPT1PHY_H_
+
+/* Interrupt Source Register */
+#define LAN87XX_INTERRUPT_SOURCE                (0x18)

include/linux/microchipT1phy.h line 26

Quoted 5 lines

include/linux/microchipT1phy.h new file +5 -0

+
+/* Interrupt Mask Register */
+#define LAN87XX_INTERRUPT_MASK                  (0x19)
+#define LAN87XX_MASK_LINK_UP                    (0x0004)
+#define LAN87XX_MASK_LINK_DOWN                  (0x0002)

What's the point of that header file if all definitions are consumed by the same driver?

Fine, will change the filename.

The reason for moving to separate file is that we have a series of T1 standard PHYs, which support cable diagnostics, signal quality indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to keep all T1 standard PHYs separate to support additional features supported by these PHYs.

Quoted 6 lines

Quoted 6 lines

+ */

include/linux/microchipT1phy.h new file +5 -0

+#ifndef _MICROCHIPT1PHY_H_
+#define _MICROCHIPT1PHY_H_
+
+/* Interrupt Source Register */
+#define LAN87XX_INTERRUPT_SOURCE                (0x18)

Quoted 9 lines

include/linux/microchipT1phy.h line 26

Quoted 5 lines

include/linux/microchipT1phy.h new file +5 -0

+
+/* Interrupt Mask Register */
+#define LAN87XX_INTERRUPT_MASK                  (0x19)
+#define LAN87XX_MASK_LINK_UP                    (0x0004)
+#define LAN87XX_MASK_LINK_DOWN                  (0x0002)

What's the point of that header file if all definitions are consumed by the same driver?

We have planned a series of patches where we planned to use this further.

Quoted 7 lines

Fine, will change the filename.

The reason for moving to separate file is that we have a series of T1 standard PHYs, which support cable diagnostics, signal quality indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to keep all T1 standard PHYs separate to support additional features supported by these PHYs.

Is there anything shared with the other microchip PHYs? If there is potential for code sharing, you should do it.

Quoted 17 lines

Quoted 16 lines

include/linux/microchipT1phy.h line 26

Quoted 12 lines

+ */

include/linux/microchipT1phy.h new file +10 -0

+#ifndef _MICROCHIPT1PHY_H_
+#define _MICROCHIPT1PHY_H_
+
+/* Interrupt Source Register */
+#define LAN87XX_INTERRUPT_SOURCE                (0x18)
+
+/* Interrupt Mask Register */
+#define LAN87XX_INTERRUPT_MASK                  (0x19)
+#define LAN87XX_MASK_LINK_UP                    (0x0004)
+#define LAN87XX_MASK_LINK_DOWN                  (0x0002)

What's the point of that header file if all definitions are consumed by the same driver?

Quoted 1 line

We have planned a series of patches where we planned to use this further.

Are you adding multiple files which share the header? If not, just add the defines to the C code.

Andrew