Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754892AbbHLN6U (ORCPT ); Wed, 12 Aug 2015 09:58:20 -0400 Received: from smtp15.mail.ru ([94.100.176.133]:53951 "EHLO smtp15.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751898AbbHLN6S (ORCPT ); Wed, 12 Aug 2015 09:58:18 -0400 X-Greylist: delayed 21331 seconds by postgrey-1.27 at vger.kernel.org; Wed, 12 Aug 2015 09:58:17 EDT Subject: Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code To: Madalin-Cristian Bucur , Florian Fainelli , "netdev@vger.kernel.org" , "grant.likely@linaro.org" , "robh+dt@kernel.org" References: <1438785745-15517-1-git-send-email-madalin.bucur@freescale.com> <55C63D3B.5020005@gmail.com> <55CA1C14.3000202@list.ru> <55CA29C8.5000707@list.ru> Cc: "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Liberman Igal , Stas Sergeev , "joakim.tjernlund@transmode.se" , Shaohui Xie From: Stas Sergeev Message-ID: <55CB50F1.7050308@list.ru> Date: Wed, 12 Aug 2015 16:58:09 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4693 Lines: 113 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 --- 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). >> 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. This is un-natural only if you deal just with a fixed case. If your driver can deal also with the non-fixed cases (either AN or MDIO), then this looks more natural as the non-fixed management should be done at any point of time, and certainly _after_ of_phy_connect(). So if your driver is universal, this look like the natural choise to me, but if it is limited to the fixed case, then, as a simplification, you move that to the init time. But I am not argueing what is more natural. Maybe the above approaches with of_phy_find_device() can be used in init time? > That would just complicate matters for my particular case ans I suspect there > will be other drivers that get into this situation. I suspect only those that are limited to the fixed-link case. Only then they may decide to move phy management to init time, but IMHO this is just an optimization. > 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. :( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/