Hi Richard, On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <d...@g0hl1n.net> wrote: > From: Richard Leitner <richard.leit...@skidata.com> > > Some PHYs need a minimum time after the reset gpio was asserted and/or > deasserted. To ensure we meet these timing requirements add two new > optional devicetree parameters for the phy: reset-delay-us and > reset-post-delay-us.
Thanks for your patch! > This patch depends on the "phylib: Add device reset GPIO support" patch > submitted by Geert Uytterhoeven/Sergei Shtylyov, see: > https://patchwork.kernel.org/patch/10090149/ The above paragraph belongs under the "---" line below, as it is not intended to be preserved in the eternal git history. > Signed-off-by: Richard Leitner <richard.leit...@skidata.com> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be> Although I have a few suggestions below: > --- a/drivers/net/phy/mdio_device.c > +++ b/drivers/net/phy/mdio_device.c > @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove); > > void mdio_device_reset(struct mdio_device *mdiodev, int value) > { > - if (mdiodev->reset) > - gpiod_set_value(mdiodev->reset, value); > + if (!mdiodev->reset) > + return; > + > + gpiod_set_value(mdiodev->reset, value); > + > + if (value && mdiodev->reset_delay) > + usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + > 100); > + else if (!value && mdiodev->reset_post_delay) > + usleep_range(mdiodev->reset_post_delay, > + mdiodev->reset_post_delay + 100); I think this can be written simpler using e.g.: unsigned int delay; ... delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay; if (delay) usleep_range(delay, delay + 100); Perhaps the range extension should be relative, e.g. "delay + min(delay / 10, 100)"? > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, > if (of_property_read_bool(child, "broken-turn-around")) > mdio->phy_ignore_ta_mask |= 1 << addr; > > + if (of_property_read_u32(child, "reset-delay-us", > + &phy->mdio.reset_delay)) > + phy->mdio.reset_delay = 0; > + > + if (of_property_read_u32(child, "reset-post-delay-us", > + &phy->mdio.reset_post_delay)) > + phy->mdio.reset_post_delay = 0; If of_property_read_u32() fails, it doesn't write to its output parameter. As the structure should be zeroed during allocation, you can just write: of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay); of_property_read_u32(child, "reset-post-delay-us", &phy->mdio.reset_post_delay); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
