Re: [PATCH 3/4] net: phy: Add Qualcomm QCA807x driver

[Andrew Lunn <andrew@lunn.ch>]

> +++ b/drivers/net/phy/Kconfig
> @@ -264,6 +264,16 @@ config QSEMI_PHY
>  	help
>  	  Currently supports the qs6612
>  
> +config QCA807X_PHY
> +	tristate "Qualcomm QCA807X PHYs"

Kconfig is sorted based on the tristate string. So this should be
after AT803X_PHY.

> +	depends on OF_MDIO
> +	help
> +	  Adds support for the Qualcomm QCA807x PHYs.
> +	  These are 802.3 Clause 22 compliant PHYs supporting gigabit
> +	  ethernet as well as 100Base-FX and 1000Base-X fibre.
> +
> +	  Currently supports the QCA8072 and QCA8075 models.
> +
>  config REALTEK_PHY
>  	tristate "Realtek PHYs"
>  	help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index ca0a313423b9..5f463ce0f711 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
>  obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
>  obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
> +obj-$(CONFIG_QCA807X_PHY)		+= qca807x.o
>  obj-$(CONFIG_REALTEK_PHY)	+= realtek.o

One line earlier please.

> +static int qca807x_cable_test_report_trans(int result)
> +{
> +	switch (result) {
> +	case QCA807X_CDT_RESULTS_OK:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> +	case QCA807X_CDT_RESULTS_OPEN:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> +	case QCA807X_CDT_RESULTS_SAME_SHORT:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OK:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OK:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
> +	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_SHORT:

Feel free to add an optional pair to the netlink message, indicating
which pair this pair is shorted to.

> +		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
> +	default:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> +	}
> +}

> +static int qca807x_cable_test_start(struct phy_device *phydev)
> +{
> +	int val, ret;
> +
> +	val = phy_read(phydev, QCA807X_CDT);
> +	/* Enable inter-pair short check as well */
> +	val &= ~QCA807X_CDT_ENABLE_INTER_PAIR_SHORT;
> +	val |= QCA807X_CDT_ENABLE;
> +	ret = phy_write(phydev, QCA807X_CDT, val);

What happens when you are using the PHY as a media converted to Fibre?
Should we return -EOPNOTSUPP here? I'm assuming it cannot do cable
test on a fibre pair.

> +static int qca807x_read_fiber_status(struct phy_device *phydev, bool combo_port)
> +{
> +	int ss, err, page, lpa, old_link = phydev->link;
> +
> +	/* Check whether fiber page is set and set if needed */
> +	page = phy_read(phydev, QCA807X_CHIP_CONFIGURATION);
> +	if (page & QCA807X_BT_BX_REG_SEL) {
> +		page &= ~QCA807X_BT_BX_REG_SEL;
> +		phy_write(phydev, QCA807X_CHIP_CONFIGURATION, page);
> +	}
> +
> +	/* Update the link, but return if there was an error */
> +	err = genphy_update_link(phydev);
> +	if (err)
> +		return err;
> +
> +	/* why bother the PHY if nothing can have changed */
> +	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
> +		return 0;
> +
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		lpa = phy_read(phydev, MII_LPA);
> +		if (lpa < 0)
> +			return lpa;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				 phydev->lp_advertising, lpa & LPA_LPACK);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +				 phydev->lp_advertising, lpa & LPA_1000XFULL);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 phydev->lp_advertising, lpa & LPA_1000XPAUSE);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 phydev->lp_advertising,
> +				 lpa & LPA_1000XPAUSE_ASYM);
> +
> +		phy_resolve_aneg_linkmode(phydev);
> +	}

This looks a lot like genphy_c37_read_status(). You could call it, and
then over write the speed and duplex with values from the PHY specific
registers.

> +static int qca807x_config_intr(struct phy_device *phydev)
> +{
> +	int ret, val;
> +
> +	val = phy_read(phydev, QCA807X_INTR_ENABLE);
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		/* Check for combo port as it has fewer interrupts */
> +		if (phy_read(phydev, QCA807X_CHIP_CONFIGURATION)) {
> +			val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
> +			val |= QCA807X_INTR_ENABLE_LINK_FAIL;
> +			val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
> +		} else {
> +			val |= QCA807X_INTR_ENABLE_AUTONEG_ERR;
> +			val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
> +			val |= QCA807X_INTR_ENABLE_DUPLEX_CHANGED;
> +			val |= QCA807X_INTR_ENABLE_LINK_FAIL;
> +			val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
> +		}
> +		ret = phy_write(phydev, QCA807X_INTR_ENABLE, val);
> +	} else {
> +		ret = phy_write(phydev, QCA807X_INTR_ENABLE, 0);
> +	}
> +
> +	return ret;
> +}
> +
> +static int qca807x_ack_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, QCA807X_INTR_STATUS);
> +
> +	return (ret < 0) ? ret : 0;
> +}

You need to rework this to follow what Ioana has done for interrupt
handling.

> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_QCA8075),
> +		.name           = "Qualcomm QCA8075",
> +		.flags		= PHY_POLL_CABLE_TEST,
> +		/* PHY_GBIT_FEATURES */
> +		.probe		= qca807x_probe,
> +		.config_init	= qca807x_config,
> +		.read_status	= qca807x_read_status,
> +		.config_intr	= qca807x_config_intr,
> +		.ack_interrupt	= qca807x_ack_intr,
> +		.soft_reset	= genphy_soft_reset,
> +		.get_tunable	= qca807x_get_tunable,
> +		.set_tunable	= qca807x_set_tunable,
> +		.cable_test_start	= qca807x_cable_test_start,
> +		.cable_test_get_status	= qca807x_cable_test_get_status,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_QCA807X_PSGMII),
> +		.name           = "Qualcomm QCA807x PSGMII",
> +		.probe		= qca807x_psgmii_config,

This looks odd. 

     Andrew