From: Madalin-Cristian Bucur <madalin.bucur@freescale.com>
To: Stas Sergeev <stsp@list.ru>, Florian Fainelli <f.fainelli@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "grant.likely@linaro.org" <grant.likely@linaro.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>
CC: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Liberman Igal <Igal.Liberman@freescale.com>,
        Stas Sergeev <stsp@users.sourceforge.net>,
        "joakim.tjernlund@transmode.se" <joakim.tjernlund@transmode.se>,
        Shaohui Xie <Shaohui.Xie@freescale.com>
Subject: RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code
Thread-Topic: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code
Thread-Index: AQHQz40u7/EeGP0M3EaSWT7KT29M554CYVyAgASdLgCAAAcBIIAACVUAgAFL3eCAABP/gIAABbOQgAAOUgCAAAFHEA==
Date: Wed, 12 Aug 2015 15:27:47 +0000
Message-ID: <BL2PR03MB545A213D584384018CCB30CE67E0@BL2PR03MB545.namprd03.prod.outlook.com>
References: <1438785745-15517-1-git-send-email-madalin.bucur@freescale.com>
 <55C63D3B.5020005@gmail.com> <55CA1C14.3000202@list.ru>
 <BL2PR03MB545D4E18DE41AE01AFC37D6E67F0@BL2PR03MB545.namprd03.prod.outlook.com>
 <55CA29C8.5000707@list.ru>
 <BL2PR03MB545929237EF33BA13089EC1E67E0@BL2PR03MB545.namprd03.prod.outlook.com>
 <55CB50F1.7050308@list.ru>
 <BL2PR03MB5458B9D2402DE68E9B9014AE67E0@BL2PR03MB545.namprd03.prod.outlook.com>
 <55CB61BC.1050209@list.ru>
In-Reply-To: <55CB61BC.1050209@list.ru>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Aug 2015 15:27:47.5861
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR03MB499
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t7CFRw5C019574
Content-Length: 4281
Lines: 103

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@list.ru]

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

I admin I failed to grasp the details of your change - the lack of ample context
Lines makes it a bit difficult. I'm sure your change could be merged then the
of parsing could be separated from the actual fixed_phy_register() call if anyone
cares about that.

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

Please note I was referring to a fictional new function that would embed the call to 
fixed_phy_register(). I was not talking about some existing API, just about a new 
of_call  named of_phy_register_fixed_phy()  that would in the end be called by
of_phy_register_fixed_link() and by some drivers that want to get in the middle
of things and get a hold on status.

Maybe the fact we're reviewing two patches in one thread is what makes the
discussion less than optimal.

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

I understand, I'll use this approach although is suboptimal imho to
scan the device tree again to get a phy pointer that you need just
to get some of info that was parsed in a call you just made.

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

Mine, when there is a fixed link node, maybe others. When there isn't any
fixed link node, the internal PHY config defaults to 1G full duplex AN enabled
and adjust link takes care of things.

> 
> >   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@freescale.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 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?

10M half duplex for instance

I'd close this thread for now and use in my driver of_phy_find_device(mac_node).

Thank you,
Madalin
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?