Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code
From: Stas Sergeev
Date: Wed Aug 12 2015 - 11:10:02 EST
12.08.2015 17:43, Madalin-Cristian Bucur ÐÐÑÐÑ:
-----Original Message-----
From: Stas Sergeev [mailto:stsp@xxxxxxx]
12.08.2015 16:26, Madalin-Cristian Bucur ÐÐÑÐÑ:
I've tried to make the smallest changes that allow me to retrieve those
without modifying existing API.
Why is it important to hide the default values from the MAC driver?
My worry is that the fixed values are not really fixed, and
therefore are not always useful to access directly. It is likely
not a problem for your use-case, as, as you say, the AN is
disabled, but this is probably not the best to do in general.
Yes, not a problem in my case.
And also you do:
---
- err = of_phy_register_fixed_link(mac_node);
- if (err)
+ struct phy_device *phy;
+
+ mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
fixed_link),
+ GFP_KERNEL);
+ if (of_phy_parse_fixed_link(mac_node, mac_dev-
fixed_link))
+ goto _return_dev_set_drvdata;
+
+ phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
+ mac_node);
---
which means you really want to circumvent the current OF
api quite a lot, without saying why in the patch description.
I circumvent the API because I din not want to change existing API.
If I could get a reference to the status struct without changing any code
or without being required to call by myself fixed_phy_register(), I
would of done that. Given the existing code in
of_phy_register_fixed_link(),
this was my only option. I could have broken of_phy_register_fixed_link()
in two functions:
of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter
doing only
the call to fixed_phy_register()
that would allow to keep of_phy_register_fixed_link() as it is, broken in
two stages:
- parsing
- registering
than can be used by other drivers in order to get the status but I think it's
overkill.
What I referred to as "circumventing an API" is that you do
phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
by hands, instead of letting the of_phy_register_fixed_link() doing so.
How about a smaller circumvention, like this for instance:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
status = fixed_phy_get_link_status(phy); // no such func, to be coded up
---
Or even like this:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
set_speed_and_duplex(phy->speed, phy->duplex); // not sure if these
values are available that early
---
After my patch, all that of_phy_register_fixed_link() does is to call
the new parsing function I introduced then register the fixed PHY.
I could have done this (pseudocode):
- add of_phy_parse_fixed_link() as seen in the patch
- add of_phy_register_fixed_phy() that just calls fixed_phy_register():
int of_phy_register_fixed_phy(node)
{
phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
mac_node);
return (!phy);
}
- change of_phy_register_fixed_link() to contain only this:
int of_phy_register_fixed_link(node)
{
of_phy_parse_fixed_link(node, &status);
return of_phy_register_fixed_phy(node);
}
But have you looked into the patch I pointed previously?
https://lkml.org/lkml/2015/7/20/711
You code will likely clash with it because my patch extends
of_phy_register_fixed_link().
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.
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.
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 enough:
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.
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 fixed
(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?
--
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/