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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 08 Jun 2015 11:03:49 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Dan Murphy <dmurphy@...com>, netdev@...r.kernel.org,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy

On 08/06/15 07:04, Dan Murphy wrote:
> 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.

Ok, that makes sense then.

> 
>> 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.

Oh, I misread the range check. I do not think this deserves a helper
function for now, since that seems to be pretty unique and localized to
your driver for now, but if we have more than one users, factoring
things out would definitively make sense then.

Thanks.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html