Re: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs

[Florian Fainelli <f.fainelli@gmail.com>]

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@microchip.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@microchip.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