diff mbox series

[RFC,net-next] net: phy: intel-xway: remove LED configuration

Message ID 20230302141651.377261-1-michael@walle.cc (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,net-next] net: phy: intel-xway: remove LED configuration | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 2
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang fail Errors and warnings before: 0 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 167 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Walle March 2, 2023, 2:16 p.m. UTC 
For this PHY, the LEDs can either be configured through an attached
EEPROM or if not available, the PHY offers sane default modes. Right now,
the driver will configure to a mode just suitable for one configuration
(although there is a bold claim that "most boards have just one LED").
I'd argue, that as long as there is no configuration through other means
(like device tree), the driver shouldn't configure anything LED related
so that the PHY is using either the modes configured by the EEPROM or
the power-on defaults.

Signed-off-by: Michael Walle <michael@walle.cc>
---
I know there is ongoing work on the device tree, but even then, my
argument holds, if there is no config in the device tree, the driver
shouldn't just use "any" configuration when there are means by the
hardware to configure the LEDs.

Not just as an RFC because netdev is closed, but also to get other
opinions. Not to be applied.
---
 drivers/net/phy/intel-xway.c | 149 -----------------------------------
 1 file changed, 149 deletions(-)

Comments

Andrew Lunn March 2, 2023, 2:55 p.m. UTC | #1 
On Thu, Mar 02, 2023 at 03:16:51PM +0100, Michael Walle wrote:
> For this PHY, the LEDs can either be configured through an attached
> EEPROM or if not available, the PHY offers sane default modes. Right now,
> the driver will configure to a mode just suitable for one configuration
> (although there is a bold claim that "most boards have just one LED").
> I'd argue, that as long as there is no configuration through other means
> (like device tree), the driver shouldn't configure anything LED related
> so that the PHY is using either the modes configured by the EEPROM or
> the power-on defaults.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> I know there is ongoing work on the device tree, but even then, my
> argument holds, if there is no config in the device tree, the driver
> shouldn't just use "any" configuration when there are means by the
> hardware to configure the LEDs.
> 
> Not just as an RFC because netdev is closed, but also to get other
> opinions. Not to be applied.

I would suggest you CC: the two people responsible for adding this
code:

    Signed-off-by: John Crispin <john@phrozen.org>
    Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

So i guess this came from OpenWRT? Maybe they can tell us if this
change is going to cause regressions.

I would say there is no right defaults for LEDs, whatever you do will
be wrong for somebody. And the way this driver sets the LEDs has been
there since the first commit. This is its decision of what the default
should be. So i'm leaning towards rejecting your definition of what
the default should be.

There is progress on controlling PHY LEDs via /sysfs. So i think you
should wait until Christian and I get the API between the core and the
PHY driver stable, and then help us by implementing support in the
intel-xway.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c
index 3c032868ef04..4428721238a7 100644
--- a/drivers/net/phy/intel-xway.c
+++ b/drivers/net/phy/intel-xway.c
@@ -18,17 +18,6 @@ 
 #define XWAY_MDIO_MIICTRL_RXSKEW_MASK	GENMASK(14, 12)
 #define XWAY_MDIO_MIICTRL_TXSKEW_MASK	GENMASK(10, 8)
 
-/* bit 15:12 are reserved */
-#define XWAY_MDIO_LED_LED3_EN		BIT(11)	/* Enable the integrated function of LED3 */
-#define XWAY_MDIO_LED_LED2_EN		BIT(10)	/* Enable the integrated function of LED2 */
-#define XWAY_MDIO_LED_LED1_EN		BIT(9)	/* Enable the integrated function of LED1 */
-#define XWAY_MDIO_LED_LED0_EN		BIT(8)	/* Enable the integrated function of LED0 */
-/* bit 7:4 are reserved */
-#define XWAY_MDIO_LED_LED3_DA		BIT(3)	/* Direct Access to LED3 */
-#define XWAY_MDIO_LED_LED2_DA		BIT(2)	/* Direct Access to LED2 */
-#define XWAY_MDIO_LED_LED1_DA		BIT(1)	/* Direct Access to LED1 */
-#define XWAY_MDIO_LED_LED0_DA		BIT(0)	/* Direct Access to LED0 */
-
 #define XWAY_MDIO_INIT_WOL		BIT(15)	/* Wake-On-LAN */
 #define XWAY_MDIO_INIT_MSRE		BIT(14)
 #define XWAY_MDIO_INIT_NPRX		BIT(13)
@@ -46,111 +35,6 @@ 
 
 #define ADVERTISED_MPD			BIT(10)	/* Multi-port device */
 
-/* LED Configuration */
-#define XWAY_MMD_LEDCH			0x01E0
-/* Inverse of SCAN Function */
-#define  XWAY_MMD_LEDCH_NACS_NONE	0x0000
-#define  XWAY_MMD_LEDCH_NACS_LINK	0x0001
-#define  XWAY_MMD_LEDCH_NACS_PDOWN	0x0002
-#define  XWAY_MMD_LEDCH_NACS_EEE	0x0003
-#define  XWAY_MMD_LEDCH_NACS_ANEG	0x0004
-#define  XWAY_MMD_LEDCH_NACS_ABIST	0x0005
-#define  XWAY_MMD_LEDCH_NACS_CDIAG	0x0006
-#define  XWAY_MMD_LEDCH_NACS_TEST	0x0007
-/* Slow Blink Frequency */
-#define  XWAY_MMD_LEDCH_SBF_F02HZ	0x0000
-#define  XWAY_MMD_LEDCH_SBF_F04HZ	0x0010
-#define  XWAY_MMD_LEDCH_SBF_F08HZ	0x0020
-#define  XWAY_MMD_LEDCH_SBF_F16HZ	0x0030
-/* Fast Blink Frequency */
-#define  XWAY_MMD_LEDCH_FBF_F02HZ	0x0000
-#define  XWAY_MMD_LEDCH_FBF_F04HZ	0x0040
-#define  XWAY_MMD_LEDCH_FBF_F08HZ	0x0080
-#define  XWAY_MMD_LEDCH_FBF_F16HZ	0x00C0
-/* LED Configuration */
-#define XWAY_MMD_LEDCL			0x01E1
-/* Complex Blinking Configuration */
-#define  XWAY_MMD_LEDCH_CBLINK_NONE	0x0000
-#define  XWAY_MMD_LEDCH_CBLINK_LINK	0x0001
-#define  XWAY_MMD_LEDCH_CBLINK_PDOWN	0x0002
-#define  XWAY_MMD_LEDCH_CBLINK_EEE	0x0003
-#define  XWAY_MMD_LEDCH_CBLINK_ANEG	0x0004
-#define  XWAY_MMD_LEDCH_CBLINK_ABIST	0x0005
-#define  XWAY_MMD_LEDCH_CBLINK_CDIAG	0x0006
-#define  XWAY_MMD_LEDCH_CBLINK_TEST	0x0007
-/* Complex SCAN Configuration */
-#define  XWAY_MMD_LEDCH_SCAN_NONE	0x0000
-#define  XWAY_MMD_LEDCH_SCAN_LINK	0x0010
-#define  XWAY_MMD_LEDCH_SCAN_PDOWN	0x0020
-#define  XWAY_MMD_LEDCH_SCAN_EEE	0x0030
-#define  XWAY_MMD_LEDCH_SCAN_ANEG	0x0040
-#define  XWAY_MMD_LEDCH_SCAN_ABIST	0x0050
-#define  XWAY_MMD_LEDCH_SCAN_CDIAG	0x0060
-#define  XWAY_MMD_LEDCH_SCAN_TEST	0x0070
-/* Configuration for LED Pin x */
-#define XWAY_MMD_LED0H			0x01E2
-/* Fast Blinking Configuration */
-#define  XWAY_MMD_LEDxH_BLINKF_MASK	0x000F
-#define  XWAY_MMD_LEDxH_BLINKF_NONE	0x0000
-#define  XWAY_MMD_LEDxH_BLINKF_LINK10	0x0001
-#define  XWAY_MMD_LEDxH_BLINKF_LINK100	0x0002
-#define  XWAY_MMD_LEDxH_BLINKF_LINK10X	0x0003
-#define  XWAY_MMD_LEDxH_BLINKF_LINK1000	0x0004
-#define  XWAY_MMD_LEDxH_BLINKF_LINK10_0	0x0005
-#define  XWAY_MMD_LEDxH_BLINKF_LINK100X	0x0006
-#define  XWAY_MMD_LEDxH_BLINKF_LINK10XX	0x0007
-#define  XWAY_MMD_LEDxH_BLINKF_PDOWN	0x0008
-#define  XWAY_MMD_LEDxH_BLINKF_EEE	0x0009
-#define  XWAY_MMD_LEDxH_BLINKF_ANEG	0x000A
-#define  XWAY_MMD_LEDxH_BLINKF_ABIST	0x000B
-#define  XWAY_MMD_LEDxH_BLINKF_CDIAG	0x000C
-/* Constant On Configuration */
-#define  XWAY_MMD_LEDxH_CON_MASK	0x00F0
-#define  XWAY_MMD_LEDxH_CON_NONE	0x0000
-#define  XWAY_MMD_LEDxH_CON_LINK10	0x0010
-#define  XWAY_MMD_LEDxH_CON_LINK100	0x0020
-#define  XWAY_MMD_LEDxH_CON_LINK10X	0x0030
-#define  XWAY_MMD_LEDxH_CON_LINK1000	0x0040
-#define  XWAY_MMD_LEDxH_CON_LINK10_0	0x0050
-#define  XWAY_MMD_LEDxH_CON_LINK100X	0x0060
-#define  XWAY_MMD_LEDxH_CON_LINK10XX	0x0070
-#define  XWAY_MMD_LEDxH_CON_PDOWN	0x0080
-#define  XWAY_MMD_LEDxH_CON_EEE		0x0090
-#define  XWAY_MMD_LEDxH_CON_ANEG	0x00A0
-#define  XWAY_MMD_LEDxH_CON_ABIST	0x00B0
-#define  XWAY_MMD_LEDxH_CON_CDIAG	0x00C0
-#define  XWAY_MMD_LEDxH_CON_COPPER	0x00D0
-#define  XWAY_MMD_LEDxH_CON_FIBER	0x00E0
-/* Configuration for LED Pin x */
-#define XWAY_MMD_LED0L			0x01E3
-/* Pulsing Configuration */
-#define  XWAY_MMD_LEDxL_PULSE_MASK	0x000F
-#define  XWAY_MMD_LEDxL_PULSE_NONE	0x0000
-#define  XWAY_MMD_LEDxL_PULSE_TXACT	0x0001
-#define  XWAY_MMD_LEDxL_PULSE_RXACT	0x0002
-#define  XWAY_MMD_LEDxL_PULSE_COL	0x0004
-/* Slow Blinking Configuration */
-#define  XWAY_MMD_LEDxL_BLINKS_MASK	0x00F0
-#define  XWAY_MMD_LEDxL_BLINKS_NONE	0x0000
-#define  XWAY_MMD_LEDxL_BLINKS_LINK10	0x0010
-#define  XWAY_MMD_LEDxL_BLINKS_LINK100	0x0020
-#define  XWAY_MMD_LEDxL_BLINKS_LINK10X	0x0030
-#define  XWAY_MMD_LEDxL_BLINKS_LINK1000	0x0040
-#define  XWAY_MMD_LEDxL_BLINKS_LINK10_0	0x0050
-#define  XWAY_MMD_LEDxL_BLINKS_LINK100X	0x0060
-#define  XWAY_MMD_LEDxL_BLINKS_LINK10XX	0x0070
-#define  XWAY_MMD_LEDxL_BLINKS_PDOWN	0x0080
-#define  XWAY_MMD_LEDxL_BLINKS_EEE	0x0090
-#define  XWAY_MMD_LEDxL_BLINKS_ANEG	0x00A0
-#define  XWAY_MMD_LEDxL_BLINKS_ABIST	0x00B0
-#define  XWAY_MMD_LEDxL_BLINKS_CDIAG	0x00C0
-#define XWAY_MMD_LED1H			0x01E4
-#define XWAY_MMD_LED1L			0x01E5
-#define XWAY_MMD_LED2H			0x01E6
-#define XWAY_MMD_LED2L			0x01E7
-#define XWAY_MMD_LED3H			0x01E8
-#define XWAY_MMD_LED3L			0x01E9
-
 #define PHY_ID_PHY11G_1_3		0x030260D1
 #define PHY_ID_PHY22F_1_3		0x030260E1
 #define PHY_ID_PHY11G_1_4		0xD565A400
