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

[Stas Sergeev]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/