Message-ID: <BL2PR03MB5458B9D2402DE68E9B9014AE67E0@BL2PR03MB545.namprd03.prod.outlook.com>
Date:	Wed, 12 Aug 2015 14:43:38 +0000
From:	Madalin-Cristian Bucur <madalin.bucur@...escale.com>
To:	Stas Sergeev <stsp@...t.ru>,
	Florian Fainelli <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>
CC:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Liberman Igal <Igal.Liberman@...escale.com>,
	Stas Sergeev <stsp@...rs.sourceforge.net>,
	"joakim.tjernlund@...nsmode.se" <joakim.tjernlund@...nsmode.se>,
	Shaohui Xie <Shaohui.Xie@...escale.com>
Subject: RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@...t.ru]
>
> 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);
}

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

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

> 
> 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 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@...escale.com>

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 this patch tries to fix that.
In the end this patch will be squashed in a new FMan patch set, let me use that as
an excuse for the brief commit log :)

<snip>

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

Yes, maybe the new binding could be updated to load semantically the presence
or absence of certain parameters such as link.

Powered by blists - more mailing lists