[PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[Daniel Golle]

Add support for forcing each connected LED to be always on or always off
by implementing the led_brightness_set() op.
This is done by modifying the COM_EXT_LED_GEN_CFG register to enable
force-mode and forcing the LED either on or off.
When calling the led_hw_control_set() force-mode is again disabled for
that LED.
Implement mxl86110_modify_extended_reg() locked helper instead of
manually acquiring and releasing the MDIO bus lock for single
__mxl86110_modify_extended_reg() calls.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: no changes
v2: introduce mxl86110_modify_extended_reg() helper

 drivers/net/phy/mxl-86110.c | 68 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mxl-86110.c b/drivers/net/phy/mxl-86110.c
index ff2a3a22bd5b..31832018655f 100644
--- a/drivers/net/phy/mxl-86110.c
+++ b/drivers/net/phy/mxl-86110.c
@@ -71,6 +71,13 @@
 
 #define MXL86110_MAX_LEDS	3
 /* LED registers and defines */
+#define MXL86110_COM_EXT_LED_GEN_CFG			0xA00B
+# define MXL86110_COM_EXT_LED_GEN_CFG_LFE(x)		BIT(2 + 3 * (x))
+# define MXL86110_COM_EXT_LED_GEN_CFG_LFM(x)		({ typeof(x) _x = (x); \
+							  GENMASK(1 + (3 * (_x)), \
+								 3 * (_x)); })
+#  define MXL86110_COM_EXT_LED_GEN_CFG_LFME(x)		BIT(3 * (x))
+
 #define MXL86110_LED0_CFG_REG 0xA00C
 #define MXL86110_LED1_CFG_REG 0xA00D
 #define MXL86110_LED2_CFG_REG 0xA00E
@@ -235,6 +242,29 @@ static int mxl86110_read_extended_reg(struct phy_device *phydev, u16 regnum)
 	return ret;
 }
 
