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
---
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).
As to your problem: would it be possible to set speed & duplex
after you do of_phy_connect()? It returns the phy_device
pointer, and perhaps you can look into phydev->speed and
phydev->duplex at that point?
It would be possible but un-natural as I'd have probing information only available at
runtime.
This is un-natural only if you deal just with a fixed case.
If your driver can deal also with the non-fixed cases
(either AN or MDIO), then this looks more natural as the
non-fixed management should be done at any point of time,
and certainly _after_ of_phy_connect(). So if your driver is
universal, this look like the natural choise to me, but if it is
limited to the fixed case, then, as a simplification, you move
that to the init time.
But I am not argueing what is more natural. Maybe the
above approaches with of_phy_find_device() can be used
in init time?
That would just complicate matters for my particular case ans I suspect there
will be other drivers that get into this situation.
I suspect only those that are limited to the fixed-link case.
Only then they may decide to move phy management to
init time, but IMHO this is just an optimization.
You are concerned about people
abusing this API to read fixed link status when the link is not really fixed, I'm concerned
about declaring the link as fixed-link when it's not. Maybe the naming/binding needs to be
revised to cover the case when all is fixed but the link.
Yes, naming is the problem. fixed-link is just a bad name.
See how it is defined:
---
Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device.
---
To me this means any non-MDIO PHY connection, but
unfortunately the name "fixed-link" suggests a bit more
than advertised. :(
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
