| Message ID | 20201119203446.20857-1-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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. >
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. >> >
> >> 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
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 ..
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.
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
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
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 --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;
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(-)