Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932742Ab3HNPF5 (ORCPT ); Wed, 14 Aug 2013 11:05:57 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:43837 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932404Ab3HNPFz (ORCPT ); Wed, 14 Aug 2013 11:05:55 -0400 Message-ID: <520B9C9E.8010002@ti.com> Date: Wed, 14 Aug 2013 20:35:02 +0530 From: Kishon Vijay Abraham I User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 MIME-Version: 1.0 To: Tomasz Figa , CC: Sylwester Nawrocki , Tomasz Figa , , , , , , , , , Alan Stern , , , , , , , , , , Greg KH , , , , Subject: Re: [PATCH 01/15] drivers: phy: add generic PHY framework References: <20130720220006.GA7977@kroah.com> <520A2100.6000709@ti.com> <520AB0F0.8010106@gmail.com> <2128883.KpgODjXPJQ@flatron> In-Reply-To: <2128883.KpgODjXPJQ@flatron> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7074 Lines: 178 Hi, On Wednesday 14 August 2013 04:34 AM, Tomasz Figa wrote: > On Wednesday 14 of August 2013 00:19:28 Sylwester Nawrocki wrote: >> W dniu 2013-08-13 14:05, Kishon Vijay Abraham I pisze: >>> On Tuesday 13 August 2013 05:07 PM, Tomasz Figa wrote: >>>> On Tuesday 13 of August 2013 16:14:44 Kishon Vijay Abraham I wrote: >>>>> On Wednesday 31 July 2013 11:45 AM, Felipe Balbi wrote: >>>>>> On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I > wrote: >>>>>>>>>>>> IMHO we need a lookup method for PHYs, just like for clocks, >>>>>>>>>>>> regulators, PWMs or even i2c busses because there are complex >>>>>>>>>>>> cases >>>>>>>>>>>> when passing just a name using platform data will not work. I >>>>>>>>>>>> would >>>>>>>>>>>> second what Stephen said [1] and define a structure doing >>>>>>>>>>>> things >>>>>>>>>>>> in a >>>>>>>>>>>> DT-like way. >>>>>>>>>>>> >>>>>>>>>>>> Example; >>>>>>>>>>>> >>>>>>>>>>>> [platform code] >>>>>>>>>>>> >>>>>>>>>>>> static const struct phy_lookup my_phy_lookup[] = { >>>>>>>>>>>> >>>>>>>>>>>> PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", >>>>>>>>>>>> "phy.2"), >>>>>>>>>>> >>>>>>>>>>> The only problem here is that if *PLATFORM_DEVID_AUTO* is used >>>>>>>>>>> while >>>>>>>>>>> creating the device, the ids in the device name would change >>>>>>>>>>> and >>>>>>>>>>> PHY_LOOKUP wont be useful. >>>>>>>>>> >>>>>>>>>> I don't think this is a problem. All the existing lookup >>>>>>>>>> methods >>>>>>>>>> already >>>>>>>>>> use ID to identify devices (see regulators, clkdev, PWMs, i2c, >>>>>>>>>> ...). You >>>>>>>>>> can simply add a requirement that the ID must be assigned >>>>>>>>>> manually, >>>>>>>>>> without using PLATFORM_DEVID_AUTO to use PHY lookup. >>>>>>>>> >>>>>>>>> And I'm saying that this idea, of using a specific name and id, >>>>>>>>> is >>>>>>>>> frought with fragility and will break in the future in various >>>>>>>>> ways >>>>>>>>> when >>>>>>>>> devices get added to systems, making these strings constantly >>>>>>>>> have >>>>>>>>> to be >>>>>>>>> kept up to date with different board configurations. >>>>>>>>> >>>>>>>>> People, NEVER, hardcode something like an id. The fact that >>>>>>>>> this >>>>>>>>> happens today with the clock code, doesn't make it right, it >>>>>>>>> makes >>>>>>>>> the >>>>>>>>> clock code wrong. Others have already said that this is wrong >>>>>>>>> there >>>>>>>>> as >>>>>>>>> well, as systems change and dynamic ids get used more and more. >>>>>>>>> >>>>>>>>> Let's not repeat the same mistakes of the past just because we >>>>>>>>> refuse to >>>>>>>>> learn from them... >>>>>>>>> >>>>>>>>> So again, the "find a phy by a string" functions should be >>>>>>>>> removed, >>>>>>>>> the >>>>>>>>> device id should be automatically created by the phy core just >>>>>>>>> to >>>>>>>>> make >>>>>>>>> things unique in sysfs, and no driver code should _ever_ be >>>>>>>>> reliant >>>>>>>>> on >>>>>>>>> the number that is being created, and the pointer to the phy >>>>>>>>> structure >>>>>>>>> should be used everywhere instead. >>>>>>>>> >>>>>>>>> With those types of changes, I will consider merging this >>>>>>>>> subsystem, >>>>>>>>> but >>>>>>>>> without them, sorry, I will not. >>>>>>>> >>>>>>>> I'll agree with Greg here, the very fact that we see people >>>>>>>> trying to >>>>>>>> add a requirement of *NOT* using PLATFORM_DEVID_AUTO already >>>>>>>> points >>>>>>>> to a big problem in the framework. >>>>>>>> >>>>>>>> The fact is that if we don't allow PLATFORM_DEVID_AUTO we will >>>>>>>> end up >>>>>>>> adding similar infrastructure to the driver themselves to make >>>>>>>> sure >>>>>>>> we >>>>>>>> don't end up with duplicate names in sysfs in case we have >>>>>>>> multiple >>>>>>>> instances of the same IP in the SoC (or several of the same PCIe >>>>>>>> card). >>>>>>>> I really don't want to go back to that. >>>>>>> >>>>>>> If we are using PLATFORM_DEVID_AUTO, then I dont see any way we >>>>>>> can >>>>>>> give the correct binding information to the PHY framework. I think >>>>>>> we >>>>>>> can drop having this non-dt support in PHY framework? I see only >>>>>>> one >>>>>>> platform (OMAP3) going to be needing this non-dt support and we >>>>>>> can >>>>>>> use the USB PHY library for it.> >>>>>> >>>>>> you shouldn't drop support for non-DT platform, in any case we >>>>>> lived >>>>>> without DT (and still do) for years. Gotta find a better way ;-) >>>>> >>>>> hmm.. >>>>> >>>>> how about passing the device names of PHY in platform data of the >>>>> controller? It should be deterministic as the PHY framework assigns >>>>> its >>>>> own id and we *don't* want to add any requirement that the ID must >>>>> be >>>>> assigned manually without using PLATFORM_DEVID_AUTO. We can get rid >>>>> of >>>>> *phy_init_data* in the v10 patch series. >> >> OK, so the PHY device name would have a fixed part, passed as >> platform data of the controller and a variable part appended >> by the PHY core, depending on the number of registered PHYs ? >> >> Then same PHY names would be passed as the PHY provider driver's >> platform data ? >> >> Then if there are 2 instances of the above (same names in platform >> data) how would be determined which PHY controller is linked to >> which PHY supplier ? >> >> I guess you want each device instance to have different PHY device >> names already in platform data ? That might work. We probably will >> be focused mostly on DT anyway. It seem without DT we are trying >> to find some layer that would allow us to couple relevant devices >> and overcome driver core inconvenience that it provides to means >> to identify specific devices in advance. :) Your proposal sounds >> reasonable, however I might be missing some details or corner cases. >> >>>> What about slightly altering the concept of v9 to pass a pointer to >>>> struct device instead of device name inside phy_init_data? >> >> As Felipe said, we don't want to pass pointers in platform_data >> to/from random subsystems. We pass data, passing pointers would >> be a total mess IMHO. > > Well, this is a total mess anyway... I don't really get the point of using > PLATFORM_DEVID_AUTO. The only thing that comes to my mind is that you can > use it if you don't care about the ID and so it can be assigned > automatically. > > However my understanding of the device ID is that it was supposed to > provide a way to identify multiple instances of identical devices in a > reliable way, to solve problems like the one we are trying to solve > here... > > So maybe let's stop solving an already solved problem and just state that > you need to explicitly assign device ID to use this framework? Felipe, Can we have it the way I had in my v10 patch series till we find a better way? I think this *non-dt* stuff shouldn't be blocking as most of the users are dt only? Thanks Kishon -- 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/