[PATCH net-next 0/3] net: phy: marvell-88q2xxx: cleanup

[PATCH net-next 0/3] net: phy: marvell-88q2xxx: cleanup

[Dimitri Fedrau]

- align defines
- order includes alphabetically
- enable temperature sensor in mv88q2xxx_config_init

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
Dimitri Fedrau (3):
      net: phy: marvell-88q2xxx: align defines
      net: phy: marvell-88q2xxx: order includes alphabetically
      net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

 drivers/net/phy/marvell-88q2xxx.c | 77 +++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 40 deletions(-)
---
base-commit: 7a7e0197133d18cfd9931e7d3a842d0f5730223f
change-id: 20250214-marvell-88q2xxx-cleanup-82e28b5271de

Best regards,
-- 
Dimitri Fedrau <dima.fedrau@gmail.com>

[PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Dimitri Fedrau]

Align some defines.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 62 +++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index bad5e7b2357da067bfd1ec6bd1307c42f5dc5c91..6e95de080bc65e8e8543d4effb9846fdd823a9d4 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -12,29 +12,29 @@
 #include <linux/phy.h>
 #include <linux/hwmon.h>
 
-#define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
-#define PHY_ID_88Q2220_REVB1	(MARVELL_PHY_ID_88Q2220 | 0x2)
-#define PHY_ID_88Q2220_REVB2	(MARVELL_PHY_ID_88Q2220 | 0x3)
-
-#define MDIO_MMD_AN_MV_STAT			32769
-#define MDIO_MMD_AN_MV_STAT_ANEG		0x0100
-#define MDIO_MMD_AN_MV_STAT_LOCAL_RX		0x1000
-#define MDIO_MMD_AN_MV_STAT_REMOTE_RX		0x2000
-#define MDIO_MMD_AN_MV_STAT_LOCAL_MASTER	0x4000
-#define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT	0x8000
-
-#define MDIO_MMD_AN_MV_STAT2			32794
-#define MDIO_MMD_AN_MV_STAT2_AN_RESOLVED	0x0800
-#define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
-#define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
-
-#define MDIO_MMD_PCS_MV_RESET_CTRL		32768
-#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE	0x8
-
-#define MDIO_MMD_PCS_MV_INT_EN			32784
-#define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
-#define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
-#define MDIO_MMD_PCS_MV_INT_EN_100BT1		0x1000
+#define PHY_ID_88Q2220_REVB0				(MARVELL_PHY_ID_88Q2220 | 0x1)
+#define PHY_ID_88Q2220_REVB1				(MARVELL_PHY_ID_88Q2220 | 0x2)
+#define PHY_ID_88Q2220_REVB2				(MARVELL_PHY_ID_88Q2220 | 0x3)
+
+#define MDIO_MMD_AN_MV_STAT				32769
+#define MDIO_MMD_AN_MV_STAT_ANEG			0x0100
+#define MDIO_MMD_AN_MV_STAT_LOCAL_RX			0x1000
+#define MDIO_MMD_AN_MV_STAT_REMOTE_RX			0x2000
+#define MDIO_MMD_AN_MV_STAT_LOCAL_MASTER		0x4000
+#define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT		0x8000
+
+#define MDIO_MMD_AN_MV_STAT2				32794
+#define MDIO_MMD_AN_MV_STAT2_AN_RESOLVED		0x0800
+#define MDIO_MMD_AN_MV_STAT2_100BT1			0x2000
+#define MDIO_MMD_AN_MV_STAT2_1000BT1			0x4000
+
+#define MDIO_MMD_PCS_MV_RESET_CTRL			32768
+#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE		0x8
+
+#define MDIO_MMD_PCS_MV_INT_EN				32784
+#define MDIO_MMD_PCS_MV_INT_EN_LINK_UP			0x0040
+#define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN		0x0080
+#define MDIO_MMD_PCS_MV_INT_EN_100BT1			0x1000
 
 #define MDIO_MMD_PCS_MV_GPIO_INT_STAT			32785
 #define MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_UP		0x0040
@@ -80,11 +80,11 @@
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_REMOTE_RX		0x2000
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_LOCAL_MASTER	0x4000
 
-#define MDIO_MMD_PCS_MV_100BT1_STAT2		33033
-#define MDIO_MMD_PCS_MV_100BT1_STAT2_JABBER	0x0001
-#define MDIO_MMD_PCS_MV_100BT1_STAT2_POL	0x0002
-#define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK	0x0004
-#define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE	0x0008
+#define MDIO_MMD_PCS_MV_100BT1_STAT2			33033
+#define MDIO_MMD_PCS_MV_100BT1_STAT2_JABBER		0x0001
+#define MDIO_MMD_PCS_MV_100BT1_STAT2_POL		0x0002
+#define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK		0x0004
+#define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE		0x0008
 
 #define MDIO_MMD_PCS_MV_100BT1_INT_EN			33042
 #define MDIO_MMD_PCS_MV_100BT1_INT_EN_LINKEVENT		0x0400
@@ -92,7 +92,7 @@
 #define MDIO_MMD_PCS_MV_COPPER_INT_STAT			33043
 #define MDIO_MMD_PCS_MV_COPPER_INT_STAT_LINKEVENT	0x0400
 
-#define MDIO_MMD_PCS_MV_RX_STAT			33328
+#define MDIO_MMD_PCS_MV_RX_STAT				33328
 
 #define MDIO_MMD_PCS_MV_TDR_RESET			65226
 #define MDIO_MMD_PCS_MV_TDR_RESET_TDR_RST		0x1000
@@ -115,8 +115,8 @@
 
 #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
 
-#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
-#define MV88Q2XXX_LED_INDEX_GPIO	1
+#define MV88Q2XXX_LED_INDEX_TX_ENABLE			0
+#define MV88Q2XXX_LED_INDEX_GPIO			1
 
 struct mv88q2xxx_priv {
 	bool enable_temp;

-- 
2.39.5

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Niklas Söderlund]

Hi Dimitri,

Thanks for your work.

