| Message ID | 1623393419-2521-2-git-send-email-liweihang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: fix some coding-style issues | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: f.fainelli@gmail.com bcm-kernel-feedback-list@broadcom.com linux@armlinux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Fri, Jun 11, 2021 at 02:36:52PM +0800, Weihang Li wrote: > From: Wenpeng Liang <liangwenpeng@huawei.com> > > There should be a blank line after declarations. > > Cc: Richard Cochran <richardcochran@gmail.com> > Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> > Signed-off-by: Weihang Li <liweihang@huawei.com> > --- > 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. > --- 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); > @@ -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
On 2021/6/11 22:32, Andrew Lunn wrote: > On Fri, Jun 11, 2021 at 02:36:52PM +0800, Weihang Li wrote: >> From: Wenpeng Liang <liangwenpeng@huawei.com> >> >> There should be a blank line after declarations. >> >> Cc: Richard Cochran <richardcochran@gmail.com> >> Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> >> Signed-off-by: Weihang Li <liweihang@huawei.com> >> --- >> 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. > >> --- 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. > >> @@ -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
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);