From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Wed, 26 Feb 2014 12:21:06 +0000 Subject: Re: [PATCH v3] sh_eth: call of_mdiobus_register() to register phys Message-Id: <530DDC32.9010002@cogentembedded.com> List-Id: References: <1393415873-10520-1-git-send-email-ben.dooks@codethink.co.uk> In-Reply-To: <1393415873-10520-1-git-send-email-ben.dooks@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Ben Dooks , netdev@vger.kernel.org Cc: linux-sh@vger.kernel.org, nobuhiro.iwamatsu.yj@renesas.com Hello. On 26-02-2014 15:57, 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. I think you now should extend your changelog as you've decided to also use of_ohy_connect(). > Signed-off-by: Ben Dooks > -- > v2: > - allocate mdio->irq array at init time > - set devdata after succesful mdio registration > v3: > - do not parse phy->irq in of setup (done by of_mdiobus) > - use of_phy_connect to connect phy > --- > drivers/net/ethernet/renesas/sh_eth.c | 46 ++++++++++++++++++++++++++--------- > 1 file changed, 34 insertions(+), 12 deletions(-) > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index a18cbe1..6d14abf 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] > @@ -1764,17 +1765,31 @@ static int sh_eth_phy_init(struct net_device *ndev) > struct sh_eth_private *mdp = netdev_priv(ndev); > char phy_id[MII_BUS_ID_SIZE + 3]; Shouldn't this variable be moved to where it's used now? > struct phy_device *phydev = NULL; > - > - snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > - mdp->mii_bus->id, mdp->phy_id); > + struct device_node *np = ndev->dev.parent->of_node; > > mdp->link = 0; > mdp->speed = 0; > mdp->duplex = -1; > > - /* Try connect to PHY */ Why remove the comment? > - 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, > + mdp->phy_interface); > + Empty line hardly needed here. > + if (!phydev) > + phydev = ERR_PTR(ENOENT); Not -ENOENT? > + } else { > + snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > + mdp->mii_bus->id, mdp->phy_id); > + > + /* Try connect to PHY */ > + phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link, > + mdp->phy_interface); > + } > + Empty line can be omitted here as well... > if (IS_ERR(phydev)) { > dev_err(&ndev->dev, "phy_connect failed\n"); The message needs some adjustment now, like "can't connect to PHY\n". > @@ -2638,6 +2653,18 @@ static int sh_mdio_init(struct net_device *ndev, int id, > goto out_free_bus; > } > > + if (ndev->dev.parent->of_node) { > + ret = of_mdiobus_register(mdp->mii_bus, > + ndev->dev.parent->of_node); > + if (ret != 0) { You forgot to remove != 0... > + dev_err(&ndev->dev, "of_mdiobus_register() failed\n"); > + goto out_free_bus; > + } > + > + dev_set_drvdata(&ndev->dev, mdp->mii_bus); > + return 0; Still repetitive without *goto* (or *else*)... > + } > + > for (i = 0; i < PHY_MAX_ADDR; i++) > mdp->mii_bus->irq[i] = PHY_POLL; > if (pd->phy_irq > 0) [...] > @@ -2727,11 +2753,7 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) > return NULL; > > pdata->phy_interface = of_get_phy_mode(np); > - > - phy = of_parse_phandle(np, "phy-handle", 0); > - if (of_property_read_u32(phy, "reg", &pdata->phy)) > - return NULL; > - pdata->phy_irq = irq_of_parse_and_map(phy, 0); > + pdata->phy_irq = PHY_POLL; You can completely omit this. It won't get used now anyway. WBR, Sergei From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Subject: Re: [PATCH v3] sh_eth: call of_mdiobus_register() to register phys Date: Wed, 26 Feb 2014 16:21:06 +0400 Message-ID: <530DDC32.9010002@cogentembedded.com> References: <1393415873-10520-1-git-send-email-ben.dooks@codethink.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-sh@vger.kernel.org, nobuhiro.iwamatsu.yj@renesas.com To: Ben Dooks , netdev@vger.kernel.org Return-path: In-Reply-To: <1393415873-10520-1-git-send-email-ben.dooks@codethink.co.uk> Sender: linux-sh-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Hello. On 26-02-2014 15:57, 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. I think you now should extend your changelog as you've decided to also use of_ohy_connect(). > Signed-off-by: Ben Dooks > -- > v2: > - allocate mdio->irq array at init time > - set devdata after succesful mdio registration > v3: > - do not parse phy->irq in of setup (done by of_mdiobus) > - use of_phy_connect to connect phy > --- > drivers/net/ethernet/renesas/sh_eth.c | 46 ++++++++++++++++++++++++++--------- > 1 file changed, 34 insertions(+), 12 deletions(-) > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index a18cbe1..6d14abf 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] > @@ -1764,17 +1765,31 @@ static int sh_eth_phy_init(struct net_device *ndev) > struct sh_eth_private *mdp = netdev_priv(ndev); > char phy_id[MII_BUS_ID_SIZE + 3]; Shouldn't this variable be moved to where it's used now? > struct phy_device *phydev = NULL; > - > - snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > - mdp->mii_bus->id, mdp->phy_id); > + struct device_node *np = ndev->dev.parent->of_node; > > mdp->link = 0; > mdp->speed = 0; > mdp->duplex = -1; > > - /* Try connect to PHY */ Why remove the comment? > - 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, > + mdp->phy_interface); > + Empty line hardly needed here. > + if (!phydev) > + phydev = ERR_PTR(ENOENT); Not -ENOENT? > + } else { > + snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > + mdp->mii_bus->id, mdp->phy_id); > + > + /* Try connect to PHY */ > + phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link, > + mdp->phy_interface); > + } > + Empty line can be omitted here as well... > if (IS_ERR(phydev)) { > dev_err(&ndev->dev, "phy_connect failed\n"); The message needs some adjustment now, like "can't connect to PHY\n". > @@ -2638,6 +2653,18 @@ static int sh_mdio_init(struct net_device *ndev, int id, > goto out_free_bus; > } > > + if (ndev->dev.parent->of_node) { > + ret = of_mdiobus_register(mdp->mii_bus, > + ndev->dev.parent->of_node); > + if (ret != 0) { You forgot to remove != 0... > + dev_err(&ndev->dev, "of_mdiobus_register() failed\n"); > + goto out_free_bus; > + } > + > + dev_set_drvdata(&ndev->dev, mdp->mii_bus); > + return 0; Still repetitive without *goto* (or *else*)... > + } > + > for (i = 0; i < PHY_MAX_ADDR; i++) > mdp->mii_bus->irq[i] = PHY_POLL; > if (pd->phy_irq > 0) [...] > @@ -2727,11 +2753,7 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) > return NULL; > > pdata->phy_interface = of_get_phy_mode(np); > - > - phy = of_parse_phandle(np, "phy-handle", 0); > - if (of_property_read_u32(phy, "reg", &pdata->phy)) > - return NULL; > - pdata->phy_irq = irq_of_parse_and_map(phy, 0); > + pdata->phy_irq = PHY_POLL; You can completely omit this. It won't get used now anyway. WBR, Sergei