* [PATCH] net: phy: at803x: stop switching phy delay config needlessly @ 2019-08-09 0:57 André Draszik 2019-08-09 9:43 ` Vladimir Oltean 2019-08-09 11:20 ` [PATCH v2] " André Draszik 0 siblings, 2 replies; 7+ messages in thread From: André Draszik @ 2019-08-09 0:57 UTC (permalink / raw) To: linux-kernel Cc: André Draszik, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev This driver does a funny dance disabling and re-enabling RX and/or TX delays. In any of the RGMII-ID modes, it first disables the delays, just to re-enable them again right away. This looks like a needless exercise. Just enable the respective delays when in any of the relevant 'id' modes, and disable them otherwise. Also, remove comments which don't add anything that can't be seen by looking at the code. Signed-off-by: André Draszik <git@andred.net> CC: Andrew Lunn <andrew@lunn.ch> CC: Florian Fainelli <f.fainelli@gmail.com> CC: Heiner Kallweit <hkallweit1@gmail.com> CC: "David S. Miller" <davem@davemloft.net> CC: netdev@vger.kernel.org --- drivers/net/phy/at803x.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 222ccd9ecfce..2ab51f552e92 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -257,35 +257,21 @@ static int at803x_config_init(struct phy_device *phydev) * after HW reset: RX delay enabled and TX delay disabled * after SW reset: RX delay enabled, while TX delay retains the * value before reset. - * - * So let's first disable the RX and TX delays in PHY and enable - * them based on the mode selected (this also takes care of RGMII - * mode where we expect delays to be disabled) */ - - ret = at803x_disable_rx_delay(phydev); - if (ret < 0) - return ret; - ret = at803x_disable_tx_delay(phydev); - if (ret < 0) - return ret; - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, - * otherwise keep it disabled - */ ret = at803x_enable_rx_delay(phydev); - if (ret < 0) - return ret; + } else { + ret = at803x_disable_rx_delay(phydev); } + if (ret < 0) + return ret; if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, - * otherwise keep it disabled - */ ret = at803x_enable_tx_delay(phydev); + } else { + ret = at803x_disable_tx_delay(phydev); } return ret; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 0:57 [PATCH] net: phy: at803x: stop switching phy delay config needlessly André Draszik @ 2019-08-09 9:43 ` Vladimir Oltean 2019-08-09 10:00 ` André Draszik 2019-08-09 11:20 ` [PATCH v2] " André Draszik 1 sibling, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2019-08-09 9:43 UTC (permalink / raw) To: André Draszik Cc: lkml, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev Hi Andre, On Fri, 9 Aug 2019 at 03:58, André Draszik <git@andred.net> wrote: > > This driver does a funny dance disabling and re-enabling > RX and/or TX delays. In any of the RGMII-ID modes, it first > disables the delays, just to re-enable them again right > away. This looks like a needless exercise. > > Just enable the respective delays when in any of the > relevant 'id' modes, and disable them otherwise. > > Also, remove comments which don't add anything that can't be > seen by looking at the code. > > Signed-off-by: André Draszik <git@andred.net> > CC: Andrew Lunn <andrew@lunn.ch> > CC: Florian Fainelli <f.fainelli@gmail.com> > CC: Heiner Kallweit <hkallweit1@gmail.com> > CC: "David S. Miller" <davem@davemloft.net> > CC: netdev@vger.kernel.org > --- Is there any particular problem you're facing? Does this make any difference? > drivers/net/phy/at803x.c | 26 ++++++-------------------- > 1 file changed, 6 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 222ccd9ecfce..2ab51f552e92 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -257,35 +257,21 @@ static int at803x_config_init(struct phy_device *phydev) > * after HW reset: RX delay enabled and TX delay disabled > * after SW reset: RX delay enabled, while TX delay retains the > * value before reset. > - * > - * So let's first disable the RX and TX delays in PHY and enable > - * them based on the mode selected (this also takes care of RGMII > - * mode where we expect delays to be disabled) > */ > - > - ret = at803x_disable_rx_delay(phydev); > - if (ret < 0) > - return ret; > - ret = at803x_disable_tx_delay(phydev); > - if (ret < 0) > - return ret; > - > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > - * otherwise keep it disabled > - */ > ret = at803x_enable_rx_delay(phydev); > - if (ret < 0) > - return ret; > + } else { > + ret = at803x_disable_rx_delay(phydev); > } > + if (ret < 0) > + return ret; > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > - * otherwise keep it disabled > - */ > ret = at803x_enable_tx_delay(phydev); > + } else { > + ret = at803x_disable_tx_delay(phydev); > } > > return ret; > -- > 2.20.1 > Regards, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 9:43 ` Vladimir Oltean @ 2019-08-09 10:00 ` André Draszik 2019-08-09 11:09 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: André Draszik @ 2019-08-09 10:00 UTC (permalink / raw) To: Vladimir Oltean Cc: lkml, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev Hi Vladimir, On Fri, 2019-08-09 at 12:43 +0300, Vladimir Oltean wrote: > Hi Andre, > > On Fri, 9 Aug 2019 at 03:58, André Draszik <git@andred.net> wrote: > > This driver does a funny dance disabling and re-enabling > > RX and/or TX delays. In any of the RGMII-ID modes, it first > > disables the delays, just to re-enable them again right > > away. This looks like a needless exercise. > > > > Just enable the respective delays when in any of the > > relevant 'id' modes, and disable them otherwise. > > > > Also, remove comments which don't add anything that can't be > > seen by looking at the code. > > > > Signed-off-by: André Draszik <git@andred.net> > > CC: Andrew Lunn <andrew@lunn.ch> > > CC: Florian Fainelli <f.fainelli@gmail.com> > > CC: Heiner Kallweit <hkallweit1@gmail.com> > > CC: "David S. Miller" <davem@davemloft.net> > > CC: netdev@vger.kernel.org > > --- > > Is there any particular problem you're facing? Does this make any difference? This is a clean-up, reducing the number of lines and if statements by removing unnecessary code paths and comments. Cheers, Andre' > > > drivers/net/phy/at803x.c | 26 ++++++-------------------- > > 1 file changed, 6 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > index 222ccd9ecfce..2ab51f552e92 100644 > > --- a/drivers/net/phy/at803x.c > > +++ b/drivers/net/phy/at803x.c > > @@ -257,35 +257,21 @@ static int at803x_config_init(struct phy_device *phydev) > > * after HW reset: RX delay enabled and TX delay disabled > > * after SW reset: RX delay enabled, while TX delay retains the > > * value before reset. > > - * > > - * So let's first disable the RX and TX delays in PHY and enable > > - * them based on the mode selected (this also takes care of RGMII > > - * mode where we expect delays to be disabled) > > */ > > - > > - ret = at803x_disable_rx_delay(phydev); > > - if (ret < 0) > > - return ret; > > - ret = at803x_disable_tx_delay(phydev); > > - if (ret < 0) > > - return ret; > > - > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > > - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > > - * otherwise keep it disabled > > - */ > > ret = at803x_enable_rx_delay(phydev); > > - if (ret < 0) > > - return ret; > > + } else { > > + ret = at803x_disable_rx_delay(phydev); > > } > > + if (ret < 0) > > + return ret; > > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > > - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > > - * otherwise keep it disabled > > - */ > > ret = at803x_enable_tx_delay(phydev); > > + } else { > > + ret = at803x_disable_tx_delay(phydev); > > } > > > > return ret; > > -- > > 2.20.1 > > > > Regards, > -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 10:00 ` André Draszik @ 2019-08-09 11:09 ` Vladimir Oltean 2019-08-09 11:15 ` André Draszik 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2019-08-09 11:09 UTC (permalink / raw) To: André Draszik Cc: lkml, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev Hi Andre, On Fri, 9 Aug 2019 at 13:00, André Draszik <git@andred.net> wrote: > > Hi Vladimir, > > On Fri, 2019-08-09 at 12:43 +0300, Vladimir Oltean wrote: > > Hi Andre, > > > > On Fri, 9 Aug 2019 at 03:58, André Draszik <git@andred.net> wrote: > > > This driver does a funny dance disabling and re-enabling > > > RX and/or TX delays. In any of the RGMII-ID modes, it first > > > disables the delays, just to re-enable them again right > > > away. This looks like a needless exercise. > > > > > > Just enable the respective delays when in any of the > > > relevant 'id' modes, and disable them otherwise. > > > > > > Also, remove comments which don't add anything that can't be > > > seen by looking at the code. > > > > > > Signed-off-by: André Draszik <git@andred.net> > > > CC: Andrew Lunn <andrew@lunn.ch> > > > CC: Florian Fainelli <f.fainelli@gmail.com> > > > CC: Heiner Kallweit <hkallweit1@gmail.com> > > > CC: "David S. Miller" <davem@davemloft.net> > > > CC: netdev@vger.kernel.org > > > --- > > > > Is there any particular problem you're facing? Does this make any difference? > > This is a clean-up, reducing the number of lines and if statements > by removing unnecessary code paths and comments. > Ok. Did checkpatch not complain about the braces which you left open around a single line? > > Cheers, > Andre' > > > > > > > drivers/net/phy/at803x.c | 26 ++++++-------------------- > > > 1 file changed, 6 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > > index 222ccd9ecfce..2ab51f552e92 100644 > > > --- a/drivers/net/phy/at803x.c > > > +++ b/drivers/net/phy/at803x.c > > > @@ -257,35 +257,21 @@ static int at803x_config_init(struct phy_device *phydev) > > > * after HW reset: RX delay enabled and TX delay disabled > > > * after SW reset: RX delay enabled, while TX delay retains the > > > * value before reset. > > > - * > > > - * So let's first disable the RX and TX delays in PHY and enable > > > - * them based on the mode selected (this also takes care of RGMII > > > - * mode where we expect delays to be disabled) > > > */ > > > - > > > - ret = at803x_disable_rx_delay(phydev); > > > - if (ret < 0) > > > - return ret; > > > - ret = at803x_disable_tx_delay(phydev); > > > - if (ret < 0) > > > - return ret; > > > - > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > > phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > > > - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > > > - * otherwise keep it disabled > > > - */ > > > ret = at803x_enable_rx_delay(phydev); > > > - if (ret < 0) > > > - return ret; > > > + } else { > > > + ret = at803x_disable_rx_delay(phydev); > > > } > > > + if (ret < 0) > > > + return ret; > > > > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > > phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > > > - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > > > - * otherwise keep it disabled > > > - */ > > > ret = at803x_enable_tx_delay(phydev); > > > + } else { > > > + ret = at803x_disable_tx_delay(phydev); > > > } > > > > > > return ret; > > > -- > > > 2.20.1 > > > > > > > Regards, > > -Vladimir > Thanks, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 11:09 ` Vladimir Oltean @ 2019-08-09 11:15 ` André Draszik 0 siblings, 0 replies; 7+ messages in thread From: André Draszik @ 2019-08-09 11:15 UTC (permalink / raw) To: Vladimir Oltean Cc: lkml, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev Hi, On Fri, 2019-08-09 at 14:09 +0300, Vladimir Oltean wrote: > Hi Andre, > > On Fri, 9 Aug 2019 at 13:00, André Draszik <git@andred.net> wrote: > > Hi Vladimir, > > > > On Fri, 2019-08-09 at 12:43 +0300, Vladimir Oltean wrote: > > > Hi Andre, > > > > > > On Fri, 9 Aug 2019 at 03:58, André Draszik <git@andred.net> wrote: > > > > This driver does a funny dance disabling and re-enabling > > > > RX and/or TX delays. In any of the RGMII-ID modes, it first > > > > disables the delays, just to re-enable them again right > > > > away. This looks like a needless exercise. > > > > > > > > Just enable the respective delays when in any of the > > > > relevant 'id' modes, and disable them otherwise. > > > > > > > > Also, remove comments which don't add anything that can't be > > > > seen by looking at the code. > > > > > > > > Signed-off-by: André Draszik <git@andred.net> > > > > CC: Andrew Lunn <andrew@lunn.ch> > > > > CC: Florian Fainelli <f.fainelli@gmail.com> > > > > CC: Heiner Kallweit <hkallweit1@gmail.com> > > > > CC: "David S. Miller" <davem@davemloft.net> > > > > CC: netdev@vger.kernel.org > > > > --- > > > > > > Is there any particular problem you're facing? Does this make any difference? > > > > This is a clean-up, reducing the number of lines and if statements > > by removing unnecessary code paths and comments. > > > > Ok. Did checkpatch not complain about the braces which you left open > around a single line? It actually doesn't... Should I send a v2? Cheers, Andre' > > > Cheers, > > Andre' > > > > > > > > drivers/net/phy/at803x.c | 26 ++++++-------------------- > > > > 1 file changed, 6 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > > > index 222ccd9ecfce..2ab51f552e92 100644 > > > > --- a/drivers/net/phy/at803x.c > > > > +++ b/drivers/net/phy/at803x.c > > > > @@ -257,35 +257,21 @@ static int at803x_config_init(struct phy_device *phydev) > > > > * after HW reset: RX delay enabled and TX delay disabled > > > > * after SW reset: RX delay enabled, while TX delay retains the > > > > * value before reset. > > > > - * > > > > - * So let's first disable the RX and TX delays in PHY and enable > > > > - * them based on the mode selected (this also takes care of RGMII > > > > - * mode where we expect delays to be disabled) > > > > */ > > > > - > > > > - ret = at803x_disable_rx_delay(phydev); > > > > - if (ret < 0) > > > > - return ret; > > > > - ret = at803x_disable_tx_delay(phydev); > > > > - if (ret < 0) > > > > - return ret; > > > > - > > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > > > phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > > > > - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > > > > - * otherwise keep it disabled > > > > - */ > > > > ret = at803x_enable_rx_delay(phydev); > > > > - if (ret < 0) > > > > - return ret; > > > > + } else { > > > > + ret = at803x_disable_rx_delay(phydev); > > > > } > > > > + if (ret < 0) > > > > + return ret; > > > > > > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > > > phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > > > > - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > > > > - * otherwise keep it disabled > > > > - */ > > > > ret = at803x_enable_tx_delay(phydev); > > > > + } else { > > > > + ret = at803x_disable_tx_delay(phydev); > > > > } > > > > > > > > return ret; > > > > -- > > > > 2.20.1 > > > > > > > > > > Regards, > > > -Vladimir > > Thanks, > -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 0:57 [PATCH] net: phy: at803x: stop switching phy delay config needlessly André Draszik 2019-08-09 9:43 ` Vladimir Oltean @ 2019-08-09 11:20 ` André Draszik 2019-08-12 21:02 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: André Draszik @ 2019-08-09 11:20 UTC (permalink / raw) To: linux-kernel Cc: André Draszik, Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev This driver does a funny dance disabling and re-enabling RX and/or TX delays. In any of the RGMII-ID modes, it first disables the delays, just to re-enable them again right away. This looks like a needless exercise. Just enable the respective delays when in any of the relevant 'id' modes, and disable them otherwise. Also, remove comments which don't add anything that can't be seen by looking at the code. Signed-off-by: André Draszik <git@andred.net> CC: Andrew Lunn <andrew@lunn.ch> CC: Florian Fainelli <f.fainelli@gmail.com> CC: Heiner Kallweit <hkallweit1@gmail.com> CC: "David S. Miller" <davem@davemloft.net> CC: netdev@vger.kernel.org --- v2: also remove braces around single lines --- drivers/net/phy/at803x.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 222ccd9ecfce..6ad8b1c63c34 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -257,36 +257,20 @@ static int at803x_config_init(struct phy_device *phydev) * after HW reset: RX delay enabled and TX delay disabled * after SW reset: RX delay enabled, while TX delay retains the * value before reset. - * - * So let's first disable the RX and TX delays in PHY and enable - * them based on the mode selected (this also takes care of RGMII - * mode where we expect delays to be disabled) */ - - ret = at803x_disable_rx_delay(phydev); - if (ret < 0) - return ret; - ret = at803x_disable_tx_delay(phydev); - if (ret < 0) - return ret; - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { - /* If RGMII_ID or RGMII_RXID are specified enable RX delay, - * otherwise keep it disabled - */ + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) ret = at803x_enable_rx_delay(phydev); - if (ret < 0) - return ret; - } + else + ret = at803x_disable_rx_delay(phydev); + if (ret < 0) + return ret; if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { - /* If RGMII_ID or RGMII_TXID are specified enable TX delay, - * otherwise keep it disabled - */ + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) ret = at803x_enable_tx_delay(phydev); - } + else + ret = at803x_disable_tx_delay(phydev); return ret; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] net: phy: at803x: stop switching phy delay config needlessly 2019-08-09 11:20 ` [PATCH v2] " André Draszik @ 2019-08-12 21:02 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2019-08-12 21:02 UTC (permalink / raw) To: git; +Cc: linux-kernel, andrew, f.fainelli, hkallweit1, netdev From: André Draszik <git@andred.net> Date: Fri, 9 Aug 2019 12:20:25 +0100 > This driver does a funny dance disabling and re-enabling > RX and/or TX delays. In any of the RGMII-ID modes, it first > disables the delays, just to re-enable them again right > away. This looks like a needless exercise. > > Just enable the respective delays when in any of the > relevant 'id' modes, and disable them otherwise. > > Also, remove comments which don't add anything that can't be > seen by looking at the code. > > Signed-off-by: André Draszik <git@andred.net> Applied. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-12 21:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-09 0:57 [PATCH] net: phy: at803x: stop switching phy delay config needlessly André Draszik 2019-08-09 9:43 ` Vladimir Oltean 2019-08-09 10:00 ` André Draszik 2019-08-09 11:09 ` Vladimir Oltean 2019-08-09 11:15 ` André Draszik 2019-08-09 11:20 ` [PATCH v2] " André Draszik 2019-08-12 21:02 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).