+/**
+ * mxl86110_modify_extended_reg() - modify bits of a PHY's extended register
+ * @phydev: pointer to the PHY device structure
+ * @regnum: register number to write
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ *
+ * Note: register value = (old register value & ~mask) | set.
+ *
+ * Return: 0 or negative error code
+ */
+static int mxl86110_modify_extended_reg(struct phy_device *phydev,
+					u16 regnum, u16 mask, u16 set)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __mxl86110_modify_extended_reg(phydev, regnum, mask, set);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
 /**
  * mxl86110_get_wol() - report if wake-on-lan is enabled
  * @phydev: pointer to the phy_device
@@ -394,6 +424,7 @@ static int mxl86110_led_hw_control_set(struct phy_device *phydev, u8 index,
 				       unsigned long rules)
 {
 	u16 val = 0;
+	int ret;
 
 	if (index >= MXL86110_MAX_LEDS)
 		return -EINVAL;
@@ -423,8 +454,42 @@ static int mxl86110_led_hw_control_set(struct phy_device *phydev, u8 index,
 	    rules & BIT(TRIGGER_NETDEV_RX))
 		val |= MXL86110_LEDX_CFG_BLINK;
 
-	return mxl86110_write_extended_reg(phydev,
+	ret = mxl86110_write_extended_reg(phydev,
 					  MXL86110_LED0_CFG_REG + index, val);
+	if (ret)
+		return ret;
+
+	ret = mxl86110_modify_extended_reg(phydev,
+					   MXL86110_COM_EXT_LED_GEN_CFG,
+					   MXL86110_COM_EXT_LED_GEN_CFG_LFE(index),
+					   0);
+
+	return ret;
+}
+
+static int mxl86110_led_brightness_set(struct phy_device *phydev,
+				       u8 index, enum led_brightness value)
+{
+	u16 mask, set;
+	int ret;
+
+	if (index >= MXL86110_MAX_LEDS)
+		return -EINVAL;
+
+	/* force manual control */
+	set = MXL86110_COM_EXT_LED_GEN_CFG_LFE(index);
+	/* clear previous force mode */
+	mask = MXL86110_COM_EXT_LED_GEN_CFG_LFM(index);
+
+	/* force LED to be permanently on */
+	if (value != LED_OFF)
+		set |= MXL86110_COM_EXT_LED_GEN_CFG_LFME(index);
+
+	ret = mxl86110_modify_extended_reg(phydev,
+					   MXL86110_COM_EXT_LED_GEN_CFG,
+					   mask, set);
+
+	return ret;
 }
 
 /**
@@ -596,6 +661,7 @@ static struct phy_driver mxl_phy_drvs[] = {
 		.config_init		= mxl86110_config_init,
 		.get_wol		= mxl86110_get_wol,
 		.set_wol		= mxl86110_set_wol,
+		.led_brightness_set	= mxl86110_led_brightness_set,
 		.led_hw_is_supported	= mxl86110_led_hw_is_supported,
 		.led_hw_control_get     = mxl86110_led_hw_control_get,
 		.led_hw_control_set     = mxl86110_led_hw_control_set,
-- 
2.50.1

[PATCH net-next v3 2/3] net: phy: mxl-86110: fix indentation in struct phy_driver

[Daniel Golle]

The .led_hw_control_get and .led_hw_control_set ops are indented with
spaces instead of tabs, unlike the rest of the values of the PHY's
struct phy_driver instance.
Use tabs instead of spaces resulting in a uniform indentation style.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: no changes
v2: move this change into a dedicated patch

 drivers/net/phy/mxl-86110.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mxl-86110.c b/drivers/net/phy/mxl-86110.c
index 31832018655f..47e2fe81842d 100644
--- a/drivers/net/phy/mxl-86110.c
+++ b/drivers/net/phy/mxl-86110.c
@@ -663,8 +663,8 @@ static struct phy_driver mxl_phy_drvs[] = {
 		.set_wol		= mxl86110_set_wol,
 		.led_brightness_set	= mxl86110_led_brightness_set,
 		.led_hw_is_supported	= mxl86110_led_hw_is_supported,
-		.led_hw_control_get     = mxl86110_led_hw_control_get,
-		.led_hw_control_set     = mxl86110_led_hw_control_set,
+		.led_hw_control_get	= mxl86110_led_hw_control_get,
+		.led_hw_control_set	= mxl86110_led_hw_control_set,
 	},
 };
 
-- 
2.50.1

[PATCH net-next v3 3/3] net: phy: mxl-86110: add basic support for MxL86111 PHY

[Daniel Golle]

Add basic support for the MxL86111 PHY which in addition to the features
of the MxL86110 also comes with an SGMII interface.
Setup the interface mode and take care of in-band-an.

Currently only RGMII-to-UTP and SGMII-to-UTP modes are supported while the
PHY would also support RGMII-to-1000Base-X, including automatic selection
of the Fiber or UTP link depending on the presence of a link partner.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: fix kernel-doc comments
v2:
 - move fixing indentation to separate patch
 - acutally use reg_page variable

 drivers/net/phy/mxl-86110.c | 321 ++++++++++++++++++++++++++++++++++--
 1 file changed, 309 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mxl-86110.c b/drivers/net/phy/mxl-86110.c
index 47e2fe81842d..537d8279719f 100644
--- a/drivers/net/phy/mxl-86110.c
+++ b/drivers/net/phy/mxl-86110.c
@@ -15,6 +15,7 @@
 
 /* PHY ID */
 #define PHY_ID_MXL86110		0xc1335580
+#define PHY_ID_MXL86111		0xc1335588
 
 /* required to access extended registers */
 #define MXL86110_EXTD_REG_ADDR_OFFSET			0x1E
@@ -22,7 +23,15 @@
 #define PHY_IRQ_ENABLE_REG				0x12
 #define PHY_IRQ_ENABLE_REG_WOL				BIT(6)
 
-/* SyncE Configuration Register - COM_EXT SYNCE_CFG */
+/* different pages for EXTD access for MXL86111 */
+/* SerDes/PHY Control Access Register - COM_EXT_SMI_SDS_PHY */
+#define MXL86111_EXT_SMI_SDS_PHY_REG			0xA000
+#define MXL86111_EXT_SMI_SDS_PHYSPACE_MASK		BIT(1)
+#define MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE		(0x1 << 1)
+#define MXL86111_EXT_SMI_SDS_PHYUTP_SPACE		(0x0 << 1)
+#define MXL86111_EXT_SMI_SDS_PHY_AUTO			0xff
+
+/* SyncE Configuration Register - COM_EXT_SYNCE_CFG */
 #define MXL86110_EXT_SYNCE_CFG_REG			0xA012
 #define MXL86110_EXT_SYNCE_CFG_CLK_FRE_SEL		BIT(4)
 #define MXL86110_EXT_SYNCE_CFG_EN_SYNC_E_DURING_LNKDN	BIT(5)
@@ -117,9 +126,67 @@
 
 /* Chip Configuration Register - COM_EXT_CHIP_CFG */
 #define MXL86110_EXT_CHIP_CFG_REG			0xA001
+#define MXL86111_EXT_CHIP_CFG_MODE_SEL_MASK		GENMASK(2, 0)
+#define MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_RGMII		0
+#define MXL86111_EXT_CHIP_CFG_MODE_FIBER_TO_RGMII	1
+#define MXL86111_EXT_CHIP_CFG_MODE_UTP_FIBER_TO_RGMII	2
+#define MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_SGMII		3
+#define MXL86111_EXT_CHIP_CFG_MODE_SGPHY_TO_RGMAC	4
+#define MXL86111_EXT_CHIP_CFG_MODE_SGMAC_TO_RGPHY	5
+#define MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_FIBER_AUTO	6
+#define MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_FIBER_FORCE	7
+
+#define MXL86111_EXT_CHIP_CFG_CLDO_MASK			GENMASK(5, 4)
+#define MXL86111_EXT_CHIP_CFG_CLDO_3V3			0
+#define MXL86111_EXT_CHIP_CFG_CLDO_2V5			1
+#define MXL86111_EXT_CHIP_CFG_CLDO_1V8_2		2
+#define MXL86111_EXT_CHIP_CFG_CLDO_1V8_3		3
+#define MXL86111_EXT_CHIP_CFG_CLDO_SHIFT		4
+#define MXL86111_EXT_CHIP_CFG_ELDO			BIT(6)
 #define MXL86110_EXT_CHIP_CFG_RXDLY_ENABLE		BIT(8)
 #define MXL86110_EXT_CHIP_CFG_SW_RST_N_MODE		BIT(15)
 
