Re: [PATCH net-next 1/8] net: phy: add a blank line after declarations

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