> > Having dev-addr stored in devices_addrs, in get_phy_c45_ids(), when
> > probing the identifiers, dev-addr can be extracted from devices_addrs
> > and probed if devices_addrs[current_identifier] is not 0.
>
> I must clearly be missing something, but why are you introducing all these
> conditionals instead of updating the existing code to be able to operate
> against
> an arbitrary dev-addr value, and then just making sure the first thing you do
> is
> fetch that property from Device Tree? There is no way someone is going to be
> testing with your specific use case in the future (except yourselves) so
> unless you
> make supporting an arbitrary "dev-addr" value become part of how the code
> works, this is going to be breaking badly.
>
Hi Florian,
My intention was to have this patch as "plugin" and modify the existing kernel
API little to none. I was thinking that with a #ifdef, ideally, all changes
could be part of a CONFIG kernel option.
Updating the existing code, instead of the conditionals, might run into just
that, and the change could propagate across multiple modules. This is from my
first RFC patch, review by Andrew:
of_mdiobus_register(), when it loops over the children, looks for
the new property. If found, it passed dev-id to
of_mdiobus_register_phy().
That passes it to get_phy_device(). I think get_phy_device() can
then set the ID in c45_ids, before passing it to get_phy_id().
get_phy_c45_ids() will first look at devices in package and can add
further devices to c45_ids. It will then probe both those found, and
the static
one you added.
Andrew
[Vicenţiu Galanopulo]
Just to make sure I understand. Do you want me to change the signature
of all of_mdiobus_register_phy(), get_phy_device(), get_phy_id() and
get_phy_c45_ids() and include the dev_addr parameter obtained from the
device tree? (a propagation of this parameter across all functions
all the way to
get_phy_c45_devs_in_pkg?) This will impact xgbe-mdio.c, fixed_phy.c
because get_phy_device() is used in these files.
The "catch" is to transport the dev-addr value from of_mdio.c (location of the
loop of the PHY device tree node which reads all PHY node properties) to
phy_device.c (this is where you can get the PHY ID).
My understanding from Andrew's comment is that the key here is the c45_ids, and
that these could be filled in of_mdio.c, first, with the IDs from dev-addr (he
called them "static" as they are queried directly by using the value of
dev-addr) and afterwards, in phy_device.c (following the lookup loop - in a
"dynamic" way).
There's nothing more to this patch than some functionality from phy_device.c
ported to of_mdio.c, to enable the extraction of the PHY IDs.
I guess the code redundancy could be reduced (between of_mdio.c and
phy_device.c) and maybe you or Andrew could comment on this if you would like
to go with this patch approach.
Not sure I understand your comment about the specific use case and the breaking
badly part.
Right now I'm able to test because I have access to a PHY with dev-addr = 0x1e.
But the whole mechanism in this patch starts to work the moment you set
<dev-addr> in the device tree. If you don't set that, nothing happens. If you
set it to a bogus value, no PHY ID will be found at that address. Besides that,
the PHY ID extraction code is the same as what is currently working in
phy_device.c.
80-90% of the patch is based on what already exists in phy_device.c and
of_mdio.c. Where is the breaking badly part supposed to happen?
> And please, can you keep me copied for next submissions?
Yes, the "to" list was pretty long and I somehow missed you. Sorry.
Vicentiu