+/* Specific Status Register - PHY_STAT */
+#define MXL86111_PHY_STAT_REG				0x11
+#define MXL86111_PHY_STAT_SPEED_MASK			GENMASK(15, 14)
+#define MXL86111_PHY_STAT_SPEED_OFFSET			14
+#define MXL86111_PHY_STAT_SPEED_10M			0x0
+#define MXL86111_PHY_STAT_SPEED_100M			0x1
+#define MXL86111_PHY_STAT_SPEED_1000M			0x2
+#define MXL86111_PHY_STAT_DPX_OFFSET			13
+#define MXL86111_PHY_STAT_DPX				BIT(13)
+#define MXL86111_PHY_STAT_LSRT				BIT(10)
+
+/* 3 phy reg page modes,auto mode combines utp and fiber mode*/
+#define MXL86111_MODE_FIBER				0x1
+#define MXL86111_MODE_UTP				0x2
+#define MXL86111_MODE_AUTO				0x3
+
+/* FIBER Auto-Negotiation link partner ability - SDS_AN_LPA */
+#define MXL86111_SDS_AN_LPA_PAUSE			(0x3 << 7)
+#define MXL86111_SDS_AN_LPA_ASYM_PAUSE			(0x2 << 7)
+
+/* Miscellaneous Control Register - COM_EXT _MISC_CFG */
+#define MXL86111_EXT_MISC_CONFIG_REG			0xa006
+#define MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL		BIT(0)
+#define MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL_1000BX	(0x1 << 0)
+#define MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL_100BX	(0x0 << 0)
+
+/* Phy fiber Link timer cfg2 Register - EXT_SDS_LINK_TIMER_CFG2 */
+#define MXL86111_EXT_SDS_LINK_TIMER_CFG2_REG		0xA5
+#define MXL86111_EXT_SDS_LINK_TIMER_CFG2_EN_AUTOSEN	BIT(15)
+
+/* default values of PHY register, required for Dual Media mode */
+#define MII_BMSR_DEFAULT_VAL		0x7949
+#define MII_ESTATUS_DEFAULT_VAL		0x2000
+
+/* Timeout in ms for PHY SW reset check in STD_CTRL/SDS_CTRL */
+#define BMCR_RESET_TIMEOUT		500
+
+/* PL P1 requires optimized RGMII timing for 1.8V RGMII voltage
+ */
+#define MXL86111_PL_P1				0x500
+
 /**
  * __mxl86110_write_extended_reg() - write to a PHY's extended register
  * @phydev: pointer to the PHY device structure
@@ -586,22 +653,15 @@ static int mxl86110_enable_led_activity_blink(struct phy_device *phydev)
 }
 
 /**
- * mxl86110_config_init() - initialize the PHY
+ * mxl86110_config_rgmii_delay() - configure RGMII delays
  * @phydev: pointer to the phy_device
  *
  * Return: 0 or negative errno code
  */
