Re: [PATCH 1/2] net: phy: core: use genphy version of callbacks read_status and config_aneg per default

[Florian Fainelli <f.fainelli@gmail.com>]

On 11/15/2017 01:42 PM, Heiner Kallweit wrote:
> read_status and config_aneg are the only mandatory callbacks and most
> of the time the generic implementation is used by drivers.
> So make the core fall back to the generic version if a driver doesn't
> implement the respective callback.
> 
> Also currently the core doesn't seem to verify that drivers implement
> the mandatory calls. If a driver doesn't do so we'd just get a NPE.

Right, which is not an unusual way to signal that something is mandatory.

> With this patch this potential issue doesn't exit any longer.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Note that net-next is closed at the moment, so you will have to resubmit
this when the tree opens back again.

> ---
>  drivers/net/phy/phy.c |  5 ++++-
>  include/linux/phy.h   | 33 ++++++++++++++++++---------------
>  2 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 2b1e67bc1..a0e7605dc 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -493,7 +493,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
>  	/* Invalidate LP advertising flags */
>  	phydev->lp_advertising = 0;
>  
> -	err = phydev->drv->config_aneg(phydev);
> +	if (phydev->drv->config_aneg)
> +		err = phydev->drv->config_aneg(phydev);
> +	else
> +		err = genphy_config_aneg(phydev);
>  	if (err < 0)
>  		goto out_unlock;
>  
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index dc82a07cb..958b5162a 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -497,13 +497,13 @@ struct phy_device {
>   * flags: A bitfield defining certain other features this PHY
>   *   supports (like interrupts)
>   *
> - * The drivers must implement config_aneg and read_status.  All
> - * other functions are optional. Note that none of these
> - * functions should be called from interrupt time.  The goal is
> - * for the bus read/write functions to be able to block when the
> - * bus transaction is happening, and be freed up by an interrupt
> - * (The MPC85xx has this ability, though it is not currently
> - * supported in the driver).
> + * All functions are optional. If config_aneg or read_status
> + * are not implemented, the phy core uses the genphy versions.
> + * Note that none of these functions should be called from
> + * interrupt time. The goal is for the bus read/write functions
> + * to be able to block when the bus transaction is happening,
> + * and be freed up by an interrupt (The MPC85xx has this ability,
> + * though it is not currently supported in the driver).
>   */
>  struct phy_driver {
>  	struct mdio_driver_common mdiodrv;
> @@ -841,14 +841,6 @@ int phy_aneg_done(struct phy_device *phydev);
>  int phy_stop_interrupts(struct phy_device *phydev);
>  int phy_restart_aneg(struct phy_device *phydev);
>  
> -static inline int phy_read_status(struct phy_device *phydev)
> -{
> -	if (!phydev->drv)
> -		return -EIO;
> -
> -	return phydev->drv->read_status(phydev);
> -}
> -
>  #define phydev_err(_phydev, format, args...)	\
>  	dev_err(&_phydev->mdio.dev, format, ##args)
>  
> @@ -890,6 +882,17 @@ int genphy_c45_read_pma(struct phy_device *phydev);
>  int genphy_c45_pma_setup_forced(struct phy_device *phydev);
>  int genphy_c45_an_disable_aneg(struct phy_device *phydev);
>  
> +static inline int phy_read_status(struct phy_device *phydev)
> +{
> +	if (!phydev->drv)
> +		return -EIO;
> +
> +	if (phydev->drv->read_status)
> +		return phydev->drv->read_status(phydev);
> +	else
> +		return genphy_read_status(phydev);
> +}
> +
>  void phy_driver_unregister(struct phy_driver *drv);
>  void phy_drivers_unregister(struct phy_driver *drv, int n);
>  int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
> 


-- 
Florian