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

[Dan Murphy]

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