netdev - Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f56aa414-3002-ef85-51a9-bb36017270e6@gmail.com>
Date:   Fri, 4 Jun 2021 09:24:25 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Xu Liang <lxu@...linear.com>, andrew@...n.ch, hkallweit1@...il.com,
        netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        vee.khee.wong@...ux.intel.com
Cc:     linux@...linux.org.uk, hmehrtens@...linear.com,
        tmohren@...linear.com
Subject: Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver



On 6/4/2021 9:12 AM, Xu Liang wrote:
> Add driver to support the Maxlinear GPY115, GPY211, GPY212, GPY215,
> GPY241, GPY245 PHYs.
> 
> Signed-off-by: Xu Liang <lxu@...linear.com>
> ---
> v2 changes:
>  Fix format warning from checkpath and some comments.
>  Use smaller PHY ID mask.
>  Split FWV register mask.
>  Call phy_trigger_machine if necessary when clear interrupt.
> v3 changes:
>  Replace unnecessary phy_modify_mmd_changed with phy_modify_mmd
>  Move firmware version print to probe.
> 
>  MAINTAINERS               |   6 +
>  drivers/net/phy/Kconfig   |   6 +
>  drivers/net/phy/Makefile  |   1 +
>  drivers/net/phy/mxl-gpy.c | 552 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 565 insertions(+)
>  create mode 100644 drivers/net/phy/mxl-gpy.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 948706174fae..140b19d038fb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11168,6 +11168,12 @@ W:	https://linuxtv.org
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/radio/radio-maxiradio*
>  
> +MAXLINEAR ETHERNET PHY DRIVER
> +M:	Xu Liang <lxu@...linear.com>
> +L:	netdev@...r.kernel.org
> +S:	Supported
> +F:	drivers/net/phy/mxl-gpy.c
> +
>  MCAN MMIO DEVICE DRIVER
>  M:	Chandrasekar Ramakrishnan <rcsekar@...sung.com>
>  L:	linux-can@...r.kernel.org
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 288bf405ebdb..d02098774d80 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -207,6 +207,12 @@ config MARVELL_88X2222_PHY
>  	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
>  	  Transceiver.
>  
> +config MXL_GPHY
> +	tristate "Maxlinear PHYs"
> +	help
> +	  Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> +	  GPY241, GPY245 PHYs.
> +
>  config MICREL_PHY
>  	tristate "Micrel PHYs"
>  	help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index bcda7ed2455d..1055fb73ac17 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -70,6 +70,7 @@ 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/
> +obj-$(CONFIG_MXL_GPHY)		+= mxl-gpy.o

Could you spell it out completely: CONFIG_MAXLINEAR_GPHY for consistency
with the other vendor's and also to have some proper namespacing in case
we see a non-Ethernet PHY being submitted for a Maxlinear product in the
future?

>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
>  obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
>  obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> new file mode 100644
> index 000000000000..3e3d3096e858
> --- /dev/null
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -0,0 +1,552 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2021 Maxlinear Corporation
> + * Copyright (C) 2020 Intel Corporation
> + *
> + * Drivers for Maxlinear Ethernet GPY
> + *
> + */
> +
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/bitfield.h>
> +#include <linux/phy.h>
> +#include <linux/netdevice.h>
> +
> +/* PHY ID */
> +#define PHY_ID_GPY		0x67C9DC00
> +#define PHY_ID_MASK		GENMASK(31, 4)

Consider initializing your phy_driver with PHY_ID_MATCH_MODEL() which
would take care of populating the mask accordingly.

[snip]

> +
> +static int gpy_read_abilities(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Detect 2.5G/5G support. */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
> +	if (ret < 0)
> +		return ret;
> +	if (!(ret & MDIO_PMA_STAT2_EXTABLE))
> +		return 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
> +	if (ret < 0)
> +		return ret;
> +	if (!(ret & MDIO_PMA_EXTABLE_NBT))
> +		return 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported,
> +			 ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +			 phydev->supported,
> +			 ret & MDIO_PMA_NG_EXTABLE_5GBT);

This does not access vendor specific registers, should not this be part
of the standard genphy_read_abilities() or moved to a helper?

> +
> +	return 0;
> +}
> +
> +static int gpy_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Mask all interrupts */
> +	ret = phy_write(phydev, PHY_IMASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear all pending interrupts */
> +	ret = phy_read(phydev, PHY_ISTAT);
> +	return ret < 0 ? ret : 0;

You can certainly simplify this to:

	return phy_read(phydev, PHY_ISTAT);

[snip]

> +
> +MODULE_DESCRIPTION("Maxlinear Ethernet GPY Driver");
> +MODULE_AUTHOR("Maxlinear Corporation");

The author is not usually a company but an individual working on behalf
of that company.
-- 
Florian