| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Apr 2018 07:21:31 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Nisar Sayed <Nisar.Sayed@...rochip.com>, davem@...emloft.net
Cc: UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX
T1 PHYs
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 <Nisar.Sayed@...rochip.com>
> ---
> 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 <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +#include <linux/microchipT1phy.h>
> +
> +#define DRIVER_AUTHOR "Nisar Sayed <nisar.sayed@...rochip.com>"
> +#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 <http://www.gnu.org/licenses/>.
> + */
> +#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
Powered by blists - more mailing lists