Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org>]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html