[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZogWx59tzuH7t4PG@pengutronix.de>
Date: Fri, 5 Jul 2024 17:52:39 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Arun.Ramadoss@...rochip.com
Cc: andrew@...n.ch, davem@...emloft.net, hkallweit1@...il.com,
Yuiko.Oshino@...rochip.com, linux@...linux.org.uk,
Woojung.Huh@...rochip.com, pabeni@...hat.com, edumazet@...gle.com,
f.fainelli@...il.com, kuba@...nel.org, michal.kubiak@...el.com,
kernel@...gutronix.de, linux-kernel@...r.kernel.org,
florian.fainelli@...adcom.com, UNGLinuxDriver@...rochip.com,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/1] net: phy: microchip: lan937x: add
support for 100BaseTX PHY
On Fri, Jul 05, 2024 at 03:15:36PM +0000, Arun.Ramadoss@...rochip.com wrote:
> Hi Oleksij,
> On Fri, 2024-07-05 at 10:55 +0200, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >
> > Add support of 100BaseTX PHY build in to LAN9371 and LAN9372
> > switches.
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@...adcom.com>
> > Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>
> > ---
> > changes v2:
> > - move LAN937X_TX code from microchip_t1.c to microchip.c
> > - add Reviewed-by tags
> > ---
> > drivers/net/phy/microchip.c | 75
> > +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 75 insertions(+)
> >
> > diff --git a/drivers/net/phy/microchip.c
> > b/drivers/net/phy/microchip.c
> > index 0b88635f4fbca..b46d5d43e2585 100644
> > --- a/drivers/net/phy/microchip.c
> > +++ b/drivers/net/phy/microchip.c
> > @@ -12,6 +12,12 @@
> > #include <linux/of.h>
> > #include <dt-bindings/net/microchip-lan78xx.h>
> >
> > +#define PHY_ID_LAN937X_TX 0x0007c190
>
> 0x0007c190 -> 0x0007C190
Why?
I wrote a python script to gather stats in the drivers/net/phy:
Uppercase hex digits count:
E: 83
F: 216
C: 130
A: 148
B: 65
D: 74
Lowercase hex digits count:
b: 218
a: 337
d: 190
e: 238
f: 2560
c: 368
Sum of uppercase A-F: 716
Sum of lowercase a-f: 3911
> > +#define LAN937X_MODE_CTRL_STATUS_REG 0x11
> > +#define LAN937X_AUTOMDIX_EN BIT(7)
> > +#define LAN937X_MDI_MODE BIT(6)
> > +
> > #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@...rochip.com>"
> > #define DRIVER_DESC "Microchip LAN88XX PHY driver"
>
> nitpick:
> It can be updated to include "Microchip LAN88XX/LAN937X TX PHY driver"
ack
> > @@ -373,6 +379,66 @@ static void lan88xx_link_change_notify(struct
> > phy_device *phydev)
> > }
> > }
> >
>
> Adding function description will be good.
ack
> > +static int lan937x_tx_config_mdix(struct phy_device *phydev, u8
> > ctrl)
> > +{
> > + u16 val;
> > +
> > + switch (ctrl) {
> > + case ETH_TP_MDI:
> > + val = 0;
> > + break;
> > + case ETH_TP_MDI_X:
> > + val = LAN937X_MDI_MODE;
> > + break;
> > + case ETH_TP_MDI_AUTO:
> > + val = LAN937X_AUTOMDIX_EN;
> > + break;
> > + default:
> > + return 0;
> > + }
> > +
> > + return phy_modify(phydev, LAN937X_MODE_CTRL_STATUS_REG,
> > + LAN937X_AUTOMDIX_EN | LAN937X_MDI_MODE,
> > val);
> > +}
> > +
> > +static int lan937x_tx_config_aneg(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = genphy_config_aneg(phydev);
> > + if (ret)
>
> Is this if( ret < 0) ?
ack
> > + return ret;
> > +
> > + return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);
>
> why we need to pass argument phydev->mdix_ctrl, since already phydev is
> passed.
good point.
> Also IMO, this two function can be combined together if
> lan937x_tx_config_mdix is not used by other functions.
I disagree here.
> > +{
> > + PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX),
> > + .name = "Microchip LAN937x TX",
> > + .suspend = genphy_suspend,
> > + .resume = genphy_resume,
> > + .config_aneg = lan937x_tx_config_aneg,
> > + .read_status = lan937x_tx_read_status,
>
> Do we need to add genphy_suspend/resume, .features?
>From PHY driver perspective - yes, otherwise to suspend or resume will be
called.
>From internal PHY perspective - i do not know. Will the MAC disable PHY
automatically?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists