[PATCH 1/2] net: ethernet: marvell: pxa168_eth: use phydev from struct net_device

[PATCH 1/2] net: ethernet: marvell: pxa168_eth: use phydev from struct net_device

[Philippe Reynes]

The private structure contain a pointer to phydev, but the structure
net_device already contain such pointer. So we can remove the pointer
phydev in the private structure, and update the driver to use the
one contained in struct net_device.

Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c |   36 +++++++++++++----------------
 1 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 54d5154..d466326 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -247,7 +247,6 @@ struct pxa168_eth_private {
 	 */
 	struct timer_list timeout;
 	struct mii_bus *smi_bus;
-	struct phy_device *phy;
 
 	/* clock */
 	struct clk *clk;
@@ -644,7 +643,7 @@ static void eth_port_start(struct net_device *dev)
 	struct pxa168_eth_private *pep = netdev_priv(dev);
 	int tx_curr_desc, rx_curr_desc;
 
-	phy_start(pep->phy);
+	phy_start(dev->phydev);
 
 	/* Assignment of Tx CTRP of given queue */
 	tx_curr_desc = pep->tx_curr_desc_q;
@@ -700,7 +699,7 @@ static void eth_port_reset(struct net_device *dev)
 	val &= ~PCR_EN;
 	wrl(pep, PORT_CONFIG, val);
 
-	phy_stop(pep->phy);
+	phy_stop(dev->phydev);
 }
 
 /*
@@ -943,7 +942,7 @@ static int set_port_config_ext(struct pxa168_eth_private *pep)
 static void pxa168_eth_adjust_link(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
-	struct phy_device *phy = pep->phy;
+	struct phy_device *phy = dev->phydev;
 	u32 cfg, cfg_o = rdl(pep, PORT_CONFIG);
 	u32 cfgext, cfgext_o = rdl(pep, PORT_CONFIG_EXT);
 
@@ -973,16 +972,17 @@ static int pxa168_init_phy(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
 	struct ethtool_cmd cmd;
+	struct phy_device *phy = NULL;
 	int err;
 
-	if (pep->phy)
+	if (dev->phydev)
 		return 0;
 
-	pep->phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
-	if (IS_ERR(pep->phy))
-		return PTR_ERR(pep->phy);
+	phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
 
-	err = phy_connect_direct(dev, pep->phy, pxa168_eth_adjust_link,
+	err = phy_connect_direct(dev, phy, pxa168_eth_adjust_link,
 				 pep->phy_intf);
 	if (err)
 		return err;
@@ -1366,30 +1366,26 @@ static int pxa168_smi_write(struct mii_bus *bus, int phy_addr, int regnum,
 static int pxa168_eth_do_ioctl(struct net_device *dev, struct ifreq *ifr,
 			       int cmd)
 {
-	struct pxa168_eth_private *pep = netdev_priv(dev);
-	if (pep->phy != NULL)
-		return phy_mii_ioctl(pep->phy, ifr, cmd);
+	if (dev->phydev != NULL)
+		return phy_mii_ioctl(dev->phydev, ifr, cmd);
 
 	return -EOPNOTSUPP;
 }
 
 static int pxa168_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
-	struct pxa168_eth_private *pep = netdev_priv(dev);
 	int err;
 
-	err = phy_read_status(pep->phy);
+	err = phy_read_status(dev->phydev);
 	if (err == 0)
-		err = phy_ethtool_gset(pep->phy, cmd);
+		err = phy_ethtool_gset(dev->phydev, cmd);
 
 	return err;
 }
 
 static int pxa168_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
-	struct pxa168_eth_private *pep = netdev_priv(dev);
-
-	return phy_ethtool_sset(pep->phy, cmd);
+	return phy_ethtool_sset(dev->phydev, cmd);
 }
 
 static void pxa168_get_drvinfo(struct net_device *dev,
@@ -1569,8 +1565,8 @@ static int pxa168_eth_remove(struct platform_device *pdev)
 				  pep->htpr, pep->htpr_dma);
 		pep->htpr = NULL;
 	}
-	if (pep->phy)
-		phy_disconnect(pep->phy);
+	if (dev->phydev)
+		phy_disconnect(dev->phydev);
 	if (pep->clk) {
 		clk_disable_unprepare(pep->clk);
 	}
-- 
1.7.4.4

[PATCH 2/2] net: ethernet: marvell: pxa168_eth: use phy_ethtool_{get|set}_link_ksettings

[Philippe Reynes]

There are two generics functions phy_ethtool_{get|set}_link_ksettings,
so we can use them instead of defining the same code in the driver.

Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c |   39 +++++++++++++---------------
 1 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index d466326..aeeb2e7 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -274,8 +274,8 @@ enum hash_table_entry {
 	HASH_ENTRY_RECEIVE_DISCARD_BIT = 2
 };
 
-static int pxa168_get_settings(struct net_device *dev, struct ethtool_cmd *cmd);
-static int pxa168_set_settings(struct net_device *dev, struct ethtool_cmd *cmd);
+static int pxa168_get_link_ksettings(struct net_device *dev,
+				     struct ethtool_link_ksettings *cmd);
 static int pxa168_init_hw(struct pxa168_eth_private *pep);
 static int pxa168_init_phy(struct net_device *dev);
 static void eth_port_reset(struct net_device *dev);
@@ -971,7 +971,7 @@ static void pxa168_eth_adjust_link(struct net_device *dev)
 static int pxa168_init_phy(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
-	struct ethtool_cmd cmd;
+	struct ethtool_link_ksettings cmd;
 	struct phy_device *phy = NULL;
 	int err;
 
@@ -987,20 +987,21 @@ static int pxa168_init_phy(struct net_device *dev)
 	if (err)
 		return err;
 
-	err = pxa168_get_settings(dev, &cmd);
+	err = pxa168_get_link_ksettings(dev, &cmd);
 	if (err)
 		return err;
 
-	cmd.phy_address = pep->phy_addr;
-	cmd.speed = pep->phy_speed;
-	cmd.duplex = pep->phy_duplex;
-	cmd.advertising = PHY_BASIC_FEATURES;
-	cmd.autoneg = AUTONEG_ENABLE;
+	cmd.base.phy_address = pep->phy_addr;
+	cmd.base.speed = pep->phy_speed;
+	cmd.base.duplex = pep->phy_duplex;
+	ethtool_convert_legacy_u32_to_link_mode(cmd.link_modes.advertising,
+						PHY_BASIC_FEATURES);
+	cmd.base.autoneg = AUTONEG_ENABLE;
 
-	if (cmd.speed != 0)
-		cmd.autoneg = AUTONEG_DISABLE;
+	if (cmd.base.speed != 0)
+		cmd.base.autoneg = AUTONEG_DISABLE;
 
-	return pxa168_set_settings(dev, &cmd);
+	return phy_ethtool_set_link_ksettings(dev, &cmd);
 }
 
 static int pxa168_init_hw(struct pxa168_eth_private *pep)
@@ -1372,22 +1373,18 @@ static int pxa168_eth_do_ioctl(struct net_device *dev, struct ifreq *ifr,
 	return -EOPNOTSUPP;
 }
 
-static int pxa168_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+static int pxa168_get_link_ksettings(struct net_device *dev,
+				     struct ethtool_link_ksettings *cmd)
 {
 	int err;
 
 	err = phy_read_status(dev->phydev);
 	if (err == 0)
-		err = phy_ethtool_gset(dev->phydev, cmd);
+		err = phy_ethtool_ksettings_get(dev->phydev, cmd);
 
 	return err;
 }
 
-static int pxa168_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
-{
-	return phy_ethtool_sset(dev->phydev, cmd);
-}
-
 static void pxa168_get_drvinfo(struct net_device *dev,
 			       struct ethtool_drvinfo *info)
 {
@@ -1398,11 +1395,11 @@ static void pxa168_get_drvinfo(struct net_device *dev,
 }
 
 static const struct ethtool_ops pxa168_ethtool_ops = {
-	.get_settings	= pxa168_get_settings,
-	.set_settings	= pxa168_set_settings,
 	.get_drvinfo	= pxa168_get_drvinfo,
 	.get_link	= ethtool_op_get_link,
 	.get_ts_info	= ethtool_op_get_ts_info,
+	.get_link_ksettings = pxa168_get_link_ksettings,
+	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 };
 
 static const struct net_device_ops pxa168_eth_netdev_ops = {
-- 
1.7.4.4

Re: [PATCH 1/2] net: ethernet: marvell: pxa168_eth: use phydev from struct net_device

[David Miller]

From: Philippe Reynes <tremyfr@gmail.com>
Date: Sun, 17 Jul 2016 23:30:45 +0200

> The private structure contain a pointer to phydev, but the structure
> net_device already contain such pointer. So we can remove the pointer
> phydev in the private structure, and update the driver to use the
> one contained in struct net_device.
> 
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>

Applied.

Re: [PATCH 2/2] net: ethernet: marvell: pxa168_eth: use phy_ethtool_{get|set}_link_ksettings

[David Miller]

From: Philippe Reynes <tremyfr@gmail.com>
Date: Sun, 17 Jul 2016 23:30:46 +0200

> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> so we can use them instead of defining the same code in the driver.
> 
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>

Applied.

Re: [PATCH 1/2] net: ethernet: marvell: pxa168_eth: use phydev from struct net_device

[Sergei Shtylyov]

Hello.

On 7/18/2016 12:30 AM, Philippe Reynes wrote:

> The private structure contain a pointer to phydev, but the structure
> net_device already contain such pointer. So we can remove the pointer
> phydev in the private structure, and update the driver to use the
> one contained in struct net_device.
>
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
> ---
>  drivers/net/ethernet/marvell/pxa168_eth.c |   36 +++++++++++++----------------
>  1 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 54d5154..d466326 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
[...]
> @@ -973,16 +972,17 @@ static int pxa168_init_phy(struct net_device *dev)
>  {
>  	struct pxa168_eth_private *pep = netdev_priv(dev);
>  	struct ethtool_cmd cmd;
> +	struct phy_device *phy = NULL;

   Initializer not really needed.

>  	int err;
>
> -	if (pep->phy)
> +	if (dev->phydev)
>  		return 0;
>
> -	pep->phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
> -	if (IS_ERR(pep->phy))
> -		return PTR_ERR(pep->phy);
> +	phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
>
> -	err = phy_connect_direct(dev, pep->phy, pxa168_eth_adjust_link,
> +	err = phy_connect_direct(dev, phy, pxa168_eth_adjust_link,
>  				 pep->phy_intf);
>  	if (err)
>  		return err;

    Hm, where do you assign 'dev->phydev'?

[...]

MBR, Sergei

Re: [PATCH 1/2] net: ethernet: marvell: pxa168_eth: use phydev from struct net_device

[Philippe Reynes]

Hi,

On 18/07/16 12:14, Sergei Shtylyov wrote:
> Hello.
>
> On 7/18/2016 12:30 AM, Philippe Reynes wrote:
>
>> The private structure contain a pointer to phydev, but the structure
>> net_device already contain such pointer. So we can remove the pointer
>> phydev in the private structure, and update the driver to use the
>> one contained in struct net_device.
>>
>> Signed-off-by: Philippe Reynes<tremyfr@gmail.com>
>> ---
>>   drivers/net/ethernet/marvell/pxa168_eth.c |   36 +++++++++++++----------------
>>   1 files changed, 16 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
>> index 54d5154..d466326 100644
>> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
>> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> [...]
>> @@ -973,16 +972,17 @@ static int pxa168_init_phy(struct net_device *dev)
>>   {
>>   	struct pxa168_eth_private *pep = netdev_priv(dev);
>>   	struct ethtool_cmd cmd;
>> +	struct phy_device *phy = NULL;
>
>     Initializer not really needed.

You're right, the first line using phy is an assign, so it's not really usefull.
  
>>   	int err;
>>
>> -	if (pep->phy)
>> +	if (dev->phydev)
>>   		return 0;
>>
>> -	pep->phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
>> -	if (IS_ERR(pep->phy))
>> -		return PTR_ERR(pep->phy);
>> +	phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
>> +	if (IS_ERR(phy))
>> +		return PTR_ERR(phy);
>>
>> -	err = phy_connect_direct(dev, pep->phy, pxa168_eth_adjust_link,
>> +	err = phy_connect_direct(dev, phy, pxa168_eth_adjust_link,
>>   				 pep->phy_intf);
>>   	if (err)
>>   		return err;
>
>      Hm, where do you assign 'dev->phydev'?

dev-> phydev is assigned in phy_connect_direct. In fact, phy_connect_direct call
phy_attach_direct, and this last function assign phydev to dev->phydev.

> [...]
>
> MBR, Sergei
>

Regards,
Philippe

Re: [PATCH 2/2] net: ethernet: marvell: pxa168_eth: use phy_ethtool_{get|set}_link_ksettings

[Florian Fainelli]

On 07/17/2016 02:30 PM, Philippe Reynes wrote:
> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> so we can use them instead of defining the same code in the driver.
> 
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
> ---
> -	cmd.phy_address = pep->phy_addr;
> -	cmd.speed = pep->phy_speed;
> -	cmd.duplex = pep->phy_duplex;
> -	cmd.advertising = PHY_BASIC_FEATURES;
> -	cmd.autoneg = AUTONEG_ENABLE;
> +	cmd.base.phy_address = pep->phy_addr;
> +	cmd.base.speed = pep->phy_speed;
> +	cmd.base.duplex = pep->phy_duplex;
> +	ethtool_convert_legacy_u32_to_link_mode(cmd.link_modes.advertising,
> +						PHY_BASIC_FEATURES);
> +	cmd.base.autoneg = AUTONEG_ENABLE;
>  
> -	if (cmd.speed != 0)
> -		cmd.autoneg = AUTONEG_DISABLE;
> +	if (cmd.base.speed != 0)
> +		cmd.base.autoneg = AUTONEG_DISABLE;
>  
> -	return pxa168_set_settings(dev, &cmd);
> +	return phy_ethtool_set_link_ksettings(dev, &cmd);

This duplicates quite a bit of code that the core PHYLIB already deals
with, you should plan for a subsequent cleanup patch which removes all
of this.
-- 
Florian