Avoids having to use own struct member.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/net/ethernet/ibm/emac/core.c | 47 +++++++++++++---------------
drivers/net/ethernet/ibm/emac/core.h | 3 --
2 files changed, 21 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index e2abda947a51..cda368701ae4 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -2459,7 +2459,7 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
static void emac_adjust_link(struct net_device *ndev)
{
struct emac_instance *dev = netdev_priv(ndev);
- struct phy_device *phy = dev->phy_dev;
+ struct phy_device *phy = ndev->phydev;
dev->phy.autoneg = phy->autoneg;
dev->phy.speed = phy->speed;
@@ -2510,22 +2510,20 @@ static int emac_mdio_phy_start_aneg(struct mii_phy *phy,
static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
{
struct net_device *ndev = phy->dev;
- struct emac_instance *dev = netdev_priv(ndev);
phy->autoneg = AUTONEG_ENABLE;
phy->advertising = advertise;
- return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
+ return emac_mdio_phy_start_aneg(phy, ndev->phydev);
}
static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
{
struct net_device *ndev = phy->dev;
- struct emac_instance *dev = netdev_priv(ndev);
phy->autoneg = AUTONEG_DISABLE;
phy->speed = speed;
phy->duplex = fd;
- return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
+ return emac_mdio_phy_start_aneg(phy, ndev->phydev);
}
static int emac_mdio_poll_link(struct mii_phy *phy)
@@ -2534,20 +2532,19 @@ static int emac_mdio_poll_link(struct mii_phy *phy)
struct emac_instance *dev = netdev_priv(ndev);
int res;
- res = phy_read_status(dev->phy_dev);
+ res = phy_read_status(ndev->phydev);
if (res) {
dev_err(&dev->ofdev->dev, "link update failed (%d).", res);
return ethtool_op_get_link(ndev);
}
- return dev->phy_dev->link;
+ return ndev->phydev->link;
}
static int emac_mdio_read_link(struct mii_phy *phy)
{
struct net_device *ndev = phy->dev;
- struct emac_instance *dev = netdev_priv(ndev);
- struct phy_device *phy_dev = dev->phy_dev;
+ struct phy_device *phy_dev = ndev->phydev;
int res;
res = phy_read_status(phy_dev);
@@ -2564,10 +2561,9 @@ static int emac_mdio_read_link(struct mii_phy *phy)
static int emac_mdio_init_phy(struct mii_phy *phy)
{
struct net_device *ndev = phy->dev;
- struct emac_instance *dev = netdev_priv(ndev);
- phy_start(dev->phy_dev);
- return phy_init_hw(dev->phy_dev);
+ phy_start(ndev->phydev);
+ return phy_init_hw(ndev->phydev);
}
static const struct mii_phy_ops emac_dt_mdio_phy_ops = {
@@ -2622,26 +2618,28 @@ static int emac_dt_mdio_probe(struct emac_instance *dev)
static int emac_dt_phy_connect(struct emac_instance *dev,
struct device_node *phy_handle)
{
+ struct phy_device *phy_dev;
+
dev->phy.def = devm_kzalloc(&dev->ofdev->dev, sizeof(*dev->phy.def),
GFP_KERNEL);
if (!dev->phy.def)
return -ENOMEM;
- dev->phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
+ phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
0, dev->phy_mode);
- if (!dev->phy_dev) {
+ if (!phy_dev) {
dev_err(&dev->ofdev->dev, "failed to connect to PHY.\n");
return -ENODEV;
}
- dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
- dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
- dev->phy.def->name = dev->phy_dev->drv->name;
+ dev->phy.def->phy_id = phy_dev->drv->phy_id;
+ dev->phy.def->phy_id_mask = phy_dev->drv->phy_id_mask;
+ dev->phy.def->name = phy_dev->drv->name;
dev->phy.def->ops = &emac_dt_mdio_phy_ops;
ethtool_convert_link_mode_to_legacy_u32(&dev->phy.features,
- dev->phy_dev->supported);
- dev->phy.address = dev->phy_dev->mdio.addr;
- dev->phy.mode = dev->phy_dev->interface;
+ phy_dev->supported);
+ dev->phy.address = phy_dev->mdio.addr;
+ dev->phy.mode = phy_dev->interface;
return 0;
}
@@ -2695,11 +2693,11 @@ static int emac_init_phy(struct emac_instance *dev)
return res;
res = of_phy_register_fixed_link(np);
- dev->phy_dev = of_phy_find_device(np);
- if (res || !dev->phy_dev)
+ ndev->phydev = of_phy_find_device(np);
+ if (res || !ndev->phydev)
return res ? res : -EINVAL;
emac_adjust_link(dev->ndev);
- put_device(&dev->phy_dev->mdio.dev);
+ put_device(&ndev->phydev->mdio.dev);
}
return 0;
}
@@ -3254,9 +3252,6 @@ static void emac_remove(struct platform_device *ofdev)
if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
zmii_detach(dev->zmii_dev, dev->zmii_port);
- if (dev->phy_dev)
- phy_disconnect(dev->phy_dev);
-
busy_phy_map &= ~(1 << dev->phy.address);
DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
index f4bd4cd8ac4a..b820a4f6e8c7 100644
--- a/drivers/net/ethernet/ibm/emac/core.h
+++ b/drivers/net/ethernet/ibm/emac/core.h
@@ -188,9 +188,6 @@ struct emac_instance {
struct emac_instance *mdio_instance;
struct mutex mdio_lock;
- /* Device-tree based phy configuration */
- struct phy_device *phy_dev;
-
/* ZMII infos if any */
u32 zmii_ph;
u32 zmii_port;
--
2.46.0Le 05/09/2024 à 22:15, Rosen Penev a écrit :
> Avoids having to use own struct member.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/net/ethernet/ibm/emac/core.c | 47 +++++++++++++---------------
> drivers/net/ethernet/ibm/emac/core.h | 3 --
> 2 files changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index e2abda947a51..cda368701ae4 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -2459,7 +2459,7 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
> static void emac_adjust_link(struct net_device *ndev)
> {
> struct emac_instance *dev = netdev_priv(ndev);
> - struct phy_device *phy = dev->phy_dev;
> + struct phy_device *phy = ndev->phydev;
>
> dev->phy.autoneg = phy->autoneg;
> dev->phy.speed = phy->speed;
> @@ -2510,22 +2510,20 @@ static int emac_mdio_phy_start_aneg(struct mii_phy *phy,
> static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
> {
> struct net_device *ndev = phy->dev;
> - struct emac_instance *dev = netdev_priv(ndev);
>
> phy->autoneg = AUTONEG_ENABLE;
> phy->advertising = advertise;
> - return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> + return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> }
>
> static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
> {
> struct net_device *ndev = phy->dev;
> - struct emac_instance *dev = netdev_priv(ndev);
>
> phy->autoneg = AUTONEG_DISABLE;
> phy->speed = speed;
> phy->duplex = fd;
> - return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> + return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> }
>
> static int emac_mdio_poll_link(struct mii_phy *phy)
> @@ -2534,20 +2532,19 @@ static int emac_mdio_poll_link(struct mii_phy *phy)
> struct emac_instance *dev = netdev_priv(ndev);
> int res;
>
> - res = phy_read_status(dev->phy_dev);
> + res = phy_read_status(ndev->phydev);
> if (res) {
> dev_err(&dev->ofdev->dev, "link update failed (%d).", res);
> return ethtool_op_get_link(ndev);
> }
>
> - return dev->phy_dev->link;
> + return ndev->phydev->link;
> }
>
> static int emac_mdio_read_link(struct mii_phy *phy)
> {
> struct net_device *ndev = phy->dev;
> - struct emac_instance *dev = netdev_priv(ndev);
> - struct phy_device *phy_dev = dev->phy_dev;
> + struct phy_device *phy_dev = ndev->phydev;
> int res;
>
> res = phy_read_status(phy_dev);
> @@ -2564,10 +2561,9 @@ static int emac_mdio_read_link(struct mii_phy *phy)
> static int emac_mdio_init_phy(struct mii_phy *phy)
> {
> struct net_device *ndev = phy->dev;
> - struct emac_instance *dev = netdev_priv(ndev);
>
> - phy_start(dev->phy_dev);
> - return phy_init_hw(dev->phy_dev);
> + phy_start(ndev->phydev);
> + return phy_init_hw(ndev->phydev);
> }
>
> static const struct mii_phy_ops emac_dt_mdio_phy_ops = {
> @@ -2622,26 +2618,28 @@ static int emac_dt_mdio_probe(struct emac_instance *dev)
> static int emac_dt_phy_connect(struct emac_instance *dev,
> struct device_node *phy_handle)
> {
> + struct phy_device *phy_dev;
> +
> dev->phy.def = devm_kzalloc(&dev->ofdev->dev, sizeof(*dev->phy.def),
> GFP_KERNEL);
> if (!dev->phy.def)
> return -ENOMEM;
>
> - dev->phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> + phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> 0, dev->phy_mode);
> - if (!dev->phy_dev) {
> + if (!phy_dev) {
> dev_err(&dev->ofdev->dev, "failed to connect to PHY.\n");
> return -ENODEV;
> }
>
> - dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
> - dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
> - dev->phy.def->name = dev->phy_dev->drv->name;
> + dev->phy.def->phy_id = phy_dev->drv->phy_id;
> + dev->phy.def->phy_id_mask = phy_dev->drv->phy_id_mask;
> + dev->phy.def->name = phy_dev->drv->name;
> dev->phy.def->ops = &emac_dt_mdio_phy_ops;
> ethtool_convert_link_mode_to_legacy_u32(&dev->phy.features,
> - dev->phy_dev->supported);
> - dev->phy.address = dev->phy_dev->mdio.addr;
> - dev->phy.mode = dev->phy_dev->interface;
> + phy_dev->supported);
> + dev->phy.address = phy_dev->mdio.addr;
> + dev->phy.mode = phy_dev->interface;
> return 0;
> }
>
> @@ -2695,11 +2693,11 @@ static int emac_init_phy(struct emac_instance *dev)
> return res;
>
> res = of_phy_register_fixed_link(np);
> - dev->phy_dev = of_phy_find_device(np);
> - if (res || !dev->phy_dev)
> + ndev->phydev = of_phy_find_device(np);
> + if (res || !ndev->phydev)
> return res ? res : -EINVAL;
> emac_adjust_link(dev->ndev);
> - put_device(&dev->phy_dev->mdio.dev);
> + put_device(&ndev->phydev->mdio.dev);
> }
> return 0;
> }
> @@ -3254,9 +3252,6 @@ static void emac_remove(struct platform_device *ofdev)
> if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
> zmii_detach(dev->zmii_dev, dev->zmii_port);
>
> - if (dev->phy_dev)
> - phy_disconnect(dev->phy_dev);
> -
Hi,
I guess that this call was to balance the of_phy_connect() from
emac_dt_phy_connect().
Is it ok to just remove this phy_disconnect()?
CJ
> busy_phy_map &= ~(1 << dev->phy.address);
> DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
>
> diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> index f4bd4cd8ac4a..b820a4f6e8c7 100644
> --- a/drivers/net/ethernet/ibm/emac/core.h
> +++ b/drivers/net/ethernet/ibm/emac/core.h
> @@ -188,9 +188,6 @@ struct emac_instance {
> struct emac_instance *mdio_instance;
> struct mutex mdio_lock;
>
> - /* Device-tree based phy configuration */
> - struct phy_device *phy_dev;
> -
> /* ZMII infos if any */
> u32 zmii_ph;
> u32 zmii_port;
On Sat, Sep 7, 2024 at 12:32 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 05/09/2024 à 22:15, Rosen Penev a écrit :
> > Avoids having to use own struct member.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> > drivers/net/ethernet/ibm/emac/core.c | 47 +++++++++++++---------------
> > drivers/net/ethernet/ibm/emac/core.h | 3 --
> > 2 files changed, 21 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index e2abda947a51..cda368701ae4 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -2459,7 +2459,7 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
> > static void emac_adjust_link(struct net_device *ndev)
> > {
> > struct emac_instance *dev = netdev_priv(ndev);
> > - struct phy_device *phy = dev->phy_dev;
> > + struct phy_device *phy = ndev->phydev;
> >
> > dev->phy.autoneg = phy->autoneg;
> > dev->phy.speed = phy->speed;
> > @@ -2510,22 +2510,20 @@ static int emac_mdio_phy_start_aneg(struct mii_phy *phy,
> > static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
> > {
> > struct net_device *ndev = phy->dev;
> > - struct emac_instance *dev = netdev_priv(ndev);
> >
> > phy->autoneg = AUTONEG_ENABLE;
> > phy->advertising = advertise;
> > - return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> > + return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> > }
> >
> > static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
> > {
> > struct net_device *ndev = phy->dev;
> > - struct emac_instance *dev = netdev_priv(ndev);
> >
> > phy->autoneg = AUTONEG_DISABLE;
> > phy->speed = speed;
> > phy->duplex = fd;
> > - return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> > + return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> > }
> >
> > static int emac_mdio_poll_link(struct mii_phy *phy)
> > @@ -2534,20 +2532,19 @@ static int emac_mdio_poll_link(struct mii_phy *phy)
> > struct emac_instance *dev = netdev_priv(ndev);
> > int res;
> >
> > - res = phy_read_status(dev->phy_dev);
> > + res = phy_read_status(ndev->phydev);
> > if (res) {
> > dev_err(&dev->ofdev->dev, "link update failed (%d).", res);
> > return ethtool_op_get_link(ndev);
> > }
> >
> > - return dev->phy_dev->link;
> > + return ndev->phydev->link;
> > }
> >
> > static int emac_mdio_read_link(struct mii_phy *phy)
> > {
> > struct net_device *ndev = phy->dev;
> > - struct emac_instance *dev = netdev_priv(ndev);
> > - struct phy_device *phy_dev = dev->phy_dev;
> > + struct phy_device *phy_dev = ndev->phydev;
> > int res;
> >
> > res = phy_read_status(phy_dev);
> > @@ -2564,10 +2561,9 @@ static int emac_mdio_read_link(struct mii_phy *phy)
> > static int emac_mdio_init_phy(struct mii_phy *phy)
> > {
> > struct net_device *ndev = phy->dev;
> > - struct emac_instance *dev = netdev_priv(ndev);
> >
> > - phy_start(dev->phy_dev);
> > - return phy_init_hw(dev->phy_dev);
> > + phy_start(ndev->phydev);
> > + return phy_init_hw(ndev->phydev);
> > }
> >
> > static const struct mii_phy_ops emac_dt_mdio_phy_ops = {
> > @@ -2622,26 +2618,28 @@ static int emac_dt_mdio_probe(struct emac_instance *dev)
> > static int emac_dt_phy_connect(struct emac_instance *dev,
> > struct device_node *phy_handle)
> > {
> > + struct phy_device *phy_dev;
> > +
> > dev->phy.def = devm_kzalloc(&dev->ofdev->dev, sizeof(*dev->phy.def),
> > GFP_KERNEL);
> > if (!dev->phy.def)
> > return -ENOMEM;
> >
> > - dev->phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> > + phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> > 0, dev->phy_mode);
> > - if (!dev->phy_dev) {
> > + if (!phy_dev) {
> > dev_err(&dev->ofdev->dev, "failed to connect to PHY.\n");
> > return -ENODEV;
> > }
> >
> > - dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
> > - dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
> > - dev->phy.def->name = dev->phy_dev->drv->name;
> > + dev->phy.def->phy_id = phy_dev->drv->phy_id;
> > + dev->phy.def->phy_id_mask = phy_dev->drv->phy_id_mask;
> > + dev->phy.def->name = phy_dev->drv->name;
> > dev->phy.def->ops = &emac_dt_mdio_phy_ops;
> > ethtool_convert_link_mode_to_legacy_u32(&dev->phy.features,
> > - dev->phy_dev->supported);
> > - dev->phy.address = dev->phy_dev->mdio.addr;
> > - dev->phy.mode = dev->phy_dev->interface;
> > + phy_dev->supported);
> > + dev->phy.address = phy_dev->mdio.addr;
> > + dev->phy.mode = phy_dev->interface;
> > return 0;
> > }
> >
> > @@ -2695,11 +2693,11 @@ static int emac_init_phy(struct emac_instance *dev)
> > return res;
> >
> > res = of_phy_register_fixed_link(np);
> > - dev->phy_dev = of_phy_find_device(np);
> > - if (res || !dev->phy_dev)
> > + ndev->phydev = of_phy_find_device(np);
> > + if (res || !ndev->phydev)
> > return res ? res : -EINVAL;
> > emac_adjust_link(dev->ndev);
> > - put_device(&dev->phy_dev->mdio.dev);
> > + put_device(&ndev->phydev->mdio.dev);
> > }
> > return 0;
> > }
> > @@ -3254,9 +3252,6 @@ static void emac_remove(struct platform_device *ofdev)
> > if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
> > zmii_detach(dev->zmii_dev, dev->zmii_port);
> >
> > - if (dev->phy_dev)
> > - phy_disconnect(dev->phy_dev);
> > -
>
> Hi,
>
> I guess that this call was to balance the of_phy_connect() from
> emac_dt_phy_connect().
>
> Is it ok to just remove this phy_disconnect()?
I would think free or unregister_netdev handles it given that phydev
is a member of netdev.
On a separate note, A lot of drivers seem to do this in open/close as
opposed to probe/remove.
>
> CJ
>
> > busy_phy_map &= ~(1 << dev->phy.address);
> > DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> > index f4bd4cd8ac4a..b820a4f6e8c7 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.h
> > +++ b/drivers/net/ethernet/ibm/emac/core.h
> > @@ -188,9 +188,6 @@ struct emac_instance {
> > struct emac_instance *mdio_instance;
> > struct mutex mdio_lock;
> >
> > - /* Device-tree based phy configuration */
> > - struct phy_device *phy_dev;
> > -
> > /* ZMII infos if any */
> > u32 zmii_ph;
> > u32 zmii_port;
>
On Thu, Sep 05, 2024 at 01:15:03PM -0700, Rosen Penev wrote:
> Avoids having to use own struct member.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
© 2016 - 2026 Red Hat, Inc.