Florian
Thanks for the re-review
On 06/03/2015 09:47 PM, Florian Fainelli wrote:
> Le 06/02/15 07:34, Dan Murphy a écrit :
>> Add support for the TI dp83867 Gigabit ethernet phy
>> device.
>>
>> The DP83867 is a robust, low power, fully featured
>> Physical Layer transceiver with integrated PMD
>> sublayers to support 10BASE-T, 100BASE-TX and
>> 1000BASE-T Ethernet protocols.
> Sorry for the late feedback, since this is a new driver, things outline
> below can still be submitted as incremental fixes.
>
> [snip]
>
>> +Required properties:
>> + - reg - The ID number for the phy, usually a small integer
>> + - ti,rx_int_delay - RGMII Recieve Clock Delay - see
>> dt-bindings/net/ti-dp83867.h
>> + for applicable values
>> + - ti,tx_int_delay - RGMII Transmit Clock Delay - see
>> dt-bindings/net/ti-dp83867.h
>> + for applicable values
>> + - ti,fifo_depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
>> + for applicable values
> We typically use "-" to separate words in DT properties, not "_". I am
> not sure about the required nature of these 3 proprietary/specific
> properties, cannot there be good reset defaults from the HW in any case?
Yes you are correct I will modify as an incremental fix.
The hardware is defaulted to an internal delay of 2 ns. This value needs to be
adjusted
per product based on trace length. I would anticipate the DT properties to
grow a little as the
part gets used more.
> You might want to add a reference to net/phy.txt for the remaiming DT
> properties.
>
> [snip]
>
>> +
>> + if (phy_interface_is_rgmii(phydev)) {
>> + ret = phy_write(phydev, MII_DP83867_PHYCTRL,
>> + (dp83867->fifo_depth <<
>> DP83867_PHYCR_FIFO_DEPTH_SHIFT));
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) ||
>> + (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
> This one has not been converted to use the phy_interface_is_rgmii()
> helper, but in fact, once you do that, you could probably just add an
> early check for this condition, return, and reduce the indentation for
> the normal case/RGMII, and eliminate two redundant condition checks.
This check is different then the generic RGMII check. I am checking to see if
the interface has the internal
delay flag set. The RGMII check is to broad. Maybe I can add a helper for this
if it makes sense thoughts? Maybe a phy_interface_has_rgmii_int_delay API just
to enter this check.
Dan
> Thanks!
--
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