-static int mxl86110_config_init(struct phy_device *phydev)
+static int mxl86110_config_rgmii_delay(struct phy_device *phydev)
 {
-	u16 val = 0;
 	int ret;
-
-	phy_lock_mdio_bus(phydev);
-
-	/* configure syncE / clk output */
-	ret = mxl86110_synce_clk_cfg(phydev);
-	if (ret < 0)
-		goto out;
+	u16 val;
 
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
@@ -643,17 +703,237 @@ static int mxl86110_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		goto out;
 
+out:
+	return ret;
+}
+
+/**
+ * mxl86110_config_init() - initialize the MXL86110 PHY
+ * @phydev: pointer to the phy_device
+ *
+ * Return: 0 or negative errno code
+ */
+static int mxl86110_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	/* configure syncE / clk output */
+	ret = mxl86110_synce_clk_cfg(phydev);
+	if (ret < 0)
+		goto out;
+
+	ret = mxl86110_config_rgmii_delay(phydev);
+	if (ret < 0)
+		goto out;
+
+	ret = mxl86110_enable_led_activity_blink(phydev);
+	if (ret < 0)
+		goto out;
+
+	ret = mxl86110_broadcast_cfg(phydev);
+
+out:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
+/**
+ * mxl86111_probe() - validate bootstrap chip config and set UTP page
+ * @phydev: pointer to the phy_device
+ *
+ * Return: 0 or negative errno code
+ */
+static int mxl86111_probe(struct phy_device *phydev)
+{
+	int chip_config;
+	u16 reg_page;
+	int ret;
+
+	chip_config = mxl86110_read_extended_reg(phydev, MXL86110_EXT_CHIP_CFG_REG);
+	if (chip_config < 0)
+		return chip_config;
+
+	switch (chip_config & MXL86111_EXT_CHIP_CFG_MODE_SEL_MASK) {
+	case MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_SGMII:
+	case MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_RGMII:
+		phydev->port = PORT_TP;
+		reg_page = MXL86111_EXT_SMI_SDS_PHYUTP_SPACE;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = mxl86110_write_extended_reg(phydev,
+					  MXL86111_EXT_SMI_SDS_PHY_REG,
+					  reg_page);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/**
+ * mxl86111_config_init() - initialize the MXL86111 PHY
+ * @phydev: pointer to the phy_device
+ *
+ * Return: 0 or negative errno code
+ */
+static int mxl86111_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	/* configure syncE / clk output */
+	ret = mxl86110_synce_clk_cfg(phydev);
+	if (ret < 0)
+		goto out;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+		ret = __mxl86110_modify_extended_reg(phydev,
+						     MXL86111_EXT_MISC_CONFIG_REG,
+						     MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL,
+						     MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL_100BX);
+		if (ret < 0)
+			goto out;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_SGMII:
+		ret = __mxl86110_modify_extended_reg(phydev,
+						     MXL86111_EXT_MISC_CONFIG_REG,
+						     MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL,
+						     MXL86111_EXT_MISC_CONFIG_FIB_SPEED_SEL_1000BX);
+		if (ret < 0)
+			goto out;
+		break;
+	default:
+		/* RGMII modes */
+		ret = mxl86110_config_rgmii_delay(phydev);
+		if (ret < 0)
+			goto out;
+		ret = __mxl86110_modify_extended_reg(phydev, MXL86110_EXT_RGMII_CFG1_REG,
+						     MXL86110_EXT_RGMII_CFG1_FULL_MASK, ret);
+
+		/* PL P1 requires optimized RGMII timing for 1.8V RGMII voltage
+		 */
+		ret = __mxl86110_read_extended_reg(phydev, 0xf);
+		if (ret < 0)
+			goto out;
+
+		if (ret == MXL86111_PL_P1) {
+			ret = __mxl86110_read_extended_reg(phydev, MXL86110_EXT_CHIP_CFG_REG);
+			if (ret < 0)
+				goto out;
+
+			/* check if LDO is in 1.8V mode */
+			switch (FIELD_GET(MXL86111_EXT_CHIP_CFG_CLDO_MASK, ret)) {
+			case MXL86111_EXT_CHIP_CFG_CLDO_1V8_3:
+			case MXL86111_EXT_CHIP_CFG_CLDO_1V8_2:
+				ret = __mxl86110_write_extended_reg(phydev, 0xa010, 0xabff);
+				if (ret < 0)
+					goto out;
+				break;
+			default:
+				break;
+			}
+		}
+		break;
+	}
+
 	ret = mxl86110_enable_led_activity_blink(phydev);
 	if (ret < 0)
 		goto out;
 
 	ret = mxl86110_broadcast_cfg(phydev);
+out:
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+/**
+ * mxl86111_read_page() - read reg page
+ * @phydev: pointer to the phy_device
+ *
+ * Return: current reg space of mxl86111 or negative errno code
+ */
+static int mxl86111_read_page(struct phy_device *phydev)
+{
+	int page;
+
+	page = __mxl86110_read_extended_reg(phydev, MXL86111_EXT_SMI_SDS_PHY_REG);
+	if (page < 0)
+		return page;
+
+	return page & MXL86111_EXT_SMI_SDS_PHYSPACE_MASK;
+};
+
+/**
+ * mxl86111_write_page() - Set reg page
+ * @phydev: pointer to the phy_device
+ * @page: The reg page to set
+ *
+ * Return: 0 or negative errno code
+ */
+static int mxl86111_write_page(struct phy_device *phydev, int page)
+{
+	return __mxl86110_modify_extended_reg(phydev, MXL86111_EXT_SMI_SDS_PHY_REG,
+					      MXL86111_EXT_SMI_SDS_PHYSPACE_MASK, page);
+};
+
+static int mxl86111_config_inband(struct phy_device *phydev, unsigned int modes)
+{
+	int ret;
+
+	ret = phy_modify_paged(phydev, MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE,
+			       MII_BMCR, BMCR_ANENABLE,
+			       (modes == LINK_INBAND_DISABLE) ? 0 : BMCR_ANENABLE);
+	if (ret < 0)
+		goto out;
+
+	phy_lock_mdio_bus(phydev);
+
+	ret = __mxl86110_modify_extended_reg(phydev, MXL86111_EXT_SDS_LINK_TIMER_CFG2_REG,
+					     MXL86111_EXT_SDS_LINK_TIMER_CFG2_EN_AUTOSEN,
+					     (modes == LINK_INBAND_DISABLE) ? 0 :
+					     MXL86111_EXT_SDS_LINK_TIMER_CFG2_EN_AUTOSEN);
+	if (ret < 0)
+		goto out;
+
+	ret = __mxl86110_modify_extended_reg(phydev, MXL86110_EXT_CHIP_CFG_REG,
+					     MXL86110_EXT_CHIP_CFG_SW_RST_N_MODE, 0);
+	if (ret < 0)
+		goto out;
+
+	/* For fiber forced mode, power down/up to re-aneg */
+	if (modes != LINK_INBAND_DISABLE) {
+		__phy_modify(phydev, MII_BMCR, 0, BMCR_PDOWN);
+		usleep_range(1000, 1050);
+		__phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	}
 
 out:
 	phy_unlock_mdio_bus(phydev);
+
 	return ret;
 }
 
+static unsigned int mxl86111_inband_caps(struct phy_device *phydev,
+					 phy_interface_t interface)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_SGMII:
+		return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
+	default:
+		return 0;
+	}
+}
+
 static struct phy_driver mxl_phy_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_MXL86110),
