Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752692AbdLEPHM (ORCPT ); Tue, 5 Dec 2017 10:07:12 -0500 Received: from mail1.skidata.com ([91.230.2.99]:27659 "EHLO mail1.skidata.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752339AbdLEPHL (ORCPT ); Tue, 5 Dec 2017 10:07:11 -0500 X-Greylist: delayed 593 seconds by postgrey-1.27 at vger.kernel.org; Tue, 05 Dec 2017 10:07:10 EST X-IronPort-AV: E=Sophos;i="5.45,364,1508796000"; d="scan'208";a="7471607" Subject: Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support To: Geert Uytterhoeven CC: Richard Leitner , Rob Herring , "Mark Rutland" , Fugang Duan , "Andrew Lunn" , Florian Fainelli , Frank Rowand , "David S. Miller" , "Geert Uytterhoeven" , Sergei Shtylyov , Baruch Siach , "David Wu" , , "netdev@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20171205132600.13796-1-dev@g0hl1n.net> <20171205132600.13796-2-dev@g0hl1n.net> From: Richard Leitner Message-ID: <35a63da6-93e1-b2f0-a3cc-ad079b2ffb28@skidata.com> Date: Tue, 5 Dec 2017 15:56:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [172.16.60.30] X-ClientProxiedBy: sdex1srv.skidata.net (172.16.10.92) To sdex1srv.skidata.net (172.16.10.92) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3217 Lines: 86 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 wrote: >> From: Richard Leitner >> >> 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 > > Reviewed-by: Geert Uytterhoeven > > 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);