lkml.org 
[lkml]   [2011]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional
Oops, I just realized that I may have misused
scripts/get_maintainer.pl... it seems to have added about 20 CCs to
this email, sorry!

On Thu, Oct 20, 2011 at 17:14, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 20 October 2011 17:10:12 Mike Frysinger wrote:
>> On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
>> > Approximately 90% of the PHY drivers follow the PHY layer docs and
>> > simply use &genphy_read_status and &genphy_config_aneg.  There would
>> > seem to be little point in requiring them all to manually specify those
>> > functions.
>>
>> well, it does make sense if you think about the compile vs build time
>> overhead.  yes, your patch does make things much nicer to read, and a
>> little easier to maintain the source.  however, it adds runtime overhead
>> (checking the func pointers) while the func pointer storage is unchanged
>> (it's now a NULL pointer instead of pointing to the genphy funcs).
>> personally, i think the savings in runtime and smaller compiled code is
>> more important.  so i'm going to NAK this.  sorry.
>
> ah, sorry, i was thinking this was u-boot since we were just having
> conversations there.
>
> since this is Linux, and i don't have real standing in the general netdev
> community, i can't really NAK here.  but i think my comment still stands in
> that this patch makes things much worse than the minor code style improvement.

I would argue that the PHY layer itself is not remotely a hot-path,
especially not to the level of caring about an extra if statement. A
single phy_read() is a sleeping call which goes over a slow serial
bus, so code maintainability is much more important.

At the higher level, the reason for this change is that currently
genphy_config_aneg() is currently responsible both for configuring
auto-negotiation *AND* counterintuitively for configuring forced-mode
links. A lot of PHY drivers need to override just one or the other,
which results in either a lot of copy-pasted code in individual PHY
drivers or in a duplicate test for autoneg-vs-fixed-mode before
calling the primary genphy_config_aneg() function.

In order to split that up and refactor it, I would like to minimize
the number of callers and references to config_aneg() to minimize the
number of places that have to be changed to match. Since all the rest
of the PHY functions are already conditionals, making these two also
conditional didn't seem to be a big deal.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-10-21 07:17    [W:0.104 / U:0.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site