RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code
From: Madalin-Cristian Bucur
Date: Wed Aug 12 2015 - 11:27:56 EST
> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@xxxxxxx]
> But have you looked into the patch I pointed previously?
> You code will likely clash with it because my patch extends
I admin I failed to grasp the details of your change - the lack of ample context
Lines makes it a bit difficult. I'm sure your change could be merged then the
of parsing could be separated from the actual fixed_phy_register() call if anyone
cares about that.
> > 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.
Maybe the fact we're reviewing two patches in one thread is what makes the
discussion less than optimal.
> > 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
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.
> >> Also I meant the description should have been in the patch,
> >> not in the e-mail. :) You only wrote _what_ the patch does
> >> (which is of course obvious from the code itself), but not
> >> _why_ and _what was fixed_ (what didn't work).
> > If you refer to the first, separation patch, I thought the description was
> > of: separate fixed link parsing from registration
> > Some drivers may need
> "may need"? I don't understand.
> If it is a fix, then they _do need_, and in this case it should
> be specified what was broken and what is fixed.
> If it is just a clean-up, then "may need" may suffice, but it
> was not mentioned it is a clean-up. So I still don't know what
> this patch is all about.
> "Some drivers" - which ones? The ones that are limited to
> the purely fixed links, and never support AN or MDIO?
> Or some other drivers too?
> So really, the description sounds very cryptic to me.
Mine, when there is a fixed link node, maybe others. When there isn't any
fixed link node, the internal PHY config defaults to 1G full duplex AN enabled
and adjust link takes care of things.
> > to parse the fixed link values before registering
> > the fixed link phy. Separate the parsing from the actual registration
> > and provide an export for the added parsing function.
> > Signed-off-by: Madalin Bucur <madalin.bucur@xxxxxxxxxxxxx>
> > For this one it was a bit brief, I admit - the longer version would be that
> before it
> > we were not using from fixed link anything else but the fact the link was
> > (ignored actual speed, duplex values there)
> And what didn't work as the result?
> > and this patch tries to fix that.
> What started to work after that patch that didn't without it?
10M half duplex for instance
I'd close this thread for now and use in my driver of_phy_find_device(mac_node).