[PATCH net-next 0/8] net: phy: fix some coding-style issues

Make some cleanups according to the coding style of kernel.

Wenpeng Liang (8):
  net: phy: add a blank line after declarations
  net: phy: correct format of block comments
  net: phy: delete repeated word of block comments
  net: phy: fixed formatting issues with braces
  net: phy: fixed space alignment issues
  net: phy: print the function name by __func__ instead of an fixed
    string
  net: phy: remove unnecessary line continuation
  net: phy: use '__packed' instead of '__attribute__((__packed__))'

 drivers/net/phy/bcm87xx.c       |   4 +-
 drivers/net/phy/davicom.c       |   4 +-
 drivers/net/phy/dp83640.c       |   5 +-
 drivers/net/phy/et1011c.c       |  12 +--
 drivers/net/phy/fixed_phy.c     |   4 +-
 drivers/net/phy/lxt.c           |   6 +-
 drivers/net/phy/marvell.c       |   9 +--
 drivers/net/phy/mdio_bus.c      |   1 +
 drivers/net/phy/mdio_device.c   |   4 +-
 drivers/net/phy/mscc/mscc_ptp.h |   4 +-
 drivers/net/phy/national.c      |   6 +-
 drivers/net/phy/phy-c45.c       |   2 +-
 drivers/net/phy/phy-core.c      | 161 ++++++++++++++++++++--------------------
 drivers/net/phy/phy.c           |   3 +-
 drivers/net/phy/phy_device.c    |   9 +--
 drivers/net/phy/phylink.c       |  14 ++--
 drivers/net/phy/qsemi.c         |   1 +
 drivers/net/phy/sfp-bus.c       |  28 +++----
 drivers/net/phy/sfp.c           |   2 +-
 drivers/net/phy/spi_ks8995.c    |  10 +--
 drivers/net/phy/ste10Xp.c       |   6 +-
 drivers/net/phy/vitesse.c       |   3 +-
 22 files changed, 153 insertions(+), 145 deletions(-)

-- 
2.8.1

From: Wenpeng Liang <redacted>

Fix following format issues:
1. open brace '{' following function definitions should go to the next line.
2. braces {} are not necessary for single line statements.
3. else should follow close brace '}'.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/fixed_phy.c  | 4 ++--
 drivers/net/phy/marvell.c    | 9 ++++-----
 drivers/net/phy/phy.c        | 3 +--
 drivers/net/phy/phy_device.c | 9 ++++-----
 drivers/net/phy/phylink.c    | 5 ++---
 5 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 18d81f4..c65fb5f 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c

@@ -161,8 +161,8 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
 }
 
 int fixed_phy_add(unsigned int irq, int phy_addr,
-		  struct fixed_phy_status *status) {
-
+		  struct fixed_phy_status *status)
+{
 	return fixed_phy_add_gpiod(irq, phy_addr, status, NULL);
 }
 EXPORT_SYMBOL_GPL(fixed_phy_add);

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 23751d9..d93c27a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c

