| Message ID | 20200903141510.20212-1-dmurphy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: phy: dp83867: Fix various styling and space issues | expand |
On 9/3/2020 7:15 AM, Dan Murphy wrote: > Fix spacing issues reported for misaligned switch..case and extra new > lines. > > Also updated the file header to comply with networking commet style. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/net/phy/dp83867.c | 47 ++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index cd7032628a28..f182a8d767c6 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -1,6 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0 > -/* > - * Driver for the Texas Instruments DP83867 PHY > +/* Driver for the Texas Instruments DP83867 PHY > * > * Copyright (C) 2015 Texas Instruments Inc. > */ > @@ -35,7 +34,7 @@ > #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) > #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) > #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) > -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) > +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) Now the definitions are inconsistent, you would want to drop this one and stick to the existing style. The rest of the changes look good, so with that fixed, and the subject correct to "net-next" (this is no bug fix material), you can add: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian On 9/3/20 11:34 AM, Florian Fainelli wrote: > > > On 9/3/2020 7:15 AM, Dan Murphy wrote: >> Fix spacing issues reported for misaligned switch..case and extra new >> lines. >> >> Also updated the file header to comply with networking commet style. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/net/phy/dp83867.c | 47 ++++++++++++++++++--------------------- >> 1 file changed, 22 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >> index cd7032628a28..f182a8d767c6 100644 >> --- a/drivers/net/phy/dp83867.c >> +++ b/drivers/net/phy/dp83867.c >> @@ -1,6 +1,5 @@ >> // SPDX-License-Identifier: GPL-2.0 >> -/* >> - * Driver for the Texas Instruments DP83867 PHY >> +/* Driver for the Texas Instruments DP83867 PHY >> * >> * Copyright (C) 2015 Texas Instruments Inc. >> */ >> @@ -35,7 +34,7 @@ >> #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) >> #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) >> #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) >> -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) >> +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) > > Now the definitions are inconsistent, you would want to drop this one > and stick to the existing style. OK I was a little conflicted making that change due to the reasons you mentioned. But if that is an acceptable warning I am ok with it. > > The rest of the changes look good, so with that fixed, and the subject > correct to "net-next" (this is no bug fix material), you can add: > I will have to reapply this to the net-next to make sure it applies cleanly there. But not an issue. Dan > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On 9/3/2020 9:41 AM, Dan Murphy wrote: > Florian > > On 9/3/20 11:34 AM, Florian Fainelli wrote: >> >> >> On 9/3/2020 7:15 AM, Dan Murphy wrote: >>> Fix spacing issues reported for misaligned switch..case and extra new >>> lines. >>> >>> Also updated the file header to comply with networking commet style. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> drivers/net/phy/dp83867.c | 47 ++++++++++++++++++--------------------- >>> 1 file changed, 22 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>> index cd7032628a28..f182a8d767c6 100644 >>> --- a/drivers/net/phy/dp83867.c >>> +++ b/drivers/net/phy/dp83867.c >>> @@ -1,6 +1,5 @@ >>> // SPDX-License-Identifier: GPL-2.0 >>> -/* >>> - * Driver for the Texas Instruments DP83867 PHY >>> +/* Driver for the Texas Instruments DP83867 PHY >>> * >>> * Copyright (C) 2015 Texas Instruments Inc. >>> */ >>> @@ -35,7 +34,7 @@ >>> #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) >>> #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) >>> #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) >>> -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) >>> +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) >> >> Now the definitions are inconsistent, you would want to drop this one >> and stick to the existing style. > > OK I was a little conflicted making that change due to the reasons you > mentioned. But if that is an acceptable warning I am ok with it. IMHO, if the are no obvious incorrect styles, and using 1 << x is not, it just may not be the preferred style (and there is quite frankly a ton of those patterns in the kernel), then ignoring the checkpatch.pl warning is fine. After all this is a tool, not an absolute truth by any means, but again, others may disagree.
On Thu, 2020-09-03 at 11:41 -0500, Dan Murphy wrote: > On 9/3/20 11:34 AM, Florian Fainelli wrote: > > On 9/3/2020 7:15 AM, Dan Murphy wrote: > > > Fix spacing issues reported for misaligned switch..case and extra new > > > lines. [] > > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c [] > > > @@ -35,7 +34,7 @@ > > > #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) > > > -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) > > > +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) > > Now the definitions are inconsistent, you would want to drop this one > > and stick to the existing style. > > OK I was a little conflicted making that change due to the reasons you > mentioned. But if that is an acceptable warning I am ok with it. Maybe use GENMASK for DP83867_CFG4_SGMII_ANEG_MASK instead.
> > > #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) > > > -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) > > > +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) > > > > Now the definitions are inconsistent, you would want to drop this one > > and stick to the existing style. > > OK I was a little conflicted making that change due to the reasons you > mentioned. But if that is an acceptable warning I am ok with it. Hi Dan I work around this by using hex: #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (0x3 << 5) #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (0x2 << 5) #define DP83867_CFG4_SGMII_ANEG_TIMER_2US (0x1 << 5) checkpatch does not complain about that. Andrew
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index cd7032628a28..f182a8d767c6 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -1,6 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -/* - * Driver for the Texas Instruments DP83867 PHY +/* Driver for the Texas Instruments DP83867 PHY * * Copyright (C) 2015 Texas Instruments Inc. */ @@ -35,7 +34,7 @@ #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US (1 << 5) +#define DP83867_CFG4_SGMII_ANEG_TIMER_2US BIT(5) #define DP83867_CFG4_SGMII_ANEG_TIMER_16MS (0 << 5) #define DP83867_RGMIICTL 0x0032 @@ -113,7 +112,6 @@ #define DP83867_RGMII_RX_CLK_DELAY_SHIFT 0 #define DP83867_RGMII_RX_CLK_DELAY_INV (DP83867_RGMII_RX_CLK_DELAY_MAX + 1) - /* IO_MUX_CFG bits */ #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK 0x1f #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX 0x0 @@ -384,22 +382,22 @@ static int dp83867_set_downshift(struct phy_device *phydev, u8 cnt) DP83867_DOWNSHIFT_EN); switch (cnt) { - case DP83867_DOWNSHIFT_1_COUNT: - count = DP83867_DOWNSHIFT_1_COUNT_VAL; - break; - case DP83867_DOWNSHIFT_2_COUNT: - count = DP83867_DOWNSHIFT_2_COUNT_VAL; - break; - case DP83867_DOWNSHIFT_4_COUNT: - count = DP83867_DOWNSHIFT_4_COUNT_VAL; - break; - case DP83867_DOWNSHIFT_8_COUNT: - count = DP83867_DOWNSHIFT_8_COUNT_VAL; - break; - default: - phydev_err(phydev, - "Downshift count must be 1, 2, 4 or 8\n"); - return -EINVAL; + case DP83867_DOWNSHIFT_1_COUNT: + count = DP83867_DOWNSHIFT_1_COUNT_VAL; + break; + case DP83867_DOWNSHIFT_2_COUNT: + count = DP83867_DOWNSHIFT_2_COUNT_VAL; + break; + case DP83867_DOWNSHIFT_4_COUNT: + count = DP83867_DOWNSHIFT_4_COUNT_VAL; + break; + case DP83867_DOWNSHIFT_8_COUNT: + count = DP83867_DOWNSHIFT_8_COUNT_VAL; + break; + default: + phydev_err(phydev, + "Downshift count must be 1, 2, 4 or 8\n"); + return -EINVAL; } val = DP83867_DOWNSHIFT_EN; @@ -411,7 +409,7 @@ static int dp83867_set_downshift(struct phy_device *phydev, u8 cnt) } static int dp83867_get_tunable(struct phy_device *phydev, - struct ethtool_tunable *tuna, void *data) + struct ethtool_tunable *tuna, void *data) { switch (tuna->id) { case ETHTOOL_PHY_DOWNSHIFT: @@ -422,7 +420,7 @@ static int dp83867_get_tunable(struct phy_device *phydev, } static int dp83867_set_tunable(struct phy_device *phydev, - struct ethtool_tunable *tuna, const void *data) + struct ethtool_tunable *tuna, const void *data) { switch (tuna->id) { case ETHTOOL_PHY_DOWNSHIFT: @@ -524,11 +522,10 @@ static int dp83867_of_init(struct phy_device *phydev) dp83867->io_impedance = -1; /* leave at default */ dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node, - "ti,dp83867-rxctrl-strap-quirk"); + "ti,dp83867-rxctrl-strap-quirk"); dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node, - "ti,sgmii-ref-clock-output-enable"); - + "ti,sgmii-ref-clock-output-enable"); dp83867->rx_id_delay = DP83867_RGMII_RX_CLK_DELAY_INV; ret = of_property_read_u32(of_node, "ti,rx-internal-delay",
Fix spacing issues reported for misaligned switch..case and extra new lines. Also updated the file header to comply with networking commet style. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- drivers/net/phy/dp83867.c | 47 ++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 25 deletions(-)