Message-ID: <55CB61BC.1050209@list.ru>
Date:	Wed, 12 Aug 2015 18:09:48 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Madalin-Cristian Bucur <madalin.bucur@...escale.com>,
	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

12.08.2015 17:43, Madalin-Cristian Bucur пишет:
>> -----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);
> }
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().

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

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

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

>   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 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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists