[U-Boot] [PATCH v3 2/6] driver: net: fsl-mc: remove unused strcture elements
Joe Hershberger
joe.hershberger at ni.com
Wed Oct 10 18:35:14 UTC 2018
On Wed, Oct 10, 2018 at 12:11 AM Pankaj Bansal <pankaj.bansal at nxp.com> wrote:
>
> 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.
This patch specifically had a complaint "CHECK: braces {} should be
used on all arms of this statement" that I fixed up.
This issue doesn't last long, since the code in question is fixed in
"driver: net: fsl-mc: Modify the dpmac link detection method", but we
prefer not to have patches that have issues. patman will tell you
about it.
I've applied v3 and it looks like "Freescale AArch64" is still warning
( https://travis-ci.org/jhershbe/u-boot/jobs/439762814 ) ...
---------
aarch64: + ls2080a_emu
+drivers/net/ldpaa_eth/ldpaa_eth.c: In function 'ldpaa_get_dpmac_state':
+drivers/net/ldpaa_eth/ldpaa_eth.c:408:6: error: unused variable 'err'
[-Werror=unused-variable]
+ int err;
+ ^~~
+drivers/net/ldpaa_eth/ldpaa_eth.c:407:6: error: unused variable
'phy_num' [-Werror=unused-variable]
+ int phy_num, phys_detected;
+ ^~~~~~~
+drivers/net/ldpaa_eth/ldpaa_eth.c:405:21: error: unused variable
'phydev' [-Werror=unused-variable]
+ struct phy_device *phydev = NULL;
+ ^~~~~~
+drivers/net/ldpaa_eth/ldpaa_eth.c: In function 'ldpaa_eth_stop':
+drivers/net/ldpaa_eth/ldpaa_eth.c:594:6: error: unused variable
'phy_num' [-Werror=unused-variable]
+ int phy_num;
+drivers/net/ldpaa_eth/ldpaa_eth.c:593:21: error: unused variable
'phydev' [-Werror=unused-variable]
+cc1: all warnings being treated as errors
+make[3]: *** [drivers/net/ldpaa_eth/ldpaa_eth.o] Error 1
+make[2]: *** [drivers/net/ldpaa_eth] Error 2
+make[1]: *** [drivers/net] Error 2
+make: *** [sub-make] Error 2
---------
Both "driver: net: fsl-mc: Modify the dpmac link detection method" and
"driver: net: fsl-mc: Add support of multiple phys for dpmac"
introduce unused variable warnings on ls2080a_emu.
-Joe
>
> >
> > 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list