linux-kernel - Re: [PATCH net-next v2 2/2] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

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] [day] [month] [year] [list]

Message-ID: <YnJ2Fvux5ZE9zqyT@lunn.ch>
Date:   Wed, 4 May 2022 14:48:22 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, kernel@...gutronix.de,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: phy: dp83td510: Add support for the
 DP83TD510 Ethernet PHY

On Wed, May 04, 2022 at 01:06:55PM +0200, Oleksij Rempel wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
> 
> This driver was tested with NXP SJA1105, STMMAC and ASIX AX88772B USB Ethernet
> controller.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>

Thanks for the re-write. This looks a lot better.

> +		ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
> +		if (ret < 0)
> +			return ret;
> +
> +		cfg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
> +		if (cfg < 0)
> +			return cfg;
> +
> +		if (ret & MDIO_AN_T1_ADV_L_FORCE_MS) {
> +			if (cfg & MDIO_AN_T1_ADV_M_MST)
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_MASTER_FORCE;
> +			else
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_SLAVE_FORCE;
> +		} else {
> +			if (cfg & MDIO_AN_T1_ADV_M_MST)
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_MASTER_PREFERRED;
> +			else
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> +		}

Is this identical to genphy_c45_baset1_read_status()? I don't think
there are any odd vendor registers here. So i suggest exporting it and
making use of it.

> +static int dp83td510_config_master_slave(struct phy_device *phydev)
> +{
> +	int ctl = 0;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		ctl = MDIO_PMA_PMD_BT1_CTRL_CFG_MST;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
> +			     MDIO_PMA_PMD_BT1_CTRL_CFG_MST, ctl);

Is this identical to what is in genphy_c45_pma_setup_forced()? If so,
i would pull that code out into a helper and make use of it.


Thanks
	Andrew