* [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status
@ 2023-03-08 20:11 Heiner Kallweit
2023-03-09 14:31 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2023-03-08 20:11 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet
Cc: netdev@vger.kernel.org
Simplify the code by using phy_clear/sert_bits().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/smsc.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index af89f3ef1..5965a8afa 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
static int lan87xx_read_status(struct phy_device *phydev)
{
struct smsc_phy_priv *priv = phydev->priv;
+ int rc;
- int err = genphy_read_status(phydev);
+ rc = genphy_read_status(phydev);
+ if (rc)
+ return rc;
if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
/* Disable EDPD to wake up PHY */
- int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
- if (rc < 0)
- return rc;
-
- rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
- rc & ~MII_LAN83C185_EDPWRDOWN);
+ rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
+ MII_LAN83C185_EDPWRDOWN);
if (rc < 0)
return rc;
@@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev)
* an actual error.
*/
read_poll_timeout(phy_read, rc,
- rc & MII_LAN83C185_ENERGYON || rc < 0,
+ rc < 0 || rc & MII_LAN83C185_ENERGYON,
10000, 640000, true, phydev,
MII_LAN83C185_CTRL_STATUS);
if (rc < 0)
return rc;
/* Re-enable EDPD */
- rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
- if (rc < 0)
- return rc;
-
- rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
- rc | MII_LAN83C185_EDPWRDOWN);
+ rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
+ MII_LAN83C185_EDPWRDOWN);
if (rc < 0)
return rc;
}
- return err;
+ return 0;
}
static int smsc_get_sset_count(struct phy_device *phydev)
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status
2023-03-08 20:11 [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status Heiner Kallweit
@ 2023-03-09 14:31 ` Simon Horman
2023-03-10 20:51 ` Heiner Kallweit
0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2023-03-09 14:31 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Russell King - ARM Linux, Andrew Lunn, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger.kernel.org
On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote:
> Simplify the code by using phy_clear/sert_bits().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/smsc.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
The phy_clear/sert_bits changes lookg good.
But I have a few nit-pick comments.
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index af89f3ef1..5965a8afa 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
> static int lan87xx_read_status(struct phy_device *phydev)
> {
> struct smsc_phy_priv *priv = phydev->priv;
> + int rc;
>
> - int err = genphy_read_status(phydev);
> + rc = genphy_read_status(phydev);
> + if (rc)
> + return rc;
nit: this seems like a separate change, possibly a fix.
>
> if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
> /* Disable EDPD to wake up PHY */
> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> - if (rc < 0)
> - return rc;
> -
> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> - rc & ~MII_LAN83C185_EDPWRDOWN);
> + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> + MII_LAN83C185_EDPWRDOWN);
> if (rc < 0)
> return rc;
>
> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev)
> * an actual error.
> */
> read_poll_timeout(phy_read, rc,
> - rc & MII_LAN83C185_ENERGYON || rc < 0,
> + rc < 0 || rc & MII_LAN83C185_ENERGYON,
nit: this also seems like a separate change.
> 10000, 640000, true, phydev,
> MII_LAN83C185_CTRL_STATUS);
> if (rc < 0)
> return rc;
>
> /* Re-enable EDPD */
> - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> - if (rc < 0)
> - return rc;
> -
> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> - rc | MII_LAN83C185_EDPWRDOWN);
> + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> + MII_LAN83C185_EDPWRDOWN);
> if (rc < 0)
> return rc;
> }
>
> - return err;
> + return 0;
> }
>
> static int smsc_get_sset_count(struct phy_device *phydev)
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status
2023-03-09 14:31 ` Simon Horman
@ 2023-03-10 20:51 ` Heiner Kallweit
2023-03-13 12:00 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2023-03-10 20:51 UTC (permalink / raw)
To: Simon Horman
Cc: Russell King - ARM Linux, Andrew Lunn, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger.kernel.org
On 09.03.2023 15:31, Simon Horman wrote:
> On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote:
>> Simplify the code by using phy_clear/sert_bits().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/smsc.c | 25 ++++++++++---------------
>> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> The phy_clear/sert_bits changes lookg good.
> But I have a few nit-pick comments.
>
>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> index af89f3ef1..5965a8afa 100644
>> --- a/drivers/net/phy/smsc.c
>> +++ b/drivers/net/phy/smsc.c
>> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
>> static int lan87xx_read_status(struct phy_device *phydev)
>> {
>> struct smsc_phy_priv *priv = phydev->priv;
>> + int rc;
>>
>> - int err = genphy_read_status(phydev);
>> + rc = genphy_read_status(phydev);
>> + if (rc)
>> + return rc;
>
> nit: this seems like a separate change, possibly a fix.
>
There's no known problem with the current code, so the need for a fix
may be questionable. But you're right, it's a separate change.
IMO it just wasn't worth it to provide it as a separate patch.
>>
>> if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
>> /* Disable EDPD to wake up PHY */
>> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>> - if (rc < 0)
>> - return rc;
>> -
>> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
>> - rc & ~MII_LAN83C185_EDPWRDOWN);
>> + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
>> + MII_LAN83C185_EDPWRDOWN);
>> if (rc < 0)
>> return rc;
>>
>> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev)
>> * an actual error.
>> */
>> read_poll_timeout(phy_read, rc,
>> - rc & MII_LAN83C185_ENERGYON || rc < 0,
>> + rc < 0 || rc & MII_LAN83C185_ENERGYON,
>
> nit: this also seems like a separate change.
>
Same as for the remark before.
>> 10000, 640000, true, phydev,
>> MII_LAN83C185_CTRL_STATUS);
>> if (rc < 0)
>> return rc;
>>
>> /* Re-enable EDPD */
>> - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>> - if (rc < 0)
>> - return rc;
>> -
>> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
>> - rc | MII_LAN83C185_EDPWRDOWN);
>> + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
>> + MII_LAN83C185_EDPWRDOWN);
>> if (rc < 0)
>> return rc;
>> }
>>
>> - return err;
>> + return 0;
>> }
>>
>> static int smsc_get_sset_count(struct phy_device *phydev)
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status
2023-03-10 20:51 ` Heiner Kallweit
@ 2023-03-13 12:00 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2023-03-13 12:00 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Russell King - ARM Linux, Andrew Lunn, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger.kernel.org
On Fri, Mar 10, 2023 at 09:51:37PM +0100, Heiner Kallweit wrote:
> On 09.03.2023 15:31, Simon Horman wrote:
> > On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote:
> >> Simplify the code by using phy_clear/sert_bits().
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >> drivers/net/phy/smsc.c | 25 ++++++++++---------------
> >> 1 file changed, 10 insertions(+), 15 deletions(-)
> >
> > The phy_clear/sert_bits changes lookg good.
> > But I have a few nit-pick comments.
> >
> >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> >> index af89f3ef1..5965a8afa 100644
> >> --- a/drivers/net/phy/smsc.c
> >> +++ b/drivers/net/phy/smsc.c
> >> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
> >> static int lan87xx_read_status(struct phy_device *phydev)
> >> {
> >> struct smsc_phy_priv *priv = phydev->priv;
> >> + int rc;
> >>
> >> - int err = genphy_read_status(phydev);
> >> + rc = genphy_read_status(phydev);
> >> + if (rc)
> >> + return rc;
> >
> > nit: this seems like a separate change, possibly a fix.
> >
> There's no known problem with the current code, so the need for a fix
> may be questionable. But you're right, it's a separate change.
> IMO it just wasn't worth it to provide it as a separate patch.
Ok, I don't feel strongly about this.
Though perhaps it would be nice to mention in the patch
description if a v2 materialises.
In any case, I am happy with the correctness of this patch.
Signed-off-by: Simon Horman <simon.horman@corigine.com>
> >>
> >> if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
> >> /* Disable EDPD to wake up PHY */
> >> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> >> - if (rc < 0)
> >> - return rc;
> >> -
> >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> >> - rc & ~MII_LAN83C185_EDPWRDOWN);
> >> + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> >> + MII_LAN83C185_EDPWRDOWN);
> >> if (rc < 0)
> >> return rc;
> >>
> >> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev)
> >> * an actual error.
> >> */
> >> read_poll_timeout(phy_read, rc,
> >> - rc & MII_LAN83C185_ENERGYON || rc < 0,
> >> + rc < 0 || rc & MII_LAN83C185_ENERGYON,
> >
> > nit: this also seems like a separate change.
> >
> Same as for the remark before.
Ack.
> >> 10000, 640000, true, phydev,
> >> MII_LAN83C185_CTRL_STATUS);
> >> if (rc < 0)
> >> return rc;
> >>
> >> /* Re-enable EDPD */
> >> - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> >> - if (rc < 0)
> >> - return rc;
> >> -
> >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> >> - rc | MII_LAN83C185_EDPWRDOWN);
> >> + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> >> + MII_LAN83C185_EDPWRDOWN);
> >> if (rc < 0)
> >> return rc;
> >> }
> >>
> >> - return err;
> >> + return 0;
> >> }
> >>
> >> static int smsc_get_sset_count(struct phy_device *phydev)
> >> --
> >> 2.39.2
> >>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-13 12:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 20:11 [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status Heiner Kallweit
2023-03-09 14:31 ` Simon Horman
2023-03-10 20:51 ` Heiner Kallweit
2023-03-13 12:00 ` Simon Horman
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).