netdev - Re: [PATCH] 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]

Message-ID: <5564C08E.2090400@ti.com>
Date:	Tue, 26 May 2015 13:50:54 -0500
From:	Dan Murphy <dmurphy@...com>
To:	Florian Fainelli <f.fainelli@...il.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: phy: dp83867: Add TI dp83867 phy

Florian

Thanks for the review!

On 05/26/2015 01:04 PM, Florian Fainelli wrote:
> On 26/05/15 06:07, Dan Murphy wrote:
>> 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.
>>
>> Signed-off-by: Dan Murphy <dmurphy@...com>
>> ---
> [snip]
>
>> +
>> +int rx_tx_delay = (DP83867_RGMIIDCTL_2_75_NS << DP83867_RGMII_RX_CLK_DELAY_SHIFT) | DP83867_RGMIIDCTL_2_25_NS;
>> +module_param(rx_tx_delay, int, 0664);
> This is not going to work, rx and tx delays are inherent properties of
> PCB/board designs, you want to be able to get that value from your
> platform configuration, Device Tree would certainly be preferred here.
> Asking an user to figure this out through module parameters is going to
> be both error prone, and limiting yourself to no more than one instance.

Yeah I agree.  I also have platform data for legacy products that I could put in the
DT as well.

> [snip]
>
>> +
>> +static int dp83867_phy_reset(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	err = dp83867_config_init(phydev);
>> +	return err;
> you could do a tail-call return directly?

Yep.  Will change that

>
> [snip]
>
>> +
>> +static int __init dp83867_init(void)
>> +{
>> +	return phy_driver_register(&dp83867_driver);
>> +}
>> +
>> +static void __exit dp83867_exit(void)
>> +{
>> +	phy_driver_unregister(&dp83867_driver);
>> +}
>> +
>> +module_init(dp83867_init);
>> +module_exit(dp83867_exit);
> You could use module_phy_driver to eliminate some boilerplate here.

Nice I will do that.

Dan

-- 
------------------
Dan Murphy

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