@@ -666,17 +946,34 @@ static struct phy_driver mxl_phy_drvs[] = {
 		.led_hw_control_get	= mxl86110_led_hw_control_get,
 		.led_hw_control_set	= mxl86110_led_hw_control_set,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_MXL86111),
+		.name			= "MXL86111 Gigabit Ethernet",
+		.probe			= mxl86111_probe,
+		.config_init		= mxl86111_config_init,
+		.get_wol		= mxl86110_get_wol,
+		.set_wol		= mxl86110_set_wol,
+		.inband_caps		= mxl86111_inband_caps,
+		.config_inband		= mxl86111_config_inband,
+		.read_page		= mxl86111_read_page,
+		.write_page		= mxl86111_write_page,
+		.led_brightness_set	= mxl86110_led_brightness_set,
+		.led_hw_is_supported	= mxl86110_led_hw_is_supported,
+		.led_hw_control_get	= mxl86110_led_hw_control_get,
+		.led_hw_control_set	= mxl86110_led_hw_control_set,
+	},
 };
 
 module_phy_driver(mxl_phy_drvs);
 
 static const struct mdio_device_id __maybe_unused mxl_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_MXL86110) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_MXL86111) },
 	{  }
 };
 
 MODULE_DEVICE_TABLE(mdio, mxl_tbl);
 
-MODULE_DESCRIPTION("MaxLinear MXL86110 PHY driver");
+MODULE_DESCRIPTION("MaxLinear MXL86110/MXL86111 PHY driver");
 MODULE_AUTHOR("Stefano Radaelli");
 MODULE_LICENSE("GPL");
-- 
2.50.1

Re: [PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[Andrew Lunn]

> +# define MXL86110_COM_EXT_LED_GEN_CFG_LFM(x)		({ typeof(x) _x = (x); \
> +							  GENMASK(1 + (3 * (_x)), \
> +								 3 * (_x)); })

> +static int mxl86110_led_brightness_set(struct phy_device *phydev,
> +				       u8 index, enum led_brightness value)
> +{
> +	u16 mask, set;
> +	int ret;
> +
> +	if (index >= MXL86110_MAX_LEDS)
> +		return -EINVAL;
> +
> +	/* force manual control */
> +	set = MXL86110_COM_EXT_LED_GEN_CFG_LFE(index);
> +	/* clear previous force mode */
> +	mask = MXL86110_COM_EXT_LED_GEN_CFG_LFM(index);
> +
> +	/* force LED to be permanently on */
> +	if (value != LED_OFF)
> +		set |= MXL86110_COM_EXT_LED_GEN_CFG_LFME(index);

That is particularly complex. We know index is a u8, so why not
GENMASK_U8(1 + 3 * index, 3 * index)? But set is a u16, so
GENMASK_U16() would also be valid.

	Andrew

Re: [PATCH net-next v3 2/3] net: phy: mxl-86110: fix indentation in struct phy_driver

[Andrew Lunn]

On Wed, Aug 20, 2025 at 01:11:29PM +0100, Daniel Golle wrote:
61;8003;1c> The .led_hw_control_get and .led_hw_control_set ops are indented with
> spaces instead of tabs, unlike the rest of the values of the PHY's
> struct phy_driver instance.
> Use tabs instead of spaces resulting in a uniform indentation style.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3 3/3] net: phy: mxl-86110: add basic support for MxL86111 PHY

