Hi Geert, On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote: > Hi Richard, > > On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <dev@xxxxxxxxxx> wrote: >> From: Richard Leitner <richard.leitner@xxxxxxxxxxx> >> >> 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. Ok. Thanks. That makes sense. I'll take it into account for v4. > >> Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxxxx> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > Although I have a few suggestions below: Thank you for your review and suggestions (they make the code look way more neater). I'll adapt v4 accordingly. > >> --- 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); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
