| Message ID | 20171103105553.16859-5-privat@egil-hjelmeland.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: lan9303: Linting | expand |
On Fri, 2017-11-03 at 11:55 +0100, Egil Hjelmeland wrote: > Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses [] > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c [] > @@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) > return reg; > } > > - if ((reg != 0) && (reg != 0xffff)) > + if (reg != 0 && reg != 0xffff) > chip->phy_addr_sel_strap = 1; > else > chip->phy_addr_sel_strap = 0; phy_addr_sel_strap is currently bool. If this is to be changed, it should be set true or false. My preference would be: chip->phy_addr_sel_strap = (reg != 0 && reg != 0xffff); But perhaps its bool type should be converted to int as this phy_addr_sel_strap is used as int several times. $ git grep phy_addr_sel_strap drivers/net/dsa/lan9303-core.c: /* depending on the 'phy_addr_sel_strap' setting, the three phys are drivers/net/dsa/lan9303-core.c: * 'phy_addr_sel_strap' setting directly, so we need a test, which drivers/net/dsa/lan9303-core.c: * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0 drivers/net/dsa/lan9303-core.c: * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3. drivers/net/dsa/lan9303-core.c: chip->phy_addr_sel_strap = 1; drivers/net/dsa/lan9303-core.c: chip->phy_addr_sel_strap = 0; drivers/net/dsa/lan9303-core.c: chip->phy_addr_sel_strap ? "1-2-3" : "0-1-2"); drivers/net/dsa/lan9303-core.c: int phy_base = chip->phy_addr_sel_strap; drivers/net/dsa/lan9303-core.c: int phy_base = chip->phy_addr_sel_strap; drivers/net/dsa/lan9303-core.c: if (port == chip->phy_addr_sel_strap) { drivers/net/dsa/lan9303-core.c: lan9303_phy_write(ds, chip->phy_addr_sel_strap + port, drivers/net/dsa/lan9303-core.c: chip->ds->phys_mii_mask = chip->phy_addr_sel_strap ? 0xe : 0x7; include/linux/dsa/lan9303.h: bool phy_addr_sel_strap;
On 03. nov. 2017 15:11, Joe Perches wrote: > On Fri, 2017-11-03 at 11:55 +0100, Egil Hjelmeland wrote: >> Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses > [] >> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > [] >> @@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) >> return reg; >> } >> >> - if ((reg != 0) && (reg != 0xffff)) >> + if (reg != 0 && reg != 0xffff) >> chip->phy_addr_sel_strap = 1; >> else >> chip->phy_addr_sel_strap = 0; > > phy_addr_sel_strap is currently bool. > > If this is to be changed, it should be set > true or false. > > My preference would be: > > chip->phy_addr_sel_strap = (reg != 0 && reg != 0xffff); > > But perhaps its bool type should be converted > to int as this phy_addr_sel_strap is used as > int several times. > Hi Joe I had not noticed that phy_addr_sel_strap is bool. I agree that is misleading. Your suggestion is perhaps a bit "magic", but on the other hand the magic is explained well in the comment above. If there are no disagreements, I can do a v2 with that. And thanks for teaching me about "git grep"! Cheers Egil
Hi Egil, Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > If there are no disagreements, I can do a v2 with that. > > And thanks for teaching me about "git grep"! If you send a v2, you may want to address the other parenthesis alignment issues found when running ./scripts/checkpatch -f on the lan9303* files. Applying this gives you a few more: https://patchwork.kernel.org/patch/10014913/ (you can also add my Reviewed-by tag on patches you didn't touch.) Thanks, Vivien
On 03. nov. 2017 15:35, Egil Hjelmeland wrote: > On 03. nov. 2017 15:11, Joe Perches wrote: >> On Fri, 2017-11-03 at 11:55 +0100, Egil Hjelmeland wrote: >>> Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses >> [] >>> diff --git a/drivers/net/dsa/lan9303-core.c >>> b/drivers/net/dsa/lan9303-core.c >> [] >>> @@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct >>> lan9303 *chip) >>> return reg; >>> } >>> - if ((reg != 0) && (reg != 0xffff)) >>> + if (reg != 0 && reg != 0xffff) >>> chip->phy_addr_sel_strap = 1; >>> else >>> chip->phy_addr_sel_strap = 0; >> >> phy_addr_sel_strap is currently bool. >> >> If this is to be changed, it should be set >> true or false. >> >> My preference would be: >> >> chip->phy_addr_sel_strap = (reg != 0 && reg != 0xffff); >> >> But perhaps its bool type should be converted >> to int as this phy_addr_sel_strap is used as >> int several times. >> > > Hi Joe > > I had not noticed that phy_addr_sel_strap is bool. I agree that is > misleading. Your suggestion is perhaps a bit "magic", but on the other > hand the magic is explained well in the comment above. > > If there are no disagreements, I can do a v2 with that. > > And thanks for teaching me about "git grep"! > > Cheers > Egil > Hi I changed my mind slightly. I will just remove patch 4 in v2. In stead deal with phy_addr_sel_strap in a separate post. Because I think I want to rename phy_addr_sel_strap as well, plus some other simplification. So starting to creep out of the "linting" scope. Egil
On 03. nov. 2017 15:54, Vivien Didelot wrote: > Hi Egil, > > Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > > If you send a v2, you may want to address the other parenthesis > alignment issues found when running ./scripts/checkpatch -f on the > lan9303* files. > There is just one remaining alignment issue. Removing that would require introducing an extra variable just for that purpose. I don't think that makes the code more readable. So I will not do it. If anybody else want to do it, fine, I will just watch in silence. > Applying this gives you a few more: https://patchwork.kernel.org/patch/10014913/ > > (you can also add my Reviewed-by tag on patches you didn't touch.) > > > Thanks, > > Vivien > Egil
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 70ecd18a5e7d..b9a95f542f65 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -483,7 +483,7 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) return reg; } - if ((reg != 0) && (reg != 0xffff)) + if (reg != 0 && reg != 0xffff) chip->phy_addr_sel_strap = 1; else chip->phy_addr_sel_strap = 0;
Remove scripts/checkpatch.pl CHECKs by remove unnecessary parentheses Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no> --- drivers/net/dsa/lan9303-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)