[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <457179162ca6fd067b22b3b7733c60c2a17129a5.camel@microchip.com>
Date: Fri, 5 Jul 2024 15:15:36 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <andrew@...n.ch>, <davem@...emloft.net>, <hkallweit1@...il.com>,
<Yuiko.Oshino@...rochip.com>, <linux@...linux.org.uk>,
<Woojung.Huh@...rochip.com>, <pabeni@...hat.com>, <o.rempel@...gutronix.de>,
<edumazet@...gle.com>, <f.fainelli@...il.com>, <kuba@...nel.org>
CC: <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
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
> +
> +#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"
>
> @@ -373,6 +379,66 @@ static void lan88xx_link_change_notify(struct
> phy_device *phydev)
> }
> }
>
Adding function description will be good.
> +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) ?
> + 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. Also IMO, this two function can be combined together if
lan937x_tx_config_mdix is not used by other functions.
> +}
> +
> static struct phy_driver microchip_phy_driver[] = {
> {
> .phy_id = 0x0007c132,
> @@ -400,12 +466,21 @@ static struct phy_driver microchip_phy_driver[]
> = {
> .set_wol = lan88xx_set_wol,
> .read_page = lan88xx_read_page,
> .write_page = lan88xx_write_page,
> +},
> +{
> + 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?
> } };
>
> module_phy_driver(microchip_phy_driver);
>
> static struct mdio_device_id __maybe_unused microchip_tbl[] = {
> { 0x0007c132, 0xfffffff2 },
> + { PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX) },
> { }
> };
>
> --
> 2.39.2
>
Powered by blists - more mailing lists