Re: [PATCH net-next v3 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Dan Murphy <dmurphy@ti.com>]

Andrew

On 10/30/20 3:15 PM, Andrew Lunn wrote:
>> +static int dp83td510_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int mst_slave_cfg;
>> +	int ret = 0;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		if (dp83td510->rgmii_delay) {
>> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
>> +					       DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
> Hi Dan
>
> I'm getting a bit paranoid about RGMII delays...
Not sure what this means.
>
>> +static int dp83td510_read_straps(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int strap;
>> +
>> +	strap = phy_read_mmd(phydev, DP83TD510_DEVADDR, DP83TD510_SOR_1);
>> +	if (strap < 0)
>> +		return strap;
>> +
>> +	if (strap & DP83TD510_RGMII)
>> +		dp83td510->is_rgmii = true;
>> +
>> +	return 0;
>> +};
> So dp83td510->is_rgmii is the strapping configuration. So if one of
> the four RGMII modes is selected, your appear to ignore which of the
> four is selected, and program the hardware with the strapping?
>
> That seems like a bad idea.
I will re-look at this code.
>
>> +#if IS_ENABLED(CONFIG_OF_MDIO)
>> +static int dp83td510_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
>> +	s32 rx_int_delay;
>> +	s32 tx_int_delay;
>> +	int ret;
>> +
>> +	if (!of_node)
>> +		return -ENODEV;
>> +
>> +	ret = dp83td510_read_straps(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dp83td510->hi_diff_output = device_property_read_bool(&phydev->mdio.dev,
>> +							      "tx-rx-output-high");
>> +
>> +	if (device_property_read_u32(&phydev->mdio.dev, "tx-fifo-depth",
>> +				     &dp83td510->tx_fifo_depth))
>> +		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
> Please don't use device_property_read_foo API, we don't want to give
> the impression it is O.K. to stuff DT properties in ACPI
> tables. Please use of_ API calls.

Hmm. Is this a new stance in DT handling for the networking tree?

If it is should I go back and rework some of my other drivers that use 
device_property APIs

Dan