[U-Boot] [PATCH v3 2/6] driver: net: fsl-mc: remove unused strcture elements
Pankaj Bansal
pankaj.bansal at nxp.com
Wed Oct 10 05:10:51 UTC 2018
Hi Joe,
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at ni.com]
> Sent: Wednesday, October 10, 2018 9:29 AM
> To: Pankaj Bansal <pankaj.bansal at nxp.com>
> Cc: Joseph Hershberger <joseph.hershberger at ni.com>; u-boot <u-
> boot at lists.denx.de>
> Subject: Re: [U-Boot] [PATCH v3 2/6] driver: net: fsl-mc: remove unused
> strcture elements
>
> On Tue, Oct 9, 2018 at 9:59 PM Pankaj Bansal <pankaj.bansal at nxp.com>
> wrote:
> >
> > The phydev structure is present in both ldpaa_eth_priv and
> > wriop_dpmac_info. the phydev in wriop_dpmac_info is not being used
> >
> > As the phydev is created based on phy_addr and bus members of
> > wriop_dpmac_info, it is appropriate to keep phydev in
> wriop_dpmac_info.
> >
> > Also phy_regs is not being used, therefore remove it
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal at nxp.com>
> > Acked-by: Joe Hershberger <joe.hershberger at ni.com>
> > ---
> >
> > Notes:
> > V3:
> > - No change
>
> Please be sure you are running scripts/checkpatch.pl on your patches.
> v2 had a number of issues I had to fix up. I'm pretty sure this was one of
> them.
I had run checkpatch script on all the versions of patches I sent.
I use this command "./scripts/checkpatch.pl 0001-something.patch"
This "no return at the end of function" issue was not reported by checkpatch script.
Can you please tell me which issues in V2 are you referring to?
Because when I ran checkpatch.pl, it gave me no errors or warnings but 7 checks regarding alignment in board/freescale/ls2080aqds/eth.c.
I did not do any changes for that because that code was not part of my patch and I think that was done
so that line doesn't exceed 80 characters.
>
> You would do yourself a favor to use tools/patman/patman.
This is a good advice. I will use patman from now onwards to prepare and send patches.
>
> > V2:
> > - change (phydev && bus != NULL) to (phydev && bus)
> > - after free phydev just pass NULL into wriop_set_phy_dev()
> >
> > drivers/net/ldpaa_eth/ldpaa_eth.c | 56 +++++++++++++++------------
> > drivers/net/ldpaa_eth/ldpaa_eth.h | 1 -
> > drivers/net/ldpaa_eth/ldpaa_wriop.c | 2 +
> > include/fsl-mc/ldpaa_wriop.h | 1 -
> > 4 files changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/ldpaa_eth/ldpaa_eth.c
> > b/drivers/net/ldpaa_eth/ldpaa_eth.c
> > index 82a684bea2..ca3459cc33 100644
> > --- a/drivers/net/ldpaa_eth/ldpaa_eth.c
> > +++ b/drivers/net/ldpaa_eth/ldpaa_eth.c
> > @@ -35,7 +35,7 @@ static int init_phy(struct eth_device *dev)
> > return -1;
> > }
> >
> > - priv->phydev = phydev;
> > + wriop_set_phy_dev(priv->dpmac_id, phydev);
> >
> > return phy_config(phydev);
> > }
> > @@ -388,6 +388,7 @@ static int ldpaa_eth_open(struct eth_device
> *net_dev, bd_t *bd)
> > struct mii_dev *bus;
> > phy_interface_t enet_if;
> > struct dpni_queue d_queue;
> > + struct phy_device *phydev = NULL;
> >
> > if (net_dev->state == ETH_STATE_ACTIVE)
> > return 0;
> > @@ -408,38 +409,41 @@ static int ldpaa_eth_open(struct eth_device
> *net_dev, bd_t *bd)
> > goto err_dpmac_setup;
> >
> > #ifdef CONFIG_PHYLIB
> > - if (priv->phydev) {
> > - err = phy_startup(priv->phydev);
> > + phydev = wriop_get_phy_dev(priv->dpmac_id);
> > + if (phydev) {
> > + err = phy_startup(phydev);
> > if (err) {
> > printf("%s: Could not initialize\n",
> > - priv->phydev->dev->name);
> > + phydev->dev->name);
> > goto err_dpmac_bind;
> > }
> > }
> > #else
> > - priv->phydev = (struct phy_device *)malloc(sizeof(struct
> phy_device));
> > - memset(priv->phydev, 0, sizeof(struct phy_device));
> > + phydev = (struct phy_device *)malloc(sizeof(struct phy_device));
> > + memset(phydev, 0, sizeof(struct phy_device));
> > + wriop_set_phy_dev(priv->dpmac_id, phydev);
> >
> > - priv->phydev->speed = SPEED_1000;
> > - priv->phydev->link = 1;
> > - priv->phydev->duplex = DUPLEX_FULL;
> > + phydev->speed = SPEED_1000;
> > + phydev->link = 1;
> > + phydev->duplex = DUPLEX_FULL;
> > #endif
> >
> > bus = wriop_get_mdio(priv->dpmac_id);
> > enet_if = wriop_get_enet_if(priv->dpmac_id);
> > if ((bus == NULL) &&
> > (enet_if == PHY_INTERFACE_MODE_XGMII)) {
> > - priv->phydev = (struct phy_device *)
> > + phydev = (struct phy_device *)
> > malloc(sizeof(struct phy_device));
> > - memset(priv->phydev, 0, sizeof(struct phy_device));
> > + memset(phydev, 0, sizeof(struct phy_device));
> > + wriop_set_phy_dev(priv->dpmac_id, phydev);
> >
> > - priv->phydev->speed = SPEED_10000;
> > - priv->phydev->link = 1;
> > - priv->phydev->duplex = DUPLEX_FULL;
> > + phydev->speed = SPEED_10000;
> > + phydev->link = 1;
> > + phydev->duplex = DUPLEX_FULL;
> > }
> >
> > - if (!priv->phydev->link) {
> > - printf("%s: No link.\n", priv->phydev->dev->name);
> > + if (!phydev->link) {
> > + printf("%s: No link.\n", phydev->dev->name);
> > err = -1;
> > goto err_dpmac_bind;
> > }
> > @@ -476,17 +480,17 @@ static int ldpaa_eth_open(struct eth_device
> *net_dev, bd_t *bd)
> > return err;
> > }
> >
> > - dpmac_link_state.rate = priv->phydev->speed;
> > + dpmac_link_state.rate = phydev->speed;
> >
> > - if (priv->phydev->autoneg == AUTONEG_DISABLE)
> > + if (phydev->autoneg == AUTONEG_DISABLE)
> > dpmac_link_state.options &= ~DPMAC_LINK_OPT_AUTONEG;
> > else
> > dpmac_link_state.options |= DPMAC_LINK_OPT_AUTONEG;
> >
> > - if (priv->phydev->duplex == DUPLEX_HALF)
> > + if (phydev->duplex == DUPLEX_HALF)
> > dpmac_link_state.options |=
> > DPMAC_LINK_OPT_HALF_DUPLEX;
> >
> > - dpmac_link_state.up = priv->phydev->link;
> > + dpmac_link_state.up = phydev->link;
> >
> > err = dpmac_set_link_state(dflt_mc_io, MC_CMD_NO_FLAGS,
> > priv->dpmac_handle,
> > &dpmac_link_state); @@ -530,7 +534,7 @@ static int
> ldpaa_eth_open(struct eth_device *net_dev, bd_t *bd)
> > goto err_qdid;
> > }
> >
> > - return priv->phydev->link;
> > + return phydev->link;
> >
> > err_qdid:
> > err_get_queue:
> > @@ -556,6 +560,7 @@ static void ldpaa_eth_stop(struct eth_device
> > *net_dev) #ifdef CONFIG_PHYLIB
> > struct mii_dev *bus = wriop_get_mdio(priv->dpmac_id); #endif
> > + struct phy_device *phydev = NULL;
> >
> > if ((net_dev->state == ETH_STATE_PASSIVE) ||
> > (net_dev->state == ETH_STATE_INIT)) @@ -588,11 +593,12 @@
> > static void ldpaa_eth_stop(struct eth_device *net_dev)
> > printf("dpni_disable() failed\n");
> >
> > #ifdef CONFIG_PHYLIB
> > - if (priv->phydev && bus != NULL)
> > - phy_shutdown(priv->phydev);
> > + phydev = wriop_get_phy_dev(priv->dpmac_id);
> > + if (phydev && bus)
> > + phy_shutdown(phydev);
> > else {
> > - free(priv->phydev);
> > - priv->phydev = NULL;
> > + free(phydev);
> > + wriop_set_phy_dev(priv->dpmac_id, NULL);
> > }
> > #endif
> >
> > diff --git a/drivers/net/ldpaa_eth/ldpaa_eth.h
> > b/drivers/net/ldpaa_eth/ldpaa_eth.h
> > index ee784a55ee..3f9154b5bb 100644
> > --- a/drivers/net/ldpaa_eth/ldpaa_eth.h
> > +++ b/drivers/net/ldpaa_eth/ldpaa_eth.h
> > @@ -127,7 +127,6 @@ struct ldpaa_eth_priv {
> > uint16_t tx_flow_id;
> >
> > enum ldpaa_eth_type type; /* 1G or 10G ethernet */
> > - struct phy_device *phydev;
> > };
> >
> > struct dprc_endpoint dpmac_endpoint;
> > diff --git a/drivers/net/ldpaa_eth/ldpaa_wriop.c
> > b/drivers/net/ldpaa_eth/ldpaa_wriop.c
> > index 0731a795c8..afbb1ca91e 100644
> > --- a/drivers/net/ldpaa_eth/ldpaa_wriop.c
> > +++ b/drivers/net/ldpaa_eth/ldpaa_wriop.c
> > @@ -26,6 +26,7 @@ void wriop_init_dpmac(int sd, int dpmac_id, int
> lane_prtcl)
> > dpmac_info[dpmac_id].enabled = 0;
> > dpmac_info[dpmac_id].id = 0;
> > dpmac_info[dpmac_id].phy_addr = -1;
> > + dpmac_info[dpmac_id].phydev = NULL;
> > dpmac_info[dpmac_id].enet_if = PHY_INTERFACE_MODE_NONE;
> >
> > enet_if = wriop_dpmac_enet_if(dpmac_id, lane_prtcl); @@ -42,6
> > +43,7 @@ void wriop_init_dpmac_enet_if(int dpmac_id, phy_interface_t
> enet_if)
> > dpmac_info[dpmac_id].id = dpmac_id;
> > dpmac_info[dpmac_id].phy_addr = -1;
> > dpmac_info[dpmac_id].enet_if = enet_if;
> > + dpmac_info[dpmac_id].phydev = NULL;
> > }
> >
> >
> > diff --git a/include/fsl-mc/ldpaa_wriop.h
> > b/include/fsl-mc/ldpaa_wriop.h index 07e5130264..8971c6c55b 100644
> > --- a/include/fsl-mc/ldpaa_wriop.h
> > +++ b/include/fsl-mc/ldpaa_wriop.h
> > @@ -41,7 +41,6 @@ struct wriop_dpmac_info {
> > u8 id;
> > u8 board_mux;
> > int phy_addr;
> > - void *phy_regs;
> > phy_interface_t enet_if;
> > struct phy_device *phydev;
> > struct mii_dev *bus;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > ts.denx.de%2Flistinfo%2Fu-
> boot&data=02%7C01%7Cpankaj.bansal%40nxp.
> >
> com%7Cbced1ce32f8e455148d408d62e64c7f5%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C636747407731429023&sdata=4cwaAt8C4V91TNo
> iNFGcoznoGEn
> > POsCGDmh9maFv%2FOA%3D&reserved=0
More information about the U-Boot
mailing list