On 2025-02-14 17:32:03 +0100, Dimitri Fedrau wrote:
> Align some defines.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/phy/marvell-88q2xxx.c | 62 +++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index bad5e7b2357da067bfd1ec6bd1307c42f5dc5c91..6e95de080bc65e8e8543d4effb9846fdd823a9d4 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -12,29 +12,29 @@
>  #include <linux/phy.h>
>  #include <linux/hwmon.h>
>  
> -#define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
> -#define PHY_ID_88Q2220_REVB1	(MARVELL_PHY_ID_88Q2220 | 0x2)
> -#define PHY_ID_88Q2220_REVB2	(MARVELL_PHY_ID_88Q2220 | 0x3)
> -
> -#define MDIO_MMD_AN_MV_STAT			32769
> -#define MDIO_MMD_AN_MV_STAT_ANEG		0x0100
> -#define MDIO_MMD_AN_MV_STAT_LOCAL_RX		0x1000
> -#define MDIO_MMD_AN_MV_STAT_REMOTE_RX		0x2000
> -#define MDIO_MMD_AN_MV_STAT_LOCAL_MASTER	0x4000
> -#define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT	0x8000
> -
> -#define MDIO_MMD_AN_MV_STAT2			32794
> -#define MDIO_MMD_AN_MV_STAT2_AN_RESOLVED	0x0800
> -#define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
> -#define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
> -
> -#define MDIO_MMD_PCS_MV_RESET_CTRL		32768
> -#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE	0x8
> -
> -#define MDIO_MMD_PCS_MV_INT_EN			32784
> -#define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
> -#define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
> -#define MDIO_MMD_PCS_MV_INT_EN_100BT1		0x1000
> +#define PHY_ID_88Q2220_REVB0				(MARVELL_PHY_ID_88Q2220 | 0x1)
> +#define PHY_ID_88Q2220_REVB1				(MARVELL_PHY_ID_88Q2220 | 0x2)
> +#define PHY_ID_88Q2220_REVB2				(MARVELL_PHY_ID_88Q2220 | 0x3)
> +
> +#define MDIO_MMD_AN_MV_STAT				32769
> +#define MDIO_MMD_AN_MV_STAT_ANEG			0x0100
> +#define MDIO_MMD_AN_MV_STAT_LOCAL_RX			0x1000
> +#define MDIO_MMD_AN_MV_STAT_REMOTE_RX			0x2000
> +#define MDIO_MMD_AN_MV_STAT_LOCAL_MASTER		0x4000
> +#define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT		0x8000
> +
> +#define MDIO_MMD_AN_MV_STAT2				32794
> +#define MDIO_MMD_AN_MV_STAT2_AN_RESOLVED		0x0800
> +#define MDIO_MMD_AN_MV_STAT2_100BT1			0x2000
> +#define MDIO_MMD_AN_MV_STAT2_1000BT1			0x4000
> +
> +#define MDIO_MMD_PCS_MV_RESET_CTRL			32768
> +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE		0x8
> +
> +#define MDIO_MMD_PCS_MV_INT_EN				32784
> +#define MDIO_MMD_PCS_MV_INT_EN_LINK_UP			0x0040
> +#define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN		0x0080
> +#define MDIO_MMD_PCS_MV_INT_EN_100BT1			0x1000
>  
>  #define MDIO_MMD_PCS_MV_GPIO_INT_STAT			32785
>  #define MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_UP		0x0040
> @@ -80,11 +80,11 @@
>  #define MDIO_MMD_PCS_MV_100BT1_STAT1_REMOTE_RX		0x2000
>  #define MDIO_MMD_PCS_MV_100BT1_STAT1_LOCAL_MASTER	0x4000
>  
> -#define MDIO_MMD_PCS_MV_100BT1_STAT2		33033
> -#define MDIO_MMD_PCS_MV_100BT1_STAT2_JABBER	0x0001
> -#define MDIO_MMD_PCS_MV_100BT1_STAT2_POL	0x0002
> -#define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK	0x0004
> -#define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE	0x0008
> +#define MDIO_MMD_PCS_MV_100BT1_STAT2			33033
> +#define MDIO_MMD_PCS_MV_100BT1_STAT2_JABBER		0x0001
> +#define MDIO_MMD_PCS_MV_100BT1_STAT2_POL		0x0002
> +#define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK		0x0004
> +#define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE		0x0008
>  
>  #define MDIO_MMD_PCS_MV_100BT1_INT_EN			33042
>  #define MDIO_MMD_PCS_MV_100BT1_INT_EN_LINKEVENT		0x0400
> @@ -92,7 +92,7 @@
>  #define MDIO_MMD_PCS_MV_COPPER_INT_STAT			33043
>  #define MDIO_MMD_PCS_MV_COPPER_INT_STAT_LINKEVENT	0x0400
>  
> -#define MDIO_MMD_PCS_MV_RX_STAT			33328
> +#define MDIO_MMD_PCS_MV_RX_STAT				33328
>  
>  #define MDIO_MMD_PCS_MV_TDR_RESET			65226
>  #define MDIO_MMD_PCS_MV_TDR_RESET_TDR_RST		0x1000
> @@ -115,8 +115,8 @@
>  
>  #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
>  
> -#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
> -#define MV88Q2XXX_LED_INDEX_GPIO	1
> +#define MV88Q2XXX_LED_INDEX_TX_ENABLE			0
> +#define MV88Q2XXX_LED_INDEX_GPIO			1
>  
>  struct mv88q2xxx_priv {
>  	bool enable_temp;
> 
> -- 
> 2.39.5
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Marek Behún]

> +#define MDIO_MMD_AN_MV_STAT				32769

Why the hell are register addresses in this driver in decimal?

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Andrew Lunn]

On Tue, Feb 18, 2025 at 12:54:29PM +0100, Marek Behún wrote:
> > +#define MDIO_MMD_AN_MV_STAT				32769
> 
> Why the hell are register addresses in this driver in decimal?

Maybe because 802.3 uses decimal? Take a look at Table 45–3—PMA/PMD
registers for example.

	Andrew

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Marek Behún]

On Tue, Feb 18, 2025 at 03:12:26PM +0100, Andrew Lunn wrote:
> On Tue, Feb 18, 2025 at 12:54:29PM +0100, Marek Behún wrote:
> > > +#define MDIO_MMD_AN_MV_STAT				32769
> > 
> > Why the hell are register addresses in this driver in decimal?
> 
> Maybe because 802.3 uses decimal? Take a look at Table 45–3—PMA/PMD
> registers for example.

Oh. Sorry about that, then :)

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Russell King (Oracle)]

On Tue, Feb 18, 2025 at 12:54:29PM +0100, Marek Behún wrote:
> > +#define MDIO_MMD_AN_MV_STAT				32769
> 
> Why the hell are register addresses in this driver in decimal?

Shocker - some documentation gives driver addresses for PHYs in
decimal.

It then becomes a pain to have to manually translate back and
forth between hex and decimal if one is reading the driver code and
referring back to the documentation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/3] net: phy: marvell-88q2xxx: align defines

[Dimitri Fedrau]

Am Tue, Feb 18, 2025 at 04:00:42PM +0000 schrieb Russell King (Oracle):
> On Tue, Feb 18, 2025 at 12:54:29PM +0100, Marek Behún wrote:
> > > +#define MDIO_MMD_AN_MV_STAT				32769
> > 
> > Why the hell are register addresses in this driver in decimal?
> 
> Shocker - some documentation gives driver addresses for PHYs in
> decimal.
> 
> It then becomes a pain to have to manually translate back and
> forth between hex and decimal if one is reading the driver code and
> referring back to the documentation.
>
Agreed. Is it worth to change addresses to hexadecimal numbers and use
BIT and GENMASK for the bits ?

Best regards,
Dimitri Fedrau

[PATCH net-next 2/3] net: phy: marvell-88q2xxx: order includes alphabetically

[Dimitri Fedrau]

Order includes alphabetically.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 6e95de080bc65e8e8543d4effb9846fdd823a9d4..7b0913968bb404df1d271b040e698a4c8c391705 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -7,10 +7,10 @@
  * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
  */
 #include <linux/ethtool_netlink.h>
+#include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
 #include <linux/of.h>
 #include <linux/phy.h>
-#include <linux/hwmon.h>
 
 #define PHY_ID_88Q2220_REVB0				(MARVELL_PHY_ID_88Q2220 | 0x1)
 #define PHY_ID_88Q2220_REVB1				(MARVELL_PHY_ID_88Q2220 | 0x2)

-- 
2.39.5

Re: [PATCH net-next 2/3] net: phy: marvell-88q2xxx: order includes alphabetically

[Niklas Söderlund]

Hi Dimitri,

Thanks for your patch.

On 2025-02-14 17:32:04 +0100, Dimitri Fedrau wrote:
> Order includes alphabetically.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/phy/marvell-88q2xxx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 6e95de080bc65e8e8543d4effb9846fdd823a9d4..7b0913968bb404df1d271b040e698a4c8c391705 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -7,10 +7,10 @@
>   * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
>   */
>  #include <linux/ethtool_netlink.h>
> +#include <linux/hwmon.h>
>  #include <linux/marvell_phy.h>
>  #include <linux/of.h>
>  #include <linux/phy.h>
> -#include <linux/hwmon.h>
>  
>  #define PHY_ID_88Q2220_REVB0				(MARVELL_PHY_ID_88Q2220 | 0x1)
>  #define PHY_ID_88Q2220_REVB1				(MARVELL_PHY_ID_88Q2220 | 0x2)
> 
> -- 
> 2.39.5
> 

-- 
Kind Regards,
Niklas Söderlund

[PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Dimitri Fedrau]

Temperature sensor gets enabled for 88Q222X devices in
mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
all 88Q2XXX devices support the temperature sensor.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 7b0913968bb404df1d271b040e698a4c8c391705..1859db10b3914f54486c7e6132b10b0353fa49e6 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -513,6 +513,15 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
+	/* Enable temperature sense */
+	if (priv->enable_temp) {
+		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
+				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+				     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -903,18 +912,6 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
 
 static int mv88q222x_config_init(struct phy_device *phydev)
 {
-	struct mv88q2xxx_priv *priv = phydev->priv;
-	int ret;
-
-	/* Enable temperature sense */
-	if (priv->enable_temp) {
-		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
-				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
-				     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
-		if (ret < 0)
-			return ret;
-	}
-
 	if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
 		return mv88q222x_revb0_config_init(phydev);
 	else

-- 
2.39.5

Re: [PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Niklas Söderlund]

Hi Dimitir,

Thanks for your work.

On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote:
> Temperature sensor gets enabled for 88Q222X devices in
> mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> all 88Q2XXX devices support the temperature sensor.

Is this true for mv88q2110 devices too? The current implementation only 
enables it for mv88q222x devices. The private structure is not even 
initialized for mv88q2110, and currently crashes. I have fixed that [1], 
but I'm not sure if that should be extended to also enable temperature 
sensor for mv88q2110?

> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

In either case with [1] for an unrelated fix this is tested on 
mv88q2110.

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

1.  https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/

> ---
>  drivers/net/phy/marvell-88q2xxx.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 7b0913968bb404df1d271b040e698a4c8c391705..1859db10b3914f54486c7e6132b10b0353fa49e6 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -513,6 +513,15 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> +	/* Enable temperature sense */
> +	if (priv->enable_temp) {
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> +				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> +				     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -903,18 +912,6 @@ static int mv88q222x_revb1_revb2_config_init(struct phy_device *phydev)
>  
>  static int mv88q222x_config_init(struct phy_device *phydev)
>  {
> -	struct mv88q2xxx_priv *priv = phydev->priv;
> -	int ret;
> -
> -	/* Enable temperature sense */
> -	if (priv->enable_temp) {
> -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> -				     MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> -				     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	if (phydev->c45_ids.device_ids[MDIO_MMD_PMAPMD] == PHY_ID_88Q2220_REVB0)
>  		return mv88q222x_revb0_config_init(phydev);
>  	else
> 
> -- 
> 2.39.5
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Dimitri Fedrau]

Hi Niklas,

Am Fri, Feb 14, 2025 at 06:59:38PM +0100 schrieb Niklas Söderlund:
> Hi Dimitir,
> 
> Thanks for your work.
> 
> On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote:
> > Temperature sensor gets enabled for 88Q222X devices in
> > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> > all 88Q2XXX devices support the temperature sensor.
> 
> Is this true for mv88q2110 devices too? The current implementation only 
> enables it for mv88q222x devices. The private structure is not even 
> initialized for mv88q2110, and currently crashes. I have fixed that [1], 
> but I'm not sure if that should be extended to also enable temperature 
> sensor for mv88q2110?
> 
Yes, according to the datasheet. I don't have a mv88q2110 device, so I
can't test it. I would like to see it enabled. So if you can test it and
it works why not enabling it. Thanks for finding this.
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> 
> In either case with [1] for an unrelated fix this is tested on 
> mv88q2110.
> 
> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> 1.  https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/
>
[...]

Best regards,
Dimitri Fedrau

Re: [PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Niklas Söderlund]

Hi,

On 2025-02-14 20:45:26 +0100, Dimitri Fedrau wrote:
> Hi Niklas,
> 
> Am Fri, Feb 14, 2025 at 06:59:38PM +0100 schrieb Niklas Söderlund:
> > Hi Dimitir,
> > 
> > Thanks for your work.
> > 
> > On 2025-02-14 17:32:05 +0100, Dimitri Fedrau wrote:
> > > Temperature sensor gets enabled for 88Q222X devices in
> > > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> > > all 88Q2XXX devices support the temperature sensor.
> > 
> > Is this true for mv88q2110 devices too? The current implementation only 
> > enables it for mv88q222x devices. The private structure is not even 
> > initialized for mv88q2110, and currently crashes. I have fixed that [1], 
> > but I'm not sure if that should be extended to also enable temperature 
> > sensor for mv88q2110?
> > 
> Yes, according to the datasheet. I don't have a mv88q2110 device, so I
> can't test it. I would like to see it enabled. So if you can test it and
> it works why not enabling it. Thanks for finding this.

Thanks for confirming. I will attempt to test and enable this once as 
soon as I can.

> > > 
> > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> > 
> > In either case with [1] for an unrelated fix this is tested on 
> > mv88q2110.
> > 
> > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > 1.  https://lore.kernel.org/all/20250214174650.2056949-1-niklas.soderlund+renesas@ragnatech.se/
> >
> [...]
> 
> Best regards,
> Dimitri Fedrau

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Andrew Lunn]

On Fri, Feb 14, 2025 at 05:32:05PM +0100, Dimitri Fedrau wrote:
> Temperature sensor gets enabled for 88Q222X devices in
> mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> all 88Q2XXX devices support the temperature sensor.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

The change itself looks fine:

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

but is the sensor actually usable if the PHY has not yet been
configured?

Architecturally, it seems odd to register the HWMON in probe, and then
enable it in config_init.

    Andrew

Re: [PATCH net-next 3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init

[Dimitri Fedrau]

Am Fri, Feb 14, 2025 at 07:06:22PM +0100 schrieb Andrew Lunn:
> On Fri, Feb 14, 2025 at 05:32:05PM +0100, Dimitri Fedrau wrote:
> > Temperature sensor gets enabled for 88Q222X devices in
> > mv88q222x_config_init. Move enabling to mv88q2xxx_config_init because
> > all 88Q2XXX devices support the temperature sensor.
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> 
> The change itself looks fine:
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> but is the sensor actually usable if the PHY has not yet been
> configured?
>
No.

> Architecturally, it seems odd to register the HWMON in probe, and then
> enable it in config_init.

I think it makes sense to enable it in probe as well. I just moved it to
config_init because of the PHYs hard reset. Before
https://lore.kernel.org/netdev/20250118-marvell-88q2xxx-fix-hwmon-v2-1-402e62ba2dcb@gmail.com/
it was already done in mv88q2xxx_hwmon_probe. I will come up with a patch.

Best regards,
Dimitri Fedrau

Re: [PATCH net-next 0/3] net: phy: marvell-88q2xxx: cleanup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 14 Feb 2025 17:32:02 +0100 you wrote:
> - align defines
> - order includes alphabetically
> - enable temperature sensor in mv88q2xxx_config_init
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
> Dimitri Fedrau (3):
>       net: phy: marvell-88q2xxx: align defines
>       net: phy: marvell-88q2xxx: order includes alphabetically
>       net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: phy: marvell-88q2xxx: align defines
    https://git.kernel.org/netdev/net-next/c/8dcaed624f6a
  - [net-next,2/3] net: phy: marvell-88q2xxx: order includes alphabetically
    https://git.kernel.org/netdev/net-next/c/cbe0449e8f9f
  - [net-next,3/3] net: phy: marvell-88q2xxx: enable temperature sensor in mv88q2xxx_config_init
    https://git.kernel.org/netdev/net-next/c/6c806720bafe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html