@@ -809,15 +809,14 @@ static int m88e1111_config_init_rgmii_delays(struct phy_device *phydev)
 {
 	int delay;
 
-	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		delay = MII_M1111_RGMII_RX_DELAY | MII_M1111_RGMII_TX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 		delay = MII_M1111_RGMII_RX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 		delay = MII_M1111_RGMII_TX_DELAY;
-	} else {
+	else
 		delay = 0;
-	}
 
 	return phy_modify(phydev, MII_M1111_PHY_EXT_CR,
 			  MII_M1111_RGMII_RX_DELAY | MII_M1111_RGMII_TX_DELAY,

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1089a93..8eeb26d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -380,8 +380,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 					else if (val & BMCR_SPEED100)
 						phydev->speed = SPEED_100;
 					else phydev->speed = SPEED_10;
-				}
-				else {
+				} else {
 					if (phydev->autoneg == AUTONEG_DISABLE)
 						change_autoneg = true;
 					phydev->autoneg = AUTONEG_ENABLE;

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea0..2b78c5c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -2904,15 +2904,14 @@ static int phy_probe(struct device *dev)
 	 * a controller will attach, and may modify one
 	 * or both of these values
 	 */
-	if (phydrv->features) {
+	if (phydrv->features)
 		linkmode_copy(phydev->supported, phydrv->features);
-	} else if (phydrv->get_features) {
+	else if (phydrv->get_features)
 		err = phydrv->get_features(phydev);
-	} else if (phydev->is_c45) {
+	else if (phydev->is_c45)
 		err = genphy_c45_pma_read_abilities(phydev);
-	} else {
+	else
 		err = genphy_read_abilities(phydev);
-	}
 
 	if (err)
 		goto out;

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9baaa34..4040d37 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -1361,11 +1361,10 @@ int phylink_ethtool_ksettings_get(struct phylink *pl,
 
 	ASSERT_RTNL();
 
-	if (pl->phydev) {
+	if (pl->phydev)
 		phy_ethtool_ksettings_get(pl->phydev, kset);
-	} else {
+	else
 		kset->base.port = pl->link_port;
-	}
 
 	linkmode_copy(kset->link_modes.supported, pl->supported);
 

-- 
2.8.1

From: Wenpeng Liang <redacted>

There should be a blank line after declarations.

Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/bcm87xx.c  | 4 ++--
 drivers/net/phy/dp83640.c  | 1 +
 drivers/net/phy/et1011c.c  | 6 ++++--
 drivers/net/phy/mdio_bus.c | 1 +
 drivers/net/phy/qsemi.c    | 1 +
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index 4ac8fd1..3135634 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c

@@ -54,9 +54,9 @@ static int bcm87xx_of_reg_init(struct phy_device *phydev)
 		u16 reg		= be32_to_cpup(paddr++);
 		u16 mask	= be32_to_cpup(paddr++);
 		u16 val_bits	= be32_to_cpup(paddr++);
-		int val;
 		u32 regnum = mdiobus_c45_addr(devid, reg);
-		val = 0;
+		int val = 0;
+
 		if (mask) {
 			val = phy_read(phydev, regnum);
 			if (val < 0) {

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 0d79f68..10769bf 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c

@@ -615,6 +615,7 @@ static void prune_rx_ts(struct dp83640_private *dp83640)
 static void enable_broadcast(struct phy_device *phydev, int init_page, int on)
 {
 	int val;
+
 	phy_write(phydev, PAGESEL, 0);
 	val = phy_read(phydev, PHYCR2);
 	if (on)

diff --git a/drivers/net/phy/et1011c.c b/drivers/net/phy/et1011c.c
index 09e07b9..4b3d035 100644
--- a/drivers/net/phy/et1011c.c
+++ b/drivers/net/phy/et1011c.c

@@ -46,7 +46,8 @@ MODULE_LICENSE("GPL");
 
 static int et1011c_config_aneg(struct phy_device *phydev)
 {
-	int ctl = 0;
+	int ctl;
+
 	ctl = phy_read(phydev, MII_BMCR);
 	if (ctl < 0)
 		return ctl;

@@ -60,9 +61,10 @@ static int et1011c_config_aneg(struct phy_device *phydev)
 
 static int et1011c_read_status(struct phy_device *phydev)
 {
+	static int speed;
 	int ret;
 	u32 val;
-	static int speed;
+
 	ret = genphy_read_status(phydev);
 
 	if (speed != phydev->speed) {

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6045ad3..2466567 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -175,6 +175,7 @@ EXPORT_SYMBOL(mdiobus_alloc_size);
 static void mdiobus_release(struct device *d)
 {
 	struct mii_bus *bus = to_mii_bus(d);
+
 	BUG_ON(bus->state != MDIOBUS_RELEASED &&
 	       /* for compatibility with error handling in drivers */
 	       bus->state != MDIOBUS_ALLOCATED);

diff --git a/drivers/net/phy/qsemi.c b/drivers/net/phy/qsemi.c
index d5c1aaa..30d15f7 100644
--- a/drivers/net/phy/qsemi.c
+++ b/drivers/net/phy/qsemi.c

@@ -100,6 +100,7 @@ static int qs6612_ack_interrupt(struct phy_device *phydev)
 static int qs6612_config_intr(struct phy_device *phydev)
 {
 	int err;
+
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
 		/* clear any interrupts before enabling them */
 		err = qs6612_ack_interrupt(phydev);

-- 
2.8.1

From: Wenpeng Liang <redacted>

There are some space related issues, including spaces at the start of the
line, before tabs, after open parenthesis and before close parenthesis.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/davicom.c    |   4 +-
 drivers/net/phy/phy-core.c   | 158 +++++++++++++++++++++----------------------
 drivers/net/phy/sfp-bus.c    |  28 ++++----
 drivers/net/phy/spi_ks8995.c |  10 +--
 drivers/net/phy/ste10Xp.c    |   6 +-
 5 files changed, 103 insertions(+), 103 deletions(-)

diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
index a3b3842c6..23aed41 100644
--- a/drivers/net/phy/davicom.c
+++ b/drivers/net/phy/davicom.c

@@ -43,10 +43,10 @@
 #define MII_DM9161_INTR_DPLX_CHANGE	0x0010
 #define MII_DM9161_INTR_SPD_CHANGE	0x0008
 #define MII_DM9161_INTR_LINK_CHANGE	0x0004
-#define MII_DM9161_INTR_INIT 		0x0000
+#define MII_DM9161_INTR_INIT		0x0000
 #define MII_DM9161_INTR_STOP	\
 (MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK \
- | MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)
+	| MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)
 #define MII_DM9161_INTR_CHANGE	\
 	(MII_DM9161_INTR_DPLX_CHANGE | \
 	 MII_DM9161_INTR_SPD_CHANGE | \

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 2870c33..c8d8ef8 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c

@@ -84,98 +84,98 @@ EXPORT_SYMBOL_GPL(phy_duplex_to_str);
 
 static const struct phy_setting settings[] = {
 	/* 400G */
-	PHY_SETTING( 400000, FULL, 400000baseCR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR8_ER8_FR8_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseCR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR4_Full		),
+	PHY_SETTING(400000, FULL, 400000baseCR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR8_ER8_FR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseCR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR4_Full),
 	/* 200G */
-	PHY_SETTING( 200000, FULL, 200000baseCR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseCR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR2_Full		),
+	PHY_SETTING(200000, FULL, 200000baseCR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseCR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR2_Full),
 	/* 100G */
-	PHY_SETTING( 100000, FULL, 100000baseCR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR4_ER4_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseSR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR_ER_FR_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR_Full		),
+	PHY_SETTING(100000, FULL, 100000baseCR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR4_ER4_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR_ER_FR_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR_Full),
 	/* 56G */
-	PHY_SETTING(  56000, FULL,  56000baseCR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseKR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseLR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseSR4_Full	  	),
+	PHY_SETTING(56000, FULL, 56000baseCR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseKR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseLR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseSR4_Full),
 	/* 50G */
-	PHY_SETTING(  50000, FULL,  50000baseCR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseCR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseLR_ER_FR_Full	),
-	PHY_SETTING(  50000, FULL,  50000baseDR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR_Full		),
+	PHY_SETTING(50000, FULL, 50000baseCR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseCR_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR_Full),
+	PHY_SETTING(50000, FULL, 50000baseLR_ER_FR_Full),
+	PHY_SETTING(50000, FULL, 50000baseDR_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR_Full),
 	/* 40G */
-	PHY_SETTING(  40000, FULL,  40000baseCR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseKR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseLR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseSR4_Full		),
+	PHY_SETTING(40000, FULL, 40000baseCR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseKR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseLR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseSR4_Full),
 	/* 25G */
-	PHY_SETTING(  25000, FULL,  25000baseCR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseKR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseSR_Full		),
+	PHY_SETTING(25000, FULL, 25000baseCR_Full),
+	PHY_SETTING(25000, FULL, 25000baseKR_Full),
+	PHY_SETTING(25000, FULL, 25000baseSR_Full),
 	/* 20G */
-	PHY_SETTING(  20000, FULL,  20000baseKR2_Full		),
-	PHY_SETTING(  20000, FULL,  20000baseMLD2_Full		),
+	PHY_SETTING(20000, FULL, 20000baseKR2_Full),
+	PHY_SETTING(20000, FULL, 20000baseMLD2_Full),
 	/* 10G */
-	PHY_SETTING(  10000, FULL,  10000baseCR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseER_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKX4_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLRM_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseR_FEC		),
-	PHY_SETTING(  10000, FULL,  10000baseSR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseT_Full		),
+	PHY_SETTING(10000, FULL, 10000baseCR_Full),
+	PHY_SETTING(10000, FULL, 10000baseER_Full),
+	PHY_SETTING(10000, FULL, 10000baseKR_Full),
+	PHY_SETTING(10000, FULL, 10000baseKX4_Full),
+	PHY_SETTING(10000, FULL, 10000baseLR_Full),
+	PHY_SETTING(10000, FULL, 10000baseLRM_Full),
+	PHY_SETTING(10000, FULL, 10000baseR_FEC),
+	PHY_SETTING(10000, FULL, 10000baseSR_Full),
+	PHY_SETTING(10000, FULL, 10000baseT_Full),
 	/* 5G */
-	PHY_SETTING(   5000, FULL,   5000baseT_Full		),
+	PHY_SETTING(5000, FULL, 5000baseT_Full),
 	/* 2.5G */
-	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
-	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
+	PHY_SETTING(2500, FULL, 2500baseT_Full),
+	PHY_SETTING(2500, FULL, 2500baseX_Full),
 	/* 1G */
-	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
-	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
-	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
+	PHY_SETTING(1000, FULL, 1000baseKX_Full),
+	PHY_SETTING(1000, FULL, 1000baseT_Full),
+	PHY_SETTING(1000, HALF, 1000baseT_Half),
+	PHY_SETTING(1000, FULL, 1000baseT1_Full),
+	PHY_SETTING(1000, FULL, 1000baseX_Full),
 	/* 100M */
-	PHY_SETTING(    100, FULL,    100baseT_Full		),
-	PHY_SETTING(    100, FULL,    100baseT1_Full		),
-	PHY_SETTING(    100, HALF,    100baseT_Half		),
-	PHY_SETTING(    100, HALF,    100baseFX_Half		),
-	PHY_SETTING(    100, FULL,    100baseFX_Full		),
+	PHY_SETTING(100, FULL, 100baseT_Full),
+	PHY_SETTING(100, FULL, 100baseT1_Full),
+	PHY_SETTING(100, HALF, 100baseT_Half),
+	PHY_SETTING(100, HALF, 100baseFX_Half),
+	PHY_SETTING(100, FULL, 100baseFX_Full),
 	/* 10M */
-	PHY_SETTING(     10, FULL,     10baseT_Full		),
-	PHY_SETTING(     10, HALF,     10baseT_Half		),
+	PHY_SETTING(10, FULL, 10baseT_Full),
+	PHY_SETTING(10, HALF, 10baseT_Half),
 };
 #undef PHY_SETTING
 

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index e61de66..fe23fd3 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c

@@ -624,14 +624,14 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
  * be put via sfp_bus_put() when done.
  *
  * Returns:
- * 	    - on success, a pointer to the sfp_bus structure,
- *	    - %NULL if no SFP is specified,
- * 	    - on failure, an error pointer value:
+ *	- on success, a pointer to the sfp_bus structure,
+ *	- %NULL if no SFP is specified,
+ *	- on failure, an error pointer value:
  *
- * 	      - corresponding to the errors detailed for
- * 	        fwnode_property_get_reference_args().
- * 	      - %-ENOMEM if we failed to allocate the bus.
- *	      - an error from the upstream's connect_phy() method.
+ *	- corresponding to the errors detailed for
+ *	  fwnode_property_get_reference_args().
+ *	- %-ENOMEM if we failed to allocate the bus.
+ *	- an error from the upstream's connect_phy() method.
  */
 struct sfp_bus *sfp_bus_find_fwnode(struct fwnode_handle *fwnode)
 {

@@ -666,14 +666,14 @@ EXPORT_SYMBOL_GPL(sfp_bus_find_fwnode);
  * bus, so it is safe to put the bus after this call.
  *
  * Returns:
- * 	    - on success, a pointer to the sfp_bus structure,
- *	    - %NULL if no SFP is specified,
- * 	    - on failure, an error pointer value:
+ *	- on success, a pointer to the sfp_bus structure,
+ *	- %NULL if no SFP is specified,
+ *	- on failure, an error pointer value:
  *
- * 	      - corresponding to the errors detailed for
- * 	        fwnode_property_get_reference_args().
- * 	      - %-ENOMEM if we failed to allocate the bus.
- *	      - an error from the upstream's connect_phy() method.
+ *	- corresponding to the errors detailed for
+ *	  fwnode_property_get_reference_args().
+ *	- %-ENOMEM if we failed to allocate the bus.
+ *	- an error from the upstream's connect_phy() method.
  */
 int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 			 const struct sfp_upstream_ops *ops)

diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c
index ca49c1a..8b5445a 100644
--- a/drivers/net/phy/spi_ks8995.c
+++ b/drivers/net/phy/spi_ks8995.c

@@ -160,11 +160,11 @@ static const struct spi_device_id ks8995_id[] = {
 MODULE_DEVICE_TABLE(spi, ks8995_id);
 
 static const struct of_device_id ks8895_spi_of_match[] = {
-        { .compatible = "micrel,ks8995" },
-        { .compatible = "micrel,ksz8864" },
-        { .compatible = "micrel,ksz8795" },
-        { },
- };
+	{ .compatible = "micrel,ks8995" },
+	{ .compatible = "micrel,ksz8864" },
+	{ .compatible = "micrel,ksz8795" },
+	{ },
+};
 MODULE_DEVICE_TABLE(of, ks8895_spi_of_match);
 
 static inline u8 get_chip_id(u8 val)

diff --git a/drivers/net/phy/ste10Xp.c b/drivers/net/phy/ste10Xp.c
index 431fe5e..309e4c3 100644
--- a/drivers/net/phy/ste10Xp.c
+++ b/drivers/net/phy/ste10Xp.c

@@ -20,12 +20,12 @@
 #include <linux/mii.h>
 #include <linux/phy.h>
 
-#define MII_XCIIS   	0x11	/* Configuration Info IRQ & Status Reg */
-#define MII_XIE     	0x12	/* Interrupt Enable Register */
+#define MII_XCIIS	0x11	/* Configuration Info IRQ & Status Reg */
+#define MII_XIE		0x12	/* Interrupt Enable Register */
 #define MII_XIE_DEFAULT_MASK 0x0070 /* ANE complete, Remote Fault, Link Down */
 
 #define STE101P_PHY_ID		0x00061c50
-#define STE100P_PHY_ID       	0x1c040011
+#define STE100P_PHY_ID		0x1c040011
 
 static int ste10Xp_config_init(struct phy_device *phydev)
 {

-- 
2.8.1

From: Wenpeng Liang <redacted>

Prefer __packed over __attribute__((__packed__)).

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/mscc/mscc_ptp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h
index da34653..01f78b4 100644
--- a/drivers/net/phy/mscc/mscc_ptp.h
+++ b/drivers/net/phy/mscc/mscc_ptp.h

@@ -450,14 +450,14 @@ struct vsc85xx_ptphdr {
 	__be16 seq_id;
 	u8 ctrl;
 	u8 log_interval;
-} __attribute__((__packed__));
+} __packed;
 
 /* Represents an entry in the timestamping FIFO */
 struct vsc85xx_ts_fifo {
 	u32 ns;
 	u64 secs:48;
 	u8 sig[16];
-} __attribute__((__packed__));
+} __packed;
 
 struct vsc85xx_ptp {
 	struct phy_device *phydev;

-- 
2.8.1

From: Wenpeng Liang <redacted>

Block comments should not use a trailing */ on a separate line and every
line of a block comment should start with an '*'.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/lxt.c      | 6 +++---
 drivers/net/phy/national.c | 6 ++++--
 drivers/net/phy/phy-core.c | 3 ++-
 drivers/net/phy/phylink.c  | 9 ++++++---
 drivers/net/phy/vitesse.c  | 3 ++-
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index bde3356..5e00910 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c

@@ -241,9 +241,9 @@ static int lxt973a2_read_status(struct phy_device *phydev)
 			if (lpa < 0)
 				return lpa;
 
-			/* If both registers are equal, it is suspect but not
-			* impossible, hence a new try
-			*/
+		/* If both registers are equal, it is suspect but not
+		 * impossible, hence a new try
+		 */
 		} while (lpa == adv && retry--);
 
 		mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);

diff --git a/drivers/net/phy/national.c b/drivers/net/phy/national.c
index 46160ba..9ae9cc6b 100644
--- a/drivers/net/phy/national.c
+++ b/drivers/net/phy/national.c

@@ -68,7 +68,8 @@ static int ns_ack_interrupt(struct phy_device *phydev)
 		return ret;
 
 	/* Clear the interrupt status bit by writing a “1”
-	 * to the corresponding bit in INT_CLEAR (2:0 are reserved) */
+	 * to the corresponding bit in INT_CLEAR (2:0 are reserved)
+	 */
 	ret = phy_write(phydev, DP83865_INT_CLEAR, ret & ~0x7);
 
 	return ret;

@@ -150,7 +151,8 @@ static int ns_config_init(struct phy_device *phydev)
 {
 	ns_giga_speed_fallback(phydev, ALL_FALLBACK_ON);
 	/* In the latest MAC or switches design, the 10 Mbps loopback
-	   is desired to be turned off. */
+	 * is desired to be turned off.
+	 */
 	ns_10_base_t_hdx_loopack(phydev, hdx_loopback_off);
 	return ns_ack_interrupt(phydev);
 }

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 8d333d3..2870c33 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c

@@ -76,7 +76,8 @@ EXPORT_SYMBOL_GPL(phy_duplex_to_str);
 
 /* A mapping of all SUPPORTED settings to speed/duplex.  This table
  * must be grouped by speed and sorted in descending match priority
- * - iow, descending speed. */
+ * - iow, descending speed.
+ */
 
 #define PHY_SETTING(s, d, b) { .speed = SPEED_ ## s, .duplex = DUPLEX_ ## d, \
 			       .bit = ETHTOOL_LINK_MODE_ ## b ## _BIT}

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 96d8e88..9baaa34 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -181,7 +181,8 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 			pl->link_config.duplex = DUPLEX_FULL;
 
 		/* We treat the "pause" and "asym-pause" terminology as
-		 * defining the link partner's ability. */
+		 * defining the link partner's ability.
+		 */
 		if (fwnode_property_read_bool(fixed_node, "pause"))
 			__set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 				  pl->link_config.lp_advertising);

@@ -679,7 +680,8 @@ static void phylink_resolve(struct work_struct *w)
 			phylink_mac_pcs_get_state(pl, &link_state);
 
 			/* If we have a phy, the "up" state is the union of
-			 * both the PHY and the MAC */
+			 * both the PHY and the MAC
+			 */
 			if (pl->phydev)
 				link_state.link &= pl->phy_state.link;
 

@@ -688,7 +690,8 @@ static void phylink_resolve(struct work_struct *w)
 				link_state.interface = pl->phy_state.interface;
 
 				/* If we have a PHY, we need to update with
-				 * the PHY flow control bits. */
+				 * the PHY flow control bits.
+				 */
 				link_state.pause = pl->phy_state.pause;
 				mac_config = true;
 			}

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 16704e2..897b979 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c

@@ -249,7 +249,8 @@ static int vsc73xx_config_aneg(struct phy_device *phydev)
 
 /* This adds a skew for both TX and RX clocks, so the skew should only be
  * applied to "rgmii-id" interfaces. It may not work as expected
- * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */
+ * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces.
+ */
 static int vsc8601_add_skew(struct phy_device *phydev)
 {
 	int ret;

-- 
2.8.1

From: Wenpeng Liang <redacted>

Avoid unnecessary line continuations.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/dp83640.c | 4 ++--
 drivers/net/phy/et1011c.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 10769bf..705c166 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c

@@ -170,9 +170,9 @@ static ushort gpio_tab[GPIO_TABLE_SIZE] = {
 module_param(chosen_phy, int, 0444);
 module_param_array(gpio_tab, ushort, NULL, 0444);
 
-MODULE_PARM_DESC(chosen_phy, \
+MODULE_PARM_DESC(chosen_phy,
 	"The address of the PHY to use for the ancillary clock features");
-MODULE_PARM_DESC(gpio_tab, \
+MODULE_PARM_DESC(gpio_tab,
 	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
 
 static void dp83640_gpio_defaults(struct ptp_pin_desc *pd)

diff --git a/drivers/net/phy/et1011c.c b/drivers/net/phy/et1011c.c
index 4b3d035..72f51b7 100644
--- a/drivers/net/phy/et1011c.c
+++ b/drivers/net/phy/et1011c.c

@@ -74,9 +74,9 @@ static int et1011c_read_status(struct phy_device *phydev)
 					ET1011C_GIGABIT_SPEED) {
 			val = phy_read(phydev, ET1011C_CONFIG_REG);
 			val &= ~ET1011C_TX_FIFO_MASK;
-			phy_write(phydev, ET1011C_CONFIG_REG, val\
-					| ET1011C_GMII_INTERFACE\
-					| ET1011C_SYS_CLK_EN\
+			phy_write(phydev, ET1011C_CONFIG_REG, val
+					| ET1011C_GMII_INTERFACE
+					| ET1011C_SYS_CLK_EN
 					| ET1011C_TX_FIFO_DEPTH_16);
 
 		}

-- 
2.8.1

From: Wenpeng Liang <redacted>

It's better to use __func__ than a fixed string to print a
function's name.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/mdio_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 0837319..c94cb53 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c

@@ -77,7 +77,7 @@ int mdio_device_register(struct mdio_device *mdiodev)
 {
 	int err;
 
-	dev_dbg(&mdiodev->dev, "mdio_device_register\n");
+	dev_dbg(&mdiodev->dev, "%s\n", __func__);
 
 	err = mdiobus_register_device(mdiodev);
 	if (err)

@@ -188,7 +188,7 @@ int mdio_driver_register(struct mdio_driver *drv)
 	struct mdio_driver_common *mdiodrv = &drv->mdiodrv;
 	int retval;
 
-	pr_debug("mdio_driver_register: %s\n", mdiodrv->driver.name);
+	pr_debug("%s: %s\n", __func__, mdiodrv->driver.name);
 
 	mdiodrv->driver.bus = &mdio_bus_type;
 	mdiodrv->driver.probe = mdio_probe;

-- 
2.8.1

From: Wenpeng Liang <redacted>

Fix syntax errors in block comments.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/phy-c45.c | 2 +-
 drivers/net/phy/sfp.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index f4816b7..c617dbc 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c

@@ -172,7 +172,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_config_aneg);
  * @phydev: target phy_device struct
  *
  * Disable auto-negotiation in the Clause 45 PHY. The link parameters
- * parameters are controlled through the PMA/PMD MMD registers.
+ * are controlled through the PMA/PMD MMD registers.
  *
  * Returns zero on success, negative errno code on failure.
  */

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 37f722c..34e9021 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -2153,7 +2153,7 @@ static void sfp_sm_main(struct sfp *sfp, unsigned int event)
 
 	case SFP_S_INIT:
 		if (event == SFP_E_TIMEOUT && sfp->state & SFP_F_TX_FAULT) {
-			/* TX_FAULT is still asserted after t_init or
+			/* TX_FAULT is still asserted after t_init
 			 * or t_start_up, so assume there is a fault.
 			 */
 			sfp_sm_fault(sfp, SFP_S_INIT_TX_FAULT,

-- 
2.8.1

On Fri, Jun 11, 2021 at 02:36:52PM +0800, Weihang Li wrote:

This does a little bit more than add a blank line. Please mention it
in the commit message.

This is to do with trust. If you say you are just added blank lines,
the review can be very quick because you cannot break anything with
just blank lines. But as soon as i see more than just blank lines, i
can no longer trust your description, and i need to look much harder
at your changes.

Since you made this change, you could go one step further

	int ctl = phy_read(phydev, MII_BMCR);

This is an O.K. change, but again, more than adding a blank line.

     Andrew

On Fri, Jun 11, 2021 at 02:36:53PM +0800, Weihang Li wrote:

Please indicate in the commit message why you changed the indentation.

       Andrew

On Fri, Jun 11, 2021 at 02:36:54PM +0800, Weihang Li wrote:

From: Wenpeng Liang <redacted>

Fix syntax errors in block comments.

I supposed double words could be considered syntax errors, but it is
pushing the definition a bit.

	Andrew

-	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		delay = MII_M1111_RGMII_RX_DELAY | MII_M1111_RGMII_TX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 		delay = MII_M1111_RGMII_RX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 		delay = MII_M1111_RGMII_TX_DELAY;
-	} else {
+	else
 		delay = 0;
-	}

Or turn it into a switch statement?

   Andrew

 (MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK \
- | MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)
+	| MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)

The convention is to put the | at the end of the line. So

  (MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK | \
   MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)

 
 static const struct phy_setting settings[] = {
 	/* 400G */
-	PHY_SETTING( 400000, FULL, 400000baseCR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR8_ER8_FR8_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseCR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR4_Full		),
+	PHY_SETTING(400000, FULL, 400000baseCR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR8_ER8_FR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseCR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR4_Full),
 	/* 200G */
-	PHY_SETTING( 200000, FULL, 200000baseCR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseCR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR2_Full		),
+	PHY_SETTING(200000, FULL, 200000baseCR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseCR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR2_Full),
 	/* 100G */
-	PHY_SETTING( 100000, FULL, 100000baseCR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR4_ER4_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseSR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR_ER_FR_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR_Full		),
+	PHY_SETTING(100000, FULL, 100000baseCR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR4_ER4_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR_ER_FR_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR_Full),
 	/* 56G */
-	PHY_SETTING(  56000, FULL,  56000baseCR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseKR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseLR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseSR4_Full	  	),
+	PHY_SETTING(56000, FULL, 56000baseCR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseKR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseLR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseSR4_Full),
 	/* 50G */
-	PHY_SETTING(  50000, FULL,  50000baseCR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseCR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseLR_ER_FR_Full	),
-	PHY_SETTING(  50000, FULL,  50000baseDR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR_Full		),
+	PHY_SETTING(50000, FULL, 50000baseCR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseCR_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR_Full),
+	PHY_SETTING(50000, FULL, 50000baseLR_ER_FR_Full),
+	PHY_SETTING(50000, FULL, 50000baseDR_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR_Full),
 	/* 40G */
-	PHY_SETTING(  40000, FULL,  40000baseCR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseKR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseLR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseSR4_Full		),
+	PHY_SETTING(40000, FULL, 40000baseCR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseKR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseLR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseSR4_Full),
 	/* 25G */
-	PHY_SETTING(  25000, FULL,  25000baseCR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseKR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseSR_Full		),
+	PHY_SETTING(25000, FULL, 25000baseCR_Full),
+	PHY_SETTING(25000, FULL, 25000baseKR_Full),
+	PHY_SETTING(25000, FULL, 25000baseSR_Full),
 	/* 20G */
-	PHY_SETTING(  20000, FULL,  20000baseKR2_Full		),
-	PHY_SETTING(  20000, FULL,  20000baseMLD2_Full		),
+	PHY_SETTING(20000, FULL, 20000baseKR2_Full),
+	PHY_SETTING(20000, FULL, 20000baseMLD2_Full),
 	/* 10G */
-	PHY_SETTING(  10000, FULL,  10000baseCR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseER_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKX4_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLRM_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseR_FEC		),
-	PHY_SETTING(  10000, FULL,  10000baseSR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseT_Full		),
+	PHY_SETTING(10000, FULL, 10000baseCR_Full),
+	PHY_SETTING(10000, FULL, 10000baseER_Full),
+	PHY_SETTING(10000, FULL, 10000baseKR_Full),
+	PHY_SETTING(10000, FULL, 10000baseKX4_Full),
+	PHY_SETTING(10000, FULL, 10000baseLR_Full),
+	PHY_SETTING(10000, FULL, 10000baseLRM_Full),
+	PHY_SETTING(10000, FULL, 10000baseR_FEC),
+	PHY_SETTING(10000, FULL, 10000baseSR_Full),
+	PHY_SETTING(10000, FULL, 10000baseT_Full),
 	/* 5G */
-	PHY_SETTING(   5000, FULL,   5000baseT_Full		),
+	PHY_SETTING(5000, FULL, 5000baseT_Full),
 	/* 2.5G */
-	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
-	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
+	PHY_SETTING(2500, FULL, 2500baseT_Full),
+	PHY_SETTING(2500, FULL, 2500baseX_Full),
 	/* 1G */
-	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
-	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
-	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
+	PHY_SETTING(1000, FULL, 1000baseKX_Full),
+	PHY_SETTING(1000, FULL, 1000baseT_Full),
+	PHY_SETTING(1000, HALF, 1000baseT_Half),
+	PHY_SETTING(1000, FULL, 1000baseT1_Full),
+	PHY_SETTING(1000, FULL, 1000baseX_Full),
 	/* 100M */
-	PHY_SETTING(    100, FULL,    100baseT_Full		),
-	PHY_SETTING(    100, FULL,    100baseT1_Full		),
-	PHY_SETTING(    100, HALF,    100baseT_Half		),
-	PHY_SETTING(    100, HALF,    100baseFX_Half		),
-	PHY_SETTING(    100, FULL,    100baseFX_Full		),
+	PHY_SETTING(100, FULL, 100baseT_Full),
+	PHY_SETTING(100, FULL, 100baseT1_Full),
+	PHY_SETTING(100, HALF, 100baseT_Half),
+	PHY_SETTING(100, HALF, 100baseFX_Half),
+	PHY_SETTING(100, FULL, 100baseFX_Full),
 	/* 10M */
-	PHY_SETTING(     10, FULL,     10baseT_Full		),
-	PHY_SETTING(     10, HALF,     10baseT_Half		),
+	PHY_SETTING(10, FULL, 10baseT_Full),
+	PHY_SETTING(10, HALF, 10baseT_Half),

Please do not change this. It is a deliberate design decision to add
these spaces here.

      Andrew

On Fri, Jun 11, 2021 at 02:36:57PM +0800, Weihang Li wrote:

It would be nice to make this

        dev_dbg(&mdiodev->dev, "%s: %s\n", __func__, mdiodrv->driver.name);

	Andrew

-			phy_write(phydev, ET1011C_CONFIG_REG, val\
-					| ET1011C_GMII_INTERFACE\
-					| ET1011C_SYS_CLK_EN\
+			phy_write(phydev, ET1011C_CONFIG_REG, val
+					| ET1011C_GMII_INTERFACE
+					| ET1011C_SYS_CLK_EN
 					| ET1011C_TX_FIFO_DEPTH_16);

Please put the | at the end of the line, not the beginning of the next
line.

Thanks
	Andrew

On Fri, Jun 11, 2021 at 02:36:59PM +0800, Weihang Li wrote:

From: Wenpeng Liang <redacted>

Prefer __packed over __attribute__((__packed__)).

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>

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

    Andrew

From: Weihang Li

...

 /* Represents an entry in the timestamping FIFO */
 struct vsc85xx_ts_fifo {
 	u32 ns;
 	u64 secs:48;
 	u8 sig[16];
-} __attribute__((__packed__));
+} __packed;

Hmmmm I'd take some convincing that 'u64 secs:48' is anything
other than 'implementation defined'.
So using it to map a hardware structure seems wrong.

If this does map a hardware structure it ought to have
'endianness' annotations.
If it doesn't then why the bitfield and why packed?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On 2021/6/11 22:32, Andrew Lunn wrote:

On Fri, Jun 11, 2021 at 02:36:52PM +0800, Weihang Li wrote:

quoted

From: Wenpeng Liang <redacted>

There should be a blank line after declarations.

Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/bcm87xx.c  | 4 ++--
 drivers/net/phy/dp83640.c  | 1 +
 drivers/net/phy/et1011c.c  | 6 ++++--
 drivers/net/phy/mdio_bus.c | 1 +
 drivers/net/phy/qsemi.c    | 1 +
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index 4ac8fd1..3135634 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c

@@ -54,9 +54,9 @@ static int bcm87xx_of_reg_init(struct phy_device *phydev)
 		u16 reg		= be32_to_cpup(paddr++);
 		u16 mask	= be32_to_cpup(paddr++);
 		u16 val_bits	= be32_to_cpup(paddr++);
-		int val;
 		u32 regnum = mdiobus_c45_addr(devid, reg);
-		val = 0;
+		int val = 0;
+

This does a little bit more than add a blank line. Please mention it
in the commit message.

This is to do with trust. If you say you are just added blank lines,
the review can be very quick because you cannot break anything with
just blank lines. But as soon as i see more than just blank lines, i
can no longer trust your description, and i need to look much harder
at your changes.

I see, thanks for the patient guidance.

quoted

--- a/drivers/net/phy/et1011c.c
+++ b/drivers/net/phy/et1011c.c

@@ -46,7 +46,8 @@ MODULE_LICENSE("GPL");
 
 static int et1011c_config_aneg(struct phy_device *phydev)
 {
-	int ctl = 0;
+	int ctl;
+
 	ctl = phy_read(phydev, MII_BMCR);
 	if (ctl < 0)
 		return ctl;

Since you made this change, you could go one step further

	int ctl = phy_read(phydev, MII_BMCR);

Sure.

quoted

@@ -60,9 +61,10 @@ static int et1011c_config_aneg(struct phy_device *phydev)
 
 static int et1011c_read_status(struct phy_device *phydev)
 {
+	static int speed;
 	int ret;
 	u32 val;
-	static int speed;
+

This is an O.K. change, but again, more than adding a blank line.

     Andrew>

OK, I will describe that in the commit description.

Thanks
Weihang

On 2021/6/11 22:36, Andrew Lunn wrote:

On Fri, Jun 11, 2021 at 02:36:53PM +0800, Weihang Li wrote:

quoted

From: Wenpeng Liang <redacted>

Block comments should not use a trailing */ on a separate line and every
line of a block comment should start with an '*'.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/lxt.c      | 6 +++---
 drivers/net/phy/national.c | 6 ++++--
 drivers/net/phy/phy-core.c | 3 ++-
 drivers/net/phy/phylink.c  | 9 ++++++---
 drivers/net/phy/vitesse.c  | 3 ++-
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index bde3356..5e00910 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c

@@ -241,9 +241,9 @@ static int lxt973a2_read_status(struct phy_device *phydev)
 			if (lpa < 0)
 				return lpa;
 
-			/* If both registers are equal, it is suspect but not
-			* impossible, hence a new try
-			*/
+		/* If both registers are equal, it is suspect but not
+		 * impossible, hence a new try
+		 */
 		} while (lpa == adv && retry--);

Please indicate in the commit message why you changed the indentation.

       Andrew

Sure, thanks.

Weihang

On 2021/6/11 22:39, Andrew Lunn wrote:

On Fri, Jun 11, 2021 at 02:36:54PM +0800, Weihang Li wrote:

quoted

From: Wenpeng Liang <redacted>

Fix syntax errors in block comments.

I supposed double words could be considered syntax errors, but it is
pushing the definition a bit.

	Andrew

OK, I will use more appropriate words.

Thanks
Weihang

On 2021/6/11 23:30, Andrew Lunn wrote:

quoted

 (MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK \
- | MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)
+	| MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)

The convention is to put the | at the end of the line. So

  (MII_DM9161_INTR_DPLX_MASK | MII_DM9161_INTR_SPD_MASK | \
   MII_DM9161_INTR_LINK_MASK | MII_DM9161_INTR_MASK)

quoted

 #define MII_DM9161_INTR_CHANGE	\
 	(MII_DM9161_INTR_DPLX_CHANGE | \
 	 MII_DM9161_INTR_SPD_CHANGE | \

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 2870c33..c8d8ef8 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c

@@ -84,98 +84,98 @@ EXPORT_SYMBOL_GPL(phy_duplex_to_str);

quoted

 
 static const struct phy_setting settings[] = {
 	/* 400G */
-	PHY_SETTING( 400000, FULL, 400000baseCR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR8_ER8_FR8_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseCR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR4_Full		),
+	PHY_SETTING(400000, FULL, 400000baseCR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR8_ER8_FR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR8_Full),
+	PHY_SETTING(400000, FULL, 400000baseCR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseKR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseDR4_Full),
+	PHY_SETTING(400000, FULL, 400000baseSR4_Full),
 	/* 200G */
-	PHY_SETTING( 200000, FULL, 200000baseCR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseCR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR2_Full		),
+	PHY_SETTING(200000, FULL, 200000baseCR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR4_ER4_FR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR4_Full),
+	PHY_SETTING(200000, FULL, 200000baseCR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseKR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseDR2_Full),
+	PHY_SETTING(200000, FULL, 200000baseSR2_Full),
 	/* 100G */
-	PHY_SETTING( 100000, FULL, 100000baseCR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR4_ER4_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseSR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR_ER_FR_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR_Full		),
+	PHY_SETTING(100000, FULL, 100000baseCR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR4_ER4_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR4_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR2_ER2_FR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR2_Full),
+	PHY_SETTING(100000, FULL, 100000baseCR_Full),
+	PHY_SETTING(100000, FULL, 100000baseKR_Full),
+	PHY_SETTING(100000, FULL, 100000baseLR_ER_FR_Full),
+	PHY_SETTING(100000, FULL, 100000baseDR_Full),
+	PHY_SETTING(100000, FULL, 100000baseSR_Full),
 	/* 56G */
-	PHY_SETTING(  56000, FULL,  56000baseCR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseKR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseLR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseSR4_Full	  	),
+	PHY_SETTING(56000, FULL, 56000baseCR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseKR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseLR4_Full),
+	PHY_SETTING(56000, FULL, 56000baseSR4_Full),
 	/* 50G */
-	PHY_SETTING(  50000, FULL,  50000baseCR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseCR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseLR_ER_FR_Full	),
-	PHY_SETTING(  50000, FULL,  50000baseDR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR_Full		),
+	PHY_SETTING(50000, FULL, 50000baseCR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR2_Full),
+	PHY_SETTING(50000, FULL, 50000baseCR_Full),
+	PHY_SETTING(50000, FULL, 50000baseKR_Full),
+	PHY_SETTING(50000, FULL, 50000baseLR_ER_FR_Full),
+	PHY_SETTING(50000, FULL, 50000baseDR_Full),
+	PHY_SETTING(50000, FULL, 50000baseSR_Full),
 	/* 40G */
-	PHY_SETTING(  40000, FULL,  40000baseCR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseKR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseLR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseSR4_Full		),
+	PHY_SETTING(40000, FULL, 40000baseCR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseKR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseLR4_Full),
+	PHY_SETTING(40000, FULL, 40000baseSR4_Full),
 	/* 25G */
-	PHY_SETTING(  25000, FULL,  25000baseCR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseKR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseSR_Full		),
+	PHY_SETTING(25000, FULL, 25000baseCR_Full),
+	PHY_SETTING(25000, FULL, 25000baseKR_Full),
+	PHY_SETTING(25000, FULL, 25000baseSR_Full),
 	/* 20G */
-	PHY_SETTING(  20000, FULL,  20000baseKR2_Full		),
-	PHY_SETTING(  20000, FULL,  20000baseMLD2_Full		),
+	PHY_SETTING(20000, FULL, 20000baseKR2_Full),
+	PHY_SETTING(20000, FULL, 20000baseMLD2_Full),
 	/* 10G */
-	PHY_SETTING(  10000, FULL,  10000baseCR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseER_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKX4_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLRM_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseR_FEC		),
-	PHY_SETTING(  10000, FULL,  10000baseSR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseT_Full		),
+	PHY_SETTING(10000, FULL, 10000baseCR_Full),
+	PHY_SETTING(10000, FULL, 10000baseER_Full),
+	PHY_SETTING(10000, FULL, 10000baseKR_Full),
+	PHY_SETTING(10000, FULL, 10000baseKX4_Full),
+	PHY_SETTING(10000, FULL, 10000baseLR_Full),
+	PHY_SETTING(10000, FULL, 10000baseLRM_Full),
+	PHY_SETTING(10000, FULL, 10000baseR_FEC),
+	PHY_SETTING(10000, FULL, 10000baseSR_Full),
+	PHY_SETTING(10000, FULL, 10000baseT_Full),
 	/* 5G */
-	PHY_SETTING(   5000, FULL,   5000baseT_Full		),
+	PHY_SETTING(5000, FULL, 5000baseT_Full),
 	/* 2.5G */
-	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
-	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
+	PHY_SETTING(2500, FULL, 2500baseT_Full),
+	PHY_SETTING(2500, FULL, 2500baseX_Full),
 	/* 1G */
-	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
-	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
-	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
+	PHY_SETTING(1000, FULL, 1000baseKX_Full),
+	PHY_SETTING(1000, FULL, 1000baseT_Full),
+	PHY_SETTING(1000, HALF, 1000baseT_Half),
+	PHY_SETTING(1000, FULL, 1000baseT1_Full),
+	PHY_SETTING(1000, FULL, 1000baseX_Full),
 	/* 100M */
-	PHY_SETTING(    100, FULL,    100baseT_Full		),
-	PHY_SETTING(    100, FULL,    100baseT1_Full		),
-	PHY_SETTING(    100, HALF,    100baseT_Half		),
-	PHY_SETTING(    100, HALF,    100baseFX_Half		),
-	PHY_SETTING(    100, FULL,    100baseFX_Full		),
+	PHY_SETTING(100, FULL, 100baseT_Full),
+	PHY_SETTING(100, FULL, 100baseT1_Full),
+	PHY_SETTING(100, HALF, 100baseT_Half),
+	PHY_SETTING(100, HALF, 100baseFX_Half),
+	PHY_SETTING(100, FULL, 100baseFX_Full),
 	/* 10M */
-	PHY_SETTING(     10, FULL,     10baseT_Full		),
-	PHY_SETTING(     10, HALF,     10baseT_Half		),
+	PHY_SETTING(10, FULL, 10baseT_Full),
+	PHY_SETTING(10, HALF, 10baseT_Half),

Please do not change this. It is a deliberate design decision to add
these spaces here.

      Andrew

OK, I will drop this part from this patch, thank you.

Weihang

On 2021/6/12 0:05, Andrew Lunn wrote:

On Fri, Jun 11, 2021 at 02:36:57PM +0800, Weihang Li wrote:

quoted

From: Wenpeng Liang <redacted>

It's better to use __func__ than a fixed string to print a
function's name.

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/mdio_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 0837319..c94cb53 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c

@@ -77,7 +77,7 @@ int mdio_device_register(struct mdio_device *mdiodev)
 {
 	int err;
 
-	dev_dbg(&mdiodev->dev, "mdio_device_register\n");
+	dev_dbg(&mdiodev->dev, "%s\n", __func__);
 
 	err = mdiobus_register_device(mdiodev);
 	if (err)

@@ -188,7 +188,7 @@ int mdio_driver_register(struct mdio_driver *drv)
 	struct mdio_driver_common *mdiodrv = &drv->mdiodrv;
 	int retval;
 
-	pr_debug("mdio_driver_register: %s\n", mdiodrv->driver.name);
+	pr_debug("%s: %s\n", __func__, mdiodrv->driver.name);

It would be nice to make this

        dev_dbg(&mdiodev->dev, "%s: %s\n", __func__, mdiodrv->driver.name);

	Andrew

Thanks for the advice, I will change it.

Weihang

On 2021/6/12 0:07, Andrew Lunn wrote:

quoted

-			phy_write(phydev, ET1011C_CONFIG_REG, val\
-					| ET1011C_GMII_INTERFACE\
-					| ET1011C_SYS_CLK_EN\
+			phy_write(phydev, ET1011C_CONFIG_REG, val
+					| ET1011C_GMII_INTERFACE
+					| ET1011C_SYS_CLK_EN
 					| ET1011C_TX_FIFO_DEPTH_16);

Please put the | at the end of the line, not the beginning of the next
line.

Thanks
	Andrew

Sure, thank you.

Weihang

On 2021/6/14 22:28, David Laight wrote:

From: Weihang Li

quoted

Sent: 11 June 2021 07:37

Prefer __packed over __attribute__((__packed__)).

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/mscc/mscc_ptp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h
index da34653..01f78b4 100644

...

quoted

 /* Represents an entry in the timestamping FIFO */
 struct vsc85xx_ts_fifo {
 	u32 ns;
 	u64 secs:48;
 	u8 sig[16];
-} __attribute__((__packed__));
+} __packed;

Hmmmm I'd take some convincing that 'u64 secs:48' is anything
other than 'implementation defined'.
So using it to map a hardware structure seems wrong.

If this does map a hardware structure it ought to have
'endianness' annotations.
If it doesn't then why the bitfield and why packed?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Hi David,

Thank you for your attention. You are right, I found the contents of structure
vsc85xx_ts_fifo is got from hardware. But I'm not sure if any issues or warnings
will be introduced into this driver after just changing 'u64 secs:48' to '__be64
secs:48'.

Let's keep this patch as it is. I cc the developers of the code, maybe they
didn't realize it or had some reasons to define it like that.

Thanks
Weihang

On 2021/6/11 22:41, Andrew Lunn wrote:

quoted

-	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		delay = MII_M1111_RGMII_RX_DELAY | MII_M1111_RGMII_TX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 		delay = MII_M1111_RGMII_RX_DELAY;
-	} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 		delay = MII_M1111_RGMII_TX_DELAY;
-	} else {
+	else
 		delay = 0;
-	}

Or turn it into a switch statement?

   Andrew

Sure, I will put them into another patch.

Weihang

On 2021/6/12 0:05, Andrew Lunn wrote:

quoted

@@ -188,7 +188,7 @@ int mdio_driver_register(struct mdio_driver *drv)
 	struct mdio_driver_common *mdiodrv = &drv->mdiodrv;
 	int retval;
 
-	pr_debug("mdio_driver_register: %s\n", mdiodrv->driver.name);
+	pr_debug("%s: %s\n", __func__, mdiodrv->driver.name);

It would be nice to make this

        dev_dbg(&mdiodev->dev, "%s: %s\n", __func__, mdiodrv->driver.name);

	Andrew

There is no way to get mdiodev from a pointer to mdio_driver, I don't think
there's a direct relationship between them. So I will keep using pr_debug :)

Thanks
Weihang

From: liweihang

Sent: 16 June 2021 07:17

On 2021/6/14 22:28, David Laight wrote:

quoted

From: Weihang Li

quoted

Sent: 11 June 2021 07:37

Prefer __packed over __attribute__((__packed__)).

Signed-off-by: Wenpeng Liang <redacted>
Signed-off-by: Weihang Li <redacted>
---
 drivers/net/phy/mscc/mscc_ptp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h
index da34653..01f78b4 100644

...

quoted

 /* Represents an entry in the timestamping FIFO */
 struct vsc85xx_ts_fifo {
 	u32 ns;
 	u64 secs:48;
 	u8 sig[16];
-} __attribute__((__packed__));
+} __packed;

Hmmmm I'd take some convincing that 'u64 secs:48' is anything
other than 'implementation defined'.
So using it to map a hardware structure seems wrong.

If this does map a hardware structure it ought to have
'endianness' annotations.
If it doesn't then why the bitfield and why packed?

	David

Hi David,

Thank you for your attention. You are right, I found the contents of structure
vsc85xx_ts_fifo is got from hardware. But I'm not sure if any issues or warnings
will be introduced into this driver after just changing 'u64 secs:48' to '__be64
secs:48'.

I've just checked what this structure looks like - see https://godbolt.org/z/h4EqbMoso

Without any 'packed' annotations  'u64 secs:48' is aligned to an 8 byte
boundary, but is only 6 bytes wide (I don't use bitfields)
so the offset of 'sig' is 6 more than 'secs'.

But the size of the whole structure looks wrong.
I'd expect a hardware fifo so be a power of 2 big.
This one is 26 bytes (as above) or 28 bytes if the 'packed'
is only applied to 'secs' (which removed the 4 byte pad before
it while still allowing aligned 4-byte accesses to the structure.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)