@@ -243,39 +127,6 @@  static int xway_gphy_config_init(struct phy_device *phydev)
 	/* Clear all pending interrupts */
 	phy_read(phydev, XWAY_MDIO_ISTAT);
 
-	/* Ensure that integrated led function is enabled for all leds */
-	err = phy_write(phydev, XWAY_MDIO_LED,
-			XWAY_MDIO_LED_LED0_EN |
-			XWAY_MDIO_LED_LED1_EN |
-			XWAY_MDIO_LED_LED2_EN |
-			XWAY_MDIO_LED_LED3_EN);
-	if (err)
-		return err;
-
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LEDCH,
-		      XWAY_MMD_LEDCH_NACS_NONE |
-		      XWAY_MMD_LEDCH_SBF_F02HZ |
-		      XWAY_MMD_LEDCH_FBF_F16HZ);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LEDCL,
-		      XWAY_MMD_LEDCH_CBLINK_NONE |
-		      XWAY_MMD_LEDCH_SCAN_NONE);
-
-	/**
-	 * In most cases only one LED is connected to this phy, so
-	 * configure them all to constant on and pulse mode. LED3 is
-	 * only available in some packages, leave it in its reset
-	 * configuration.
-	 */
-	ledxh = XWAY_MMD_LEDxH_BLINKF_NONE | XWAY_MMD_LEDxH_CON_LINK10XX;
-	ledxl = XWAY_MMD_LEDxL_PULSE_TXACT | XWAY_MMD_LEDxL_PULSE_RXACT |
-		XWAY_MMD_LEDxL_BLINKS_NONE;
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED0H, ledxh);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED0L, ledxl);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED1H, ledxh);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED1L, ledxl);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
-	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
-
 	err = xway_gphy_rgmii_init(phydev);
 	if (err)
 		return err;