From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs Date: Wed, 25 Apr 2018 07:21:31 -0700 Message-ID: References: <20180425184944.24939-1-Nisar.Sayed@microchip.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: UNGLinuxDriver@microchip.com, netdev@vger.kernel.org To: Nisar Sayed , davem@davemloft.net Return-path: Received: from mail-ot0-f194.google.com ([74.125.82.194]:45348 "EHLO mail-ot0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754612AbeDYOVg (ORCPT ); Wed, 25 Apr 2018 10:21:36 -0400 Received: by mail-ot0-f194.google.com with SMTP id w4-v6so25489275ote.12 for ; Wed, 25 Apr 2018 07:21:36 -0700 (PDT) In-Reply-To: <20180425184944.24939-1-Nisar.Sayed@microchip.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 04/25/2018 11:49 AM, Nisar Sayed wrote: > Add driver for Microchip LAN87XX T1 PHYs > > This patch support driver for Microchp T1 PHYs. > There will be followup patches to this driver to support T1 PHY > features such as cable diagnostics, signal quality indicator(SQI), > sleep and wakeup (TC10) support. > > Signed-off-by: Nisar Sayed > --- > drivers/net/phy/Kconfig | 5 ++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/microchipT1phy.c | 116 +++++++++++++++++++++++++++++++++++++++ > include/linux/microchipT1phy.h | 28 ++++++++++ > 4 files changed, 150 insertions(+) > create mode 100644 drivers/net/phy/microchipT1phy.c > create mode 100644 include/linux/microchipT1phy.h > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index bdfbabb..7b0b351 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -354,6 +354,11 @@ config MICROCHIP_PHY > help > Supports the LAN88XX PHYs. > > +config MICROCHIP_T1_PHY > + tristate "Microchip T1 PHYs" > + ---help--- > + Supports the LAN87XX PHYs. > + > config MICROSEMI_PHY > tristate "Microsemi PHYs" > ---help--- > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 01acbcb..5706613 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o > obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o > obj-$(CONFIG_MICREL_PHY) += micrel.o > obj-$(CONFIG_MICROCHIP_PHY) += microchip.o > +obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o > obj-$(CONFIG_MICROSEMI_PHY) += mscc.o > obj-$(CONFIG_NATIONAL_PHY) += national.o > obj-$(CONFIG_QSEMI_PHY) += qsemi.o > diff --git a/drivers/net/phy/microchipT1phy.c b/drivers/net/phy/microchipT1phy.c > new file mode 100644 > index 0000000..15fbb04 > --- /dev/null > +++ b/drivers/net/phy/microchipT1phy.c Can we avoid CamelCase file names? Any reasons why this is not part of microchip.c? > @@ -0,0 +1,116 @@ > +/* > + * Copyright (C) 2018 Microchip Technology Please use a SPDX license tag. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see . > + */ > +#include > +#include > +#include > +#include > +#include > + > +#define DRIVER_AUTHOR "Nisar Sayed " > +#define DRIVER_DESC "Microchip LAN87XX T1 PHY driver" > + > +struct lan87xx_priv { > + int chip_id; > + int chip_rev; > +}; > + > +static int lan87xx_phy_config_intr(struct phy_device *phydev) > +{ > + int rc = 0; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + /* unmask all source and clear them before enable */ > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0x7FFF); > + rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE); > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, > + LAN87XX_MASK_LINK_UP | LAN87XX_MASK_LINK_DOWN); > + } else { > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0); > + } Since the last thing you do in both branches is write to LAN87XX_INTERRUPT_MASK, just move that write outside of the branches and make sure the value you will set is correct in both cases. > + > + return rc < 0 ? rc : 0; > +} > + > +static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) > +{ > + int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE); > + > + return rc < 0 ? rc : 0; > +} > + > +static int lan87xx_probe(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct lan87xx_priv *priv; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + /* these values can be used to identify internal PHY */ > + priv->chip_id = phy_read(phydev, MII_PHYSID1); > + priv->chip_rev = phy_read(phydev, MII_PHYSID2); This is absolutely not necessary, phydev->phy_id stores that information and you can use that at any point in your driver's lifetime. Besides, the revision is not the full MII_PHYSID2, it's only the lower 4 bits of it. Your probe() and remove() function then become unnecessary. > + > + phydev->priv = priv; > + > + return 0; > +} > + > +static void lan87xx_remove(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct lan87xx_priv *priv = phydev->priv; > + > + if (priv) > + devm_kfree(dev, priv); > +} > + > +static struct phy_driver microchip_T1_phy_driver[] = { > + { > + .phy_id = 0x0007c150, > + .phy_id_mask = 0xfffffff0, > + .name = "Microchip LAN87xx", > + > + .features = SUPPORTED_100baseT_Full, PHY_BASIC_FEATURES maybe? You cannot possibly be supporting just 100BaseT full duplex, 100BaseT half duplex, 10BaseT full and half duplex are also supported, right? > + .flags = PHY_HAS_INTERRUPT, > + > + .probe = lan87xx_probe, > + .remove = lan87xx_remove, > + > + .config_init = genphy_config_init, > + .config_aneg = genphy_config_aneg, > + > + .ack_interrupt = lan87xx_phy_ack_interrupt, > + .config_intr = lan87xx_phy_config_intr, > + > + .suspend = genphy_suspend, > + .resume = genphy_resume, > + } > +}; > + > +module_phy_driver(microchip_T1_phy_driver); > + > +static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = { > + { 0x0007c150, 0xfffffff0 }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl); > + > +MODULE_AUTHOR(DRIVER_AUTHOR); > +MODULE_DESCRIPTION(DRIVER_DESC); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/microchipT1phy.h b/include/linux/microchipT1phy.h > new file mode 100644 > index 0000000..7353e7c > --- /dev/null > +++ b/include/linux/microchipT1phy.h > @@ -0,0 +1,28 @@ > +/* > + * Copyright (C) 2018 Microchip Technology Same comment as before, please use SPDX license identifiers. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see . > + */ > +#ifndef _MICROCHIPT1PHY_H_ > +#define _MICROCHIPT1PHY_H_ > + > +/* Interrupt Source Register */ > +#define LAN87XX_INTERRUPT_SOURCE (0x18) > + > +/* Interrupt Mask Register */ > +#define LAN87XX_INTERRUPT_MASK (0x19) > +#define LAN87XX_MASK_LINK_UP (0x0004) > +#define LAN87XX_MASK_LINK_DOWN (0x0002) What's the point of that header file if all definitions are consumed by the same driver? > + > +#endif /* _MICROCHIPT1PHY_H_ */ > -- Florian