Date:	Wed, 12 Aug 2015 19:03:01 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Madalin-Cristian Bucur <madalin.bucur@...escale.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>
Cc:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Liberman Igal <Igal.Liberman@...escale.com>,
	Stas Sergeev <stsp@...rs.sourceforge.net>,
	"joakim.tjernlund@...nsmode.se" <joakim.tjernlund@...nsmode.se>,
	Shaohui Xie <Shaohui.Xie@...escale.com>
Subject: Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

12.08.2015 18:27, Madalin-Cristian Bucur пишет:
>>> Then I could call only of_* functions but the end result would be the same
>>> and  of_phy_register_fixed_phy() would not justify its existence that much...
>> You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
>> Since it is there (and even changed by me in a way your
>> patch will likely clash), IMHO it would be better if it is used,
>> rather than copy/pasted into the driver.
> Please note I was referring to a fictional new function that would embed the call to
> fixed_phy_register(). I was not talking about some existing API, just about a new
> of_call  named of_phy_register_fixed_phy()  that would in the end be called by
> of_phy_register_fixed_link() and by some drivers that want to get in the middle
> of things and get a hold on status.
Hmm, and for exactly unknown reason in your pseudocode
of_phy_register_fixed_phy() doesn't take status as an argument. :)
So I didn't see its point.
If you fix your pseudo-code, you'll add the status argument,
because it is needed for fixed_phy_register() anyway.
After that, the drivers that want to provide the status, will
just use it rather than to call fixed_phy_register() directly.
And with my changes it really have even more merits to exist.

> Maybe the fact we're reviewing two patches in one thread is what makes the
> discussion less than optimal.
I guess the bugs in the pseudo-code made me to miss its point.

>>> The getter for status you suggest would be fine, but not sure how one
>>> would retrieve
>>> it from the mac_node unless we change of_phy_register_fixed_link() to
>>> return the
>>> pointer to phy (and all the drivers that use it...).
>> If you look for instance to mvneta.c, you'll find the following:
>> ---
>> err = of_phy_register_fixed_link(dn);
>> /* In the case of a fixed PHY, the DT node associated
>>    * to the PHY is the Ethernet MAC DT node.
>>    */
>>    phy_node = of_node_get(dn);
>> ...
>> phy = of_phy_find_device(dn);
>> ---
>>
>> So the answer is: just use the same mac_node for both.
> I understand, I'll use this approach although is suboptimal imho to
Exactly!
But at least this way is used in many currently existing
drivers, while getting the fixed-link parameters directly from
DT - is a new way of circumventing the existing API.
So I'd vote for the currently existing hacks, and in fact
I already tried to start a discussion about getting rid of the
need for of_phy_find_device(), but it didn't go.

> scan the device tree again to get a phy pointer that you need just
> to get some of info that was parsed in a call you just made.
Maybe (just maybe) of_phy_register_fixed_link() could
return the phy_device pointer. At least it will solve your
problem very cheaply. But I am sure such API additions
require a separate discussion, can't be done in a context
of discussing a small fix. If you have some free time, feel
free to raise such a discussion with API extension proposals.
--
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

Powered by blists - more mailing lists