Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
> > > #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
Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
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_2USBIT(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.
Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
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 --- 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. -- Florian
Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
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 --- 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
Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
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 --- 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_2USBIT(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 -- Florian
