Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy

[Dan Murphy <dmurphy@ti.com>]

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