From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH 1/2] net: phy: core: use genphy version of callbacks read_status and config_aneg per default Date: Wed, 29 Nov 2017 21:47:16 +0100 Message-ID: <8aab2425-9738-f54c-dd57-2cf38236ae3f@gmail.com> References: <1b1e4842-690c-95a7-0d07-d596ce97bba5@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" To: Florian Fainelli , David Miller , Andrew Lunn Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:39557 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752361AbdK2Uua (ORCPT ); Wed, 29 Nov 2017 15:50:30 -0500 Received: by mail-wm0-f68.google.com with SMTP id i11so8507261wmf.4 for ; Wed, 29 Nov 2017 12:50:29 -0800 (PST) In-Reply-To: <1b1e4842-690c-95a7-0d07-d596ce97bba5@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: Am 15.11.2017 um 22:56 schrieb Florian Fainelli: > 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 > > Reviewed-by: Florian Fainelli > > Note that net-next is closed at the moment, so you will have to resubmit > this when the tree opens back again. > I see that the two patches have status "deferred" in patchwork. So do I have to actually resubmit or are they going to be be picked up from patchwork? Rgds, Heiner >> --- >> 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); >> > >