lkml.org 
[lkml]   [2020]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next v3 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY
From
Date
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

\
 
 \ /
  Last update: 2020-11-03 18:08    [W:0.283 / U:0.868 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site