[Andrew Lunn]

On Wed, Aug 20, 2025 at 01:12:06PM +0100, Daniel Golle wrote:
> Add basic support for the MxL86111 PHY which in addition to the features
> of the MxL86110 also comes with an SGMII interface.
> Setup the interface mode and take care of in-band-an.
> 
> Currently only RGMII-to-UTP and SGMII-to-UTP modes are supported while the
> PHY would also support RGMII-to-1000Base-X, including automatic selection
> of the Fiber or UTP link depending on the presence of a link partner.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[Daniel Golle]

On Wed, Aug 20, 2025 at 02:34:48PM +0200, Andrew Lunn wrote:
> > +# define MXL86110_COM_EXT_LED_GEN_CFG_LFM(x)		({ typeof(x) _x = (x); \
> > +							  GENMASK(1 + (3 * (_x)), \
> > +								 3 * (_x)); })
> 
> > +static int mxl86110_led_brightness_set(struct phy_device *phydev,
> > +				       u8 index, enum led_brightness value)
> > +{
> > +	u16 mask, set;
> > +	int ret;
> > +
> > +	if (index >= MXL86110_MAX_LEDS)
> > +		return -EINVAL;
> > +
> > +	/* force manual control */
> > +	set = MXL86110_COM_EXT_LED_GEN_CFG_LFE(index);
> > +	/* clear previous force mode */
> > +	mask = MXL86110_COM_EXT_LED_GEN_CFG_LFM(index);
> > +
> > +	/* force LED to be permanently on */
> > +	if (value != LED_OFF)
> > +		set |= MXL86110_COM_EXT_LED_GEN_CFG_LFME(index);
> 
> That is particularly complex. We know index is a u8, so why not
> GENMASK_U8(1 + 3 * index, 3 * index)? But set is a u16, so
> GENMASK_U16() would also be valid.

I chose this construct to avoid reusing the macro parameter as gcc would
rightously complain about that potentially having unexpected side-effects.

Eg.

#define FOO(a) ((a)+(a))

Now with var=10, when calling FOO(var++) the result will be 21 and
var will be equal to 12, which isn't intuitive without seeing the
macro definition.

Also using GENMASK_TYPE would not avoid the problem of macro
parameter reuse.

However, I agree that the macro itself is also weirdly complex and
confusing (but at the same time also very common, a quick grep reveals
hundreds of occurances of that pattern in Linux sources), so maybe we
should introduce some generic helpers for this (quite common) use-case?
I can do that, but I certainly can't take care of migrating all the
existing uses of this pattern to switch to the new helper.

Re: [PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[Jakub Kicinski]

On Wed, 20 Aug 2025 14:34:07 +0100 Daniel Golle wrote:
> > > +	if (index >= MXL86110_MAX_LEDS)
> > > +		return -EINVAL;
> > > +
> > > +	/* force manual control */
> > > +	set = MXL86110_COM_EXT_LED_GEN_CFG_LFE(index);
> > > +	/* clear previous force mode */
> > > +	mask = MXL86110_COM_EXT_LED_GEN_CFG_LFM(index);
> > > +
> > > +	/* force LED to be permanently on */
> > > +	if (value != LED_OFF)
> > > +		set |= MXL86110_COM_EXT_LED_GEN_CFG_LFME(index);  
> > 
> > That is particularly complex. We know index is a u8, so why not
> > GENMASK_U8(1 + 3 * index, 3 * index)? But set is a u16, so
> > GENMASK_U16() would also be valid.  
> 
> I chose this construct to avoid reusing the macro parameter as gcc would
> rightously complain about that potentially having unexpected side-effects.
> 
> Eg.
> 
> #define FOO(a) ((a)+(a))
> 
> Now with var=10, when calling FOO(var++) the result will be 21 and
> var will be equal to 12, which isn't intuitive without seeing the
> macro definition.
> 
> Also using GENMASK_TYPE would not avoid the problem of macro
> parameter reuse.
> 
> However, I agree that the macro itself is also weirdly complex and
> confusing (but at the same time also very common, a quick grep reveals
> hundreds of occurances of that pattern in Linux sources), so maybe we
> should introduce some generic helpers for this (quite common) use-case?
> I can do that, but I certainly can't take care of migrating all the
> existing uses of this pattern to switch to the new helper.

IMHO this is one of the cases where the code would be far clearer
without he macros..

#define MXL86110_COM_EXT_LED_GEN_CFG			0xA00B
# define MXL86110_COM_EXT_LED_GEN_CFG_LFME(x)		(1 << (3 * (x)))
# define MXL86110_COM_EXT_LED_GEN_CFG_LFE(x)		(2 << (3 * (x)))
# define MXL86110_COM_EXT_LED_GEN_CFG_MASK(x)		(3 << (3 * (x)))

Right?

Re: [PATCH net-next v3 1/3] net: phy: mxl-86110: add basic support for led_brightness_set op

[Daniel Golle]

On Thu, Aug 21, 2025 at 05:36:20PM -0700, Jakub Kicinski wrote:
> On Wed, 20 Aug 2025 14:34:07 +0100 Daniel Golle wrote:
> > > > +	if (index >= MXL86110_MAX_LEDS)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* force manual control */
> > > > +	set = MXL86110_COM_EXT_LED_GEN_CFG_LFE(index);
> > > > +	/* clear previous force mode */
> > > > +	mask = MXL86110_COM_EXT_LED_GEN_CFG_LFM(index);
> > > > +
> > > > +	/* force LED to be permanently on */
> > > > +	if (value != LED_OFF)
> > > > +		set |= MXL86110_COM_EXT_LED_GEN_CFG_LFME(index);  
> > > 
> > > That is particularly complex. We know index is a u8, so why not
> > > GENMASK_U8(1 + 3 * index, 3 * index)? But set is a u16, so
> > > GENMASK_U16() would also be valid.  
> > 
> > I chose this construct to avoid reusing the macro parameter as gcc would
> > rightously complain about that potentially having unexpected side-effects.
> > 
> > Eg.
> > 
> > #define FOO(a) ((a)+(a))
> > 
> > Now with var=10, when calling FOO(var++) the result will be 21 and
> > var will be equal to 12, which isn't intuitive without seeing the
> > macro definition.
> > 
> > Also using GENMASK_TYPE would not avoid the problem of macro
> > parameter reuse.
> > 
> > However, I agree that the macro itself is also weirdly complex and
> > confusing (but at the same time also very common, a quick grep reveals
> > hundreds of occurances of that pattern in Linux sources), so maybe we
> > should introduce some generic helpers for this (quite common) use-case?
> > I can do that, but I certainly can't take care of migrating all the
> > existing uses of this pattern to switch to the new helper.
> 
> IMHO this is one of the cases where the code would be far clearer
> without he macros..
> 
> #define MXL86110_COM_EXT_LED_GEN_CFG			0xA00B
> # define MXL86110_COM_EXT_LED_GEN_CFG_LFME(x)		(1 << (3 * (x)))
> # define MXL86110_COM_EXT_LED_GEN_CFG_LFE(x)		(2 << (3 * (x)))
> # define MXL86110_COM_EXT_LED_GEN_CFG_MASK(x)		(3 << (3 * (x)))
> 
> Right?

I agree. If everyone is fine with using numbers and bitshift instead of
GENMASK(...) then I'd also prefer it in this case.