Re: [PATCH v2 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable

[Andrew Lunn]

On Wed, Sep 04, 2019 at 07:23:22PM +0300, Alexandru Ardelean wrote:
> This driver becomes the first user of the kernel's `ETHTOOL_PHY_EDPD`
> phy-tunable feature.
> EDPD is also enabled by default on PHY config_init, but can be disabled via
> the phy-tunable control.
> 
> When enabling EDPD, it's also a good idea (for the ADIN PHYs) to enable TX
> periodic pulses, so that in case the other PHY is also on EDPD mode, there
> is no lock-up situation where both sides are waiting for the other to
> transmit.
> 
> Via the phy-tunable control, TX pulses can be disabled if specifying 0
> `tx-interval` via ethtool.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> ---
>  drivers/net/phy/adin.c | 50 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 4dec83df048d..742728ab2a5d 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -26,6 +26,11 @@
>  
>  #define ADIN1300_RX_ERR_CNT			0x0014
>  
> +#define ADIN1300_PHY_CTRL_STATUS2		0x0015
> +#define   ADIN1300_NRG_PD_EN			BIT(3)
> +#define   ADIN1300_NRG_PD_TX_EN			BIT(2)
> +#define   ADIN1300_NRG_PD_STATUS		BIT(1)
> +
>  #define ADIN1300_PHY_CTRL2			0x0016
>  #define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
>  #define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
> @@ -328,12 +333,51 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt)
>  			    ADIN1300_DOWNSPEEDS_EN);
>  }
>  
> +static int adin_get_edpd(struct phy_device *phydev, u16 *tx_interval)
> +{
> +	int val;
> +
> +	val = phy_read(phydev, ADIN1300_PHY_CTRL_STATUS2);
> +	if (val < 0)
> +		return val;
> +
> +	if (ADIN1300_NRG_PD_EN & val) {
> +		if (val & ADIN1300_NRG_PD_TX_EN)
> +			*tx_interval = 1;

What does 1 mean? 1 pico second, one hour? Anything but zero seconds?
Does the datasheet specify what it actually does? Maybe you should be
using ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL here, to indicate you actually
have no idea, but it is the default for this PHY?

> +		else
> +			*tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
> +	} else {
> +		*tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
> +{
> +	u16 val;
> +
> +	if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
> +		return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
> +				(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
> +
> +	val = ADIN1300_NRG_PD_EN;
> +	if (tx_interval != ETHTOOL_PHY_EDPD_NO_TX)
> +		val |= ADIN1300_NRG_PD_TX_EN;

So you silently accept any interval? That sounds wrong. You really
should be returning -EINVAL for any value other than, either 1, or
maybe ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, if you change the get
function.

      Andrew