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

[Madalin-Cristian Bucur]

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@xxxxxxx]
> 
> 11.08.2015 19:33, Madalin-Cristian Bucur ÐÐÑÐÑ:
> > + Joakim, Shaohui
> >
> >> -----Original Message-----
> >> From: Stas Sergeev [mailto:stsp@xxxxxxx]
> >>
> >> 08.08.2015 20:32, Florian Fainelli ÐÐÑÐÑ:
> >>> CC'ing Stas,
> >> Hi.
> >>
> >>> Le 08/05/15 07:42, Madalin Bucur a Ãcrit :
> >>>> The FMan MAC configuration code needs the speed and duplex
> >> information
> >>>> for fixed-link interfaces that is parsed now by the of function
> >>>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
> >>>> does not expose to the caller neither the phy_device pointer nor the
> >>>> status struct where it loads the fixed-link params.
<snip>

> > 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.

> As such, it may be difficult to review. Could you please write
> a more complete description to the patch?

To better understand this patch, think of it as just a refactoring of the
of_phy_register_fixed_link() that does two things inside:

- parsing of fixed link node (2 bindings supported)
- register phy by calling fixed_phy_register() in the same way, in the same codebase

I've extracted the parsing in a separate function ( following the "one function should
do one thing" rule).

Then I've exported this function to make status available to callers.

> 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. That would just complicate matters for my particular case ans I suspect there
will be other drivers that get into this situation. 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.