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

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

On 04/27/2018 08:10 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>
> ---
> v0 - v1:
>         * Rename microchipT1phy.c file to microchip_t1.c
>         * Remove microchipT1phy.h include file
>         * Add SPDX license identifier
>         * Remove remove probe and remove functions
>         * Update LAN87XX_INTERRUPT_MASK write as suggested

This does look better, small comments below.

> ---
>  drivers/net/phy/Kconfig        |  5 +++
>  drivers/net/phy/Makefile       |  1 +
>  drivers/net/phy/microchip_t1.c | 88 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)
>  create mode 100644 drivers/net/phy/microchip_t1.c
> 
> 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..3d0550b 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)	+= microchip_t1.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/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> new file mode 100644
> index 0000000..1f6f299
> --- /dev/null
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

This is not the standard comment for a .c file, it should be: // (C++ style)

> +/*
> + * Copyright (C) 2018 Microchip Technology
> + *
> + * 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/>.

That needs to go away now that you used SPDX

> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.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)
> +
> +#define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@microchip.com>"
> +#define DRIVER_DESC	"Microchip LAN87XX T1 PHY driver"
> +
> +static int lan87xx_phy_config_intr(struct phy_device *phydev)
> +{
> +	int rc, val = 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);
> +		val = (LAN87XX_MASK_LINK_UP | LAN87XX_MASK_LINK_DOWN);

The parenthesis are not necessary here.

> +	}
> +
> +	rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
> +
> +	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 struct phy_driver microchip_t1_phy_driver[] = {
> +	{
> +		.phy_id         = 0x0007c150,
> +		.phy_id_mask    = 0xfffffff0,
> +		.name           = "Microchip LAN87xx",

Would you want to name this "Microchip LAN87xx T1"?
-- 
Florian