diff mbox series

[v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER

Message ID 20201119203446.20857-1-grygorii.strashko@ti.com
State New
Headers show
Series [v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER | expand

Commit Message

Grygorii Strashko Nov. 19, 2020, 8:34 p.m. UTC 
The mdio_bus may have dependencies from GPIO controller and so got
deferred. Now it will print error message every time -EPROBE_DEFER is
returned which from:
__mdiobus_register()
 |-devm_gpiod_get_optional()
without actually identifying error code.

"mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"

Hence, suppress error message for devm_gpiod_get_optional() returning
-EPROBE_DEFER case by using dev_err_probe().

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

---
 drivers/net/phy/mdio_bus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

Comments

Heiner Kallweit Nov. 19, 2020, 9:11 p.m. UTC | #1 
Am 19.11.2020 um 21:34 schrieb Grygorii Strashko:
> The mdio_bus may have dependencies from GPIO controller and so got

> deferred. Now it will print error message every time -EPROBE_DEFER is

> returned which from:

> __mdiobus_register()

>  |-devm_gpiod_get_optional()

> without actually identifying error code.

> 

> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"

> 

> Hence, suppress error message for devm_gpiod_get_optional() returning

> -EPROBE_DEFER case by using dev_err_probe().

> 

> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

> ---

>  drivers/net/phy/mdio_bus.c | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c

> index 757e950fb745..83cd61c3dd01 100644

> --- a/drivers/net/phy/mdio_bus.c

> +++ b/drivers/net/phy/mdio_bus.c

> @@ -546,10 +546,10 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)

>  	/* de-assert bus level PHY GPIO reset */

>  	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);

>  	if (IS_ERR(gpiod)) {

> -		dev_err(&bus->dev, "mii_bus %s couldn't get reset GPIO\n",

> -			bus->id);

> +		err = dev_err_probe(&bus->dev, PTR_ERR(gpiod),

> +				    "mii_bus %s couldn't get reset GPIO\n", bus->id);


Doesn't checkpatch complain about line length > 80 here?

>  		device_del(&bus->dev);

> -		return PTR_ERR(gpiod);

> +		return err;

>  	} else	if (gpiod) {

>  		bus->reset_gpiod = gpiod;

>  

> 


Last but not least the net or net-next patch annotation is missing.
I'd be fine with treating the change as an improvement (net-next).

Apart from that change looks good to me.
Grygorii Strashko Nov. 19, 2020, 9:17 p.m. UTC | #2 
On 19/11/2020 23:11, Heiner Kallweit wrote:
> Am 19.11.2020 um 21:34 schrieb Grygorii Strashko:

>> The mdio_bus may have dependencies from GPIO controller and so got

>> deferred. Now it will print error message every time -EPROBE_DEFER is

>> returned which from:

>> __mdiobus_register()

>>   |-devm_gpiod_get_optional()

>> without actually identifying error code.

>>

>> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"

>>

>> Hence, suppress error message for devm_gpiod_get_optional() returning

>> -EPROBE_DEFER case by using dev_err_probe().

>>

>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

>> ---

>>   drivers/net/phy/mdio_bus.c | 6 +++---

>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>

>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c

>> index 757e950fb745..83cd61c3dd01 100644

>> --- a/drivers/net/phy/mdio_bus.c

>> +++ b/drivers/net/phy/mdio_bus.c

>> @@ -546,10 +546,10 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)

>>   	/* de-assert bus level PHY GPIO reset */

>>   	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);

>>   	if (IS_ERR(gpiod)) {

>> -		dev_err(&bus->dev, "mii_bus %s couldn't get reset GPIO\n",

>> -			bus->id);

>> +		err = dev_err_probe(&bus->dev, PTR_ERR(gpiod),

>> +				    "mii_bus %s couldn't get reset GPIO\n", bus->id);

> 

> Doesn't checkpatch complain about line length > 80 here?


:)

commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
Author: Joe Perches <joe@perches.com>
Date:   Fri May 29 16:12:21 2020 -0700

     checkpatch/coding-style: deprecate 80-column warning

> 

>>   		device_del(&bus->dev);

>> -		return PTR_ERR(gpiod);

>> +		return err;

>>   	} else	if (gpiod) {

>>   		bus->reset_gpiod = gpiod;

>>   

>>

> 

> Last but not least the net or net-next patch annotation is missing.

> I'd be fine with treating the change as an improvement (net-next).

> 

> Apart from that change looks good to me.

> 


-- 
Best regards,
grygorii
Heiner Kallweit Nov. 19, 2020, 9:23 p.m. UTC | #3 
Am 19.11.2020 um 22:17 schrieb Grygorii Strashko:
> 

> 

> On 19/11/2020 23:11, Heiner Kallweit wrote:

>> Am 19.11.2020 um 21:34 schrieb Grygorii Strashko:

>>> The mdio_bus may have dependencies from GPIO controller and so got

>>> deferred. Now it will print error message every time -EPROBE_DEFER is

>>> returned which from:

>>> __mdiobus_register()

>>>   |-devm_gpiod_get_optional()

>>> without actually identifying error code.

>>>

>>> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"

>>>

>>> Hence, suppress error message for devm_gpiod_get_optional() returning

>>> -EPROBE_DEFER case by using dev_err_probe().

>>>

>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

>>> ---

>>>   drivers/net/phy/mdio_bus.c | 6 +++---

>>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>>

>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c

>>> index 757e950fb745..83cd61c3dd01 100644

>>> --- a/drivers/net/phy/mdio_bus.c

>>> +++ b/drivers/net/phy/mdio_bus.c

>>> @@ -546,10 +546,10 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)

>>>       /* de-assert bus level PHY GPIO reset */

>>>       gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);

>>>       if (IS_ERR(gpiod)) {

>>> -        dev_err(&bus->dev, "mii_bus %s couldn't get reset GPIO\n",

>>> -            bus->id);

>>> +        err = dev_err_probe(&bus->dev, PTR_ERR(gpiod),

>>> +                    "mii_bus %s couldn't get reset GPIO\n", bus->id);

>>

>> Doesn't checkpatch complain about line length > 80 here?

> 

> :)

> 

> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144

> Author: Joe Perches <joe@perches.com>

> Date:   Fri May 29 16:12:21 2020 -0700

> 

>     checkpatch/coding-style: deprecate 80-column warning

> 


Ah, again something learnt. Thanks for the reference.

>>

>>>           device_del(&bus->dev);

>>> -        return PTR_ERR(gpiod);

>>> +        return err;

>>>       } else    if (gpiod) {

>>>           bus->reset_gpiod = gpiod;

>>>  

>>

>> Last but not least the net or net-next patch annotation is missing.

>> I'd be fine with treating the change as an improvement (net-next).

>>

>> Apart from that change looks good to me.

>>

>
Andrew Lunn Nov. 19, 2020, 9:41 p.m. UTC | #4 
> >> Doesn't checkpatch complain about line length > 80 here?

> > 

> > :)

> > 

> > commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144

> > Author: Joe Perches <joe@perches.com>

> > Date:   Fri May 29 16:12:21 2020 -0700

> > 

> >     checkpatch/coding-style: deprecate 80-column warning

> > 

> 

> Ah, again something learnt. Thanks for the reference.


But it then got revoked for netdev. Or at least it was planned to
re-impose 80 for netdev. I don't know if checkpatch got patched yet.

	  Andrew
Heiner Kallweit Nov. 19, 2020, 10:09 p.m. UTC | #5 
Am 19.11.2020 um 22:41 schrieb Andrew Lunn:
>>>> Doesn't checkpatch complain about line length > 80 here?

>>>

>>> :)

>>>

>>> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144

>>> Author: Joe Perches <joe@perches.com>

>>> Date:   Fri May 29 16:12:21 2020 -0700

>>>

>>>     checkpatch/coding-style: deprecate 80-column warning

>>>

>>

>> Ah, again something learnt. Thanks for the reference.

> 

> But it then got revoked for netdev. Or at least it was planned to

> re-impose 80 for netdev. I don't know if checkpatch got patched yet.

> 

> 	  Andrew

> 

At a first glance it sounds strange that subsystems may define own
rules for such basic things. But supposedly there has been a longer
emotional disucssion about this already ..
Jakub Kicinski Nov. 20, 2020, 5:21 a.m. UTC | #6 
On Thu, 19 Nov 2020 23:09:52 +0100 Heiner Kallweit wrote:
> Am 19.11.2020 um 22:41 schrieb Andrew Lunn:

> >>>> Doesn't checkpatch complain about line length > 80 here?  

> >>>

> >>> :)

> >>>

> >>> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144

> >>> Author: Joe Perches <joe@perches.com>

> >>> Date:   Fri May 29 16:12:21 2020 -0700

> >>>

> >>>     checkpatch/coding-style: deprecate 80-column warning

> >>>  

> >>

> >> Ah, again something learnt. Thanks for the reference.  

> > 

> > But it then got revoked for netdev. Or at least it was planned to

> > re-impose 80 for netdev. I don't know if checkpatch got patched yet.


FWIW I had a patch for it but before I sent it Dave suggested I ask
around and Alexei was opposed.

And I don't have the strength to argue :)

I'll just tell people case by case when they have 4+ indentation levels
in their code or use 40+ character variables/defines, in my copious
spare time. 

> At a first glance it sounds strange that subsystems may define own

> rules for such basic things. But supposedly there has been a longer

> emotional disucssion about this already ..


We do have our own comment style rule in networking since the beginning
of time, and reverse xmas tree, so it's not completely crazy.
Joe Perches Nov. 20, 2020, 7:06 a.m. UTC | #7 
On Thu, 2020-11-19 at 21:21 -0800, Jakub Kicinski wrote:
> We do have our own comment style rule in networking since the beginning

> of time, and reverse xmas tree, so it's not completely crazy.


reverse xmas tree is completely crazy.

But I posted a patch to checkpatch to suggest it for net/
and drivers/net/ once

https://lkml.org/lkml/2016/11/4/54
Andrew Lunn Nov. 20, 2020, 2:50 p.m. UTC | #8 
Hi Joe

> reverse xmas tree is completely crazy.

> 

> But I posted a patch to checkpatch to suggest it for net/

> and drivers/net/ once

> 

> https://lkml.org/lkml/2016/11/4/54

 

> From Joe Perches <> 

>

...

> and the reverse xmas tree helpfulness of looking up the

> type of bar is neither obvious nor easy.

>

> My preference would be for a bar that serves coffee and alcohol.


Ah, those were the days.

Anyway, can this patch be brought back to life, with the problem
pointed out fixed? It is still something we do in netdev, and a
machine can spot these problems better than a human maintainer or
developer.

Thanks
	Andrew
Jakub Kicinski Nov. 21, 2020, 3:12 a.m. UTC | #9 
On Thu, 19 Nov 2020 22:34:46 +0200 Grygorii Strashko wrote:
> The mdio_bus may have dependencies from GPIO controller and so got

> deferred. Now it will print error message every time -EPROBE_DEFER is

> returned which from:

> __mdiobus_register()

>  |-devm_gpiod_get_optional()

> without actually identifying error code.

> 

> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"

> 

> Hence, suppress error message for devm_gpiod_get_optional() returning

> -EPROBE_DEFER case by using dev_err_probe().

> 

> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>


Applied (with the line wrapped), thanks!
diff mbox series

Patch

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 757e950fb745..83cd61c3dd01 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -546,10 +546,10 @@  int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	/* de-assert bus level PHY GPIO reset */
 	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(gpiod)) {
-		dev_err(&bus->dev, "mii_bus %s couldn't get reset GPIO\n",
-			bus->id);
+		err = dev_err_probe(&bus->dev, PTR_ERR(gpiod),
+				    "mii_bus %s couldn't get reset GPIO\n", bus->id);
 		device_del(&bus->dev);
-		return PTR_ERR(gpiod);
+		return err;
 	} else	if (gpiod) {
 		bus->reset_gpiod = gpiod;