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 <dmur...@ti.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
