From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Dooks Subject: Re: [RESEND] [PATCH v4] sh_eth: update OF PHY registeration Date: Thu, 27 Feb 2014 12:46:07 +0000 Message-ID: <530F338F.7060604@codethink.co.uk> References: <1393439008-26698-1-git-send-email-ben.dooks@codethink.co.uk> <530E44BA.8090209@cogentembedded.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, linux-sh@vger.kernel.org, nobuhiro.iwamatsu.yj@renesas.com, magnus.damn@opensource.se, horms@verge.net.au To: Sergei Shtylyov Return-path: In-Reply-To: <530E44BA.8090209@cogentembedded.com> Sender: linux-sh-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 26/02/14 19:47, Sergei Shtylyov wrote: > On 02/26/2014 09:23 PM, Ben Dooks wrote: > >> If the sh_eth device is registered using OF, then the driver >> should call of_mdiobus_register() to register any PHYs connected >> to the system and of_phy_connect() to connect it. > > s/it/to PHY/? Ok, will change. >> This ensures that any PHYs registered in the device tree are >> appropriately connected to the parent devices nodes so that >> the PHY drivers can access their OF properties. > >> Signed-off-by: Ben Dooks > > [...] > >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c >> b/drivers/net/ethernet/renesas/sh_eth.c >> index a18cbe1..e597698 100644 >> --- a/drivers/net/ethernet/renesas/sh_eth.c >> +++ b/drivers/net/ethernet/renesas/sh_eth.c > [...] >> @@ -1761,20 +1762,36 @@ static void sh_eth_adjust_link(struct >> net_device *ndev) >> /* PHY init function */ >> static int sh_eth_phy_init(struct net_device *ndev) >> { >> + struct device_node *np = ndev->dev.parent->of_node; >> struct sh_eth_private *mdp = netdev_priv(ndev); >> - char phy_id[MII_BUS_ID_SIZE + 3]; >> struct phy_device *phydev = NULL; >> >> - snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, >> - mdp->mii_bus->id, mdp->phy_id); >> - >> mdp->link = 0; >> mdp->speed = 0; >> mdp->duplex = -1; >> >> /* Try connect to PHY */ >> - phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link, >> - mdp->phy_interface); >> + if (np) { >> + struct device_node *pn; >> + >> + pn = of_parse_phandle(np, "phy-handle", 0); >> + phydev = of_phy_connect(ndev, pn, >> + sh_eth_adjust_link, 0, > > Hm, that 0 corresponds to 'flags' parameter which is not used. I > wonder if that was intentional... This can be left to a separate patch to fix. > >> + mdp->phy_interface); >> + >> + if (!phydev) >> + phydev = ERR_PTR(-ENOENT); >> + } else { >> + char phy_id[MII_BUS_ID_SIZE + 3]; >> + >> + snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, >> + mdp->mii_bus->id, mdp->phy_id); >> + >> + /* Try connect to PHY */ > > Now this comment is kinda duplicate... :-) Bah, got rid of it. >> + phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link, >> + mdp->phy_interface); >> + } >> + >> if (IS_ERR(phydev)) { >> dev_err(&ndev->dev, "phy_connect failed\n"); > > I have asked to adjust the message a bit as we now have 2 alternate > calls that can fail... ok, changed to "failed to connect phy" > >> return PTR_ERR(phydev); >> @@ -2638,15 +2655,23 @@ static int sh_mdio_init(struct net_device >> *ndev, int id, >> goto out_free_bus; >> } >> >> - for (i = 0; i < PHY_MAX_ADDR; i++) >> - mdp->mii_bus->irq[i] = PHY_POLL; >> - if (pd->phy_irq > 0) >> - mdp->mii_bus->irq[pd->phy] = pd->phy_irq; >> + if (ndev->dev.parent->of_node) { >> + ret = of_mdiobus_register(mdp->mii_bus, >> + ndev->dev.parent->of_node); >> + } else { >> + for (i = 0; i < PHY_MAX_ADDR; i++) >> + mdp->mii_bus->irq[i] = PHY_POLL; >> + if (pd->phy_irq > 0) >> + mdp->mii_bus->irq[pd->phy] = pd->phy_irq; >> >> - /* register mdio bus */ >> - ret = mdiobus_register(mdp->mii_bus); >> - if (ret) >> + /* register mdio bus */ >> + ret = mdiobus_register(mdp->mii_bus); >> + } > > That looks much better, thanks. > >> + > > Although I'd have avoided the empty line... > >> + if (ret) { >> + dev_err(&ndev->dev, "failed to register mdio\n"); > > That function generally avoids error messages, no? Although all > errors are -ENOMEM type and that's supposed to cause kernel to loudly > curse anyway... Well, let it be if you want. mdiobus_register() or of_mdiobus_register() could return any error, but i've moved the error print to the caller as this is useful to know. Thanks for the reviews, hopefully the next round will work. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius