Re: [PATCH v4 net-next 3/4] net: macb: Add phy-handle DT support

[Brad Mouring]

On Mon, Mar 12, 2018 at 03:30:53PM -0700, Florian Fainelli wrote:
> On 03/12/2018 02:57 PM, Andrew Lunn wrote:
> >> +  /* attempt to find a phy-handle */
> >> +  if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 
> >> 0))) {
> >> +
> >> +  /* fallback to standard phy registration if no 
> >> phy were
> >> +   * found during dt phy registration
> >> +   */
> >> +  if (!phy_find_first(bp->mii_bus)) {
> >> +  for (i = 0; i < PHY_MAX_ADDR; i++) {
> >> +  struct phy_device *phydev;
> >> +  
> >> +  phydev = 
> >> mdiobus_scan(bp->mii_bus, i);
> >> +  if (IS_ERR(phydev) &&
> >> +  PTR_ERR(phydev) != -ENODEV) 
> >> {
> >> +  ret = PTR_ERR(phydev);
> >> +  break;
> >> +  }
> > 
> > Hi Brad
> 
> I would be a bit relaxed on these warnings because the existing function
> indentation really makes it easy to be over 80 columns. Unless you have
> a preliminary patch which, as I was suggesting earlier, re-arranges the
> branches such that if (!np) is the first thing you test, I don't see how
> this can look any better...

I'll try to work this a bit more, thanks for the feedback and the
patience.

-- 
Brad

Re: [PATCH v4 net-next 3/4] net: macb: Add phy-handle DT support

[Florian Fainelli]

On 03/12/2018 02:57 PM, Andrew Lunn wrote:
>> +/* attempt to find a phy-handle */
>> +if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 
>> 0))) {
>> +
>> +/* fallback to standard phy registration if no 
>> phy were
>> + * found during dt phy registration
>> + */
>> +if (!phy_find_first(bp->mii_bus)) {
>> +for (i = 0; i < PHY_MAX_ADDR; i++) {
>> +struct phy_device *phydev;
>> +
>> +phydev = 
>> mdiobus_scan(bp->mii_bus, i);
>> +if (IS_ERR(phydev) &&
>> +PTR_ERR(phydev) != -ENODEV) 
>> {
>> +ret = PTR_ERR(phydev);
>> +break;
>> +}
> 
> Hi Brad

I would be a bit relaxed on these warnings because the existing function
indentation really makes it easy to be over 80 columns. Unless you have
a preliminary patch which, as I was suggesting earlier, re-arranges the
branches such that if (!np) is the first thing you test, I don't see how
this can look any better...

> 
> ./scipts/checkpatch.pl ~/brad.mouring 
> WARNING: line over 80 characters
> #122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
> + if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> 
> ERROR: do not use assignment in if condition
> #122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
> + if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> 
> CHECK: Blank lines aren't necessary after an open brace '{'
> #123: FILE: drivers/net/ethernet/cadence/macb_main.c:493:
> + if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> +
> 
> WARNING: line over 80 characters
> #124: FILE: drivers/net/ethernet/cadence/macb_main.c:494:
> + /* fallback to standard phy registration if no phy were
> 
> ERROR: trailing whitespace
> #130: FILE: drivers/net/ethernet/cadence/macb_main.c:500:
> +^I$
> 
> WARNING: line over 80 characters
> #131: FILE: drivers/net/ethernet/cadence/macb_main.c:501:
> + phydev = mdiobus_scan(bp->mii_bus, i);
> 
> WARNING: Too many leading tabs - consider code refactoring
> #132: FILE: drivers/net/ethernet/cadence/macb_main.c:502:
> + if (IS_ERR(phydev) &&
> 
> etc
> 
>   Andrew
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Florian

Re: [PATCH v4 net-next 3/4] net: macb: Add phy-handle DT support

[Andrew Lunn]

> + /* attempt to find a phy-handle */
> + if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 
> 0))) {
> +
> + /* fallback to standard phy registration if no 
> phy were
> +  * found during dt phy registration
> +  */
> + if (!phy_find_first(bp->mii_bus)) {
> + for (i = 0; i < PHY_MAX_ADDR; i++) {
> + struct phy_device *phydev;
> + 
> + phydev = 
> mdiobus_scan(bp->mii_bus, i);
> + if (IS_ERR(phydev) &&
> + PTR_ERR(phydev) != -ENODEV) 
> {
> + ret = PTR_ERR(phydev);
> + break;
> + }

Hi Brad

./scipts/checkpatch.pl ~/brad.mouring 
WARNING: line over 80 characters
#122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
+   if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {

ERROR: do not use assignment in if condition
#122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
+   if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {

CHECK: Blank lines aren't necessary after an open brace '{'
#123: FILE: drivers/net/ethernet/cadence/macb_main.c:493:
+   if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
+

WARNING: line over 80 characters
#124: FILE: drivers/net/ethernet/cadence/macb_main.c:494:
+   /* fallback to standard phy registration if no phy were

ERROR: trailing whitespace
#130: FILE: drivers/net/ethernet/cadence/macb_main.c:500:
+^I$

WARNING: line over 80 characters
#131: FILE: drivers/net/ethernet/cadence/macb_main.c:501:
+   phydev = mdiobus_scan(bp->mii_bus, i);

WARNING: Too many leading tabs - consider code refactoring
#132: FILE: drivers/net/ethernet/cadence/macb_main.c:502:
+   if (IS_ERR(phydev) &&

etc

Andrew