From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932595AbbLHBR5 (ORCPT ); Mon, 7 Dec 2015 20:17:57 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:49002 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932146AbbLHBRz convert rfc822-to-8bit (ORCPT ); Mon, 7 Dec 2015 20:17:55 -0500 X-AuditID: cbfee68f-f793a6d000001364-8c-56662fc04eac MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <56662FBB.5080708@samsung.com> Date: Tue, 08 Dec 2015 10:17:47 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Heikki Krogerus Cc: Greg Kroah-Hartman , MyungJoo Ham , David Cohen , Lu Baolu , Mathias Nyman , Felipe Balbi , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCHv2 1/2] extcon: add driver for Intel USB mux References: <1449134991-39095-1-git-send-email-heikki.krogerus@linux.intel.com> <1449134991-39095-2-git-send-email-heikki.krogerus@linux.intel.com> <56600E3E.6080300@samsung.com> <20151204085136.GA24400@kuha.fi.intel.com> <5664DFC6.40801@samsung.com> <20151207125200.GA11485@kuha.fi.intel.com> In-reply-to: <20151207125200.GA11485@kuha.fi.intel.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrEIsWRmVeSWpSXmKPExsWyRsSkQPeAflqYwY1POhYH79dbbJ64lc3i 5+X77BbNi9ezWXSt3slicXnXHDaLRctamS1ef2hisbjduILNgdNj3slAj/1z17B79G1Zxehx /MZ2Jo/Pm+QCWKO4bFJSczLLUov07RK4Mvbfn8JcsMCq4tmKVcwNjHd0uxg5OSQETCT6531l hbDFJC7cW8/WxcjFISSwglFi1tb7rDBFt55fZ4JIzGKUOHhwOxtIgldAUOLH5HssXYwcHMwC 6hJTpuSChJkFRCSabq1lh7C1JZYtfM0M0fuAUaL101xGiF4tiYUtl5lBbBYBVYn9W16CNbAB xfe/uMEGMlNUIEKi+0QlSFhEwFRi7YSVrCBzmAX2Mkks3gBxg7CAvcT+ZTdYIBasZpKY+KEL bBCngLnE+m+P2CE+uMcu0faHH2KZgMS3yYfAjpYQkJXYdIAZokRS4uCKGywTGMVnIXltFsJr s5C8NgvJawsYWVYxiqYWJBcUJ6UXGesVJ+YWl+al6yXn525iBEbt6X/P+ncw3j1gfYhRgINR iYdX8WRqmBBrYllxZe4hRlOggyYyS4km5wNTQ15JvKGxmZGFqYmpsZG5pZmSOO9CqZ/BQgLp iSWp2ampBalF8UWlOanFhxiZODilGhjZxH9ONq4xlZ/3mD3mpv19/kOyz24wSuWePrDh24Z6 /vSpRrtUTBRfGDyrvxwrkfi1J62H6VPh4eW/Lu8/p37lZt5rhrkP89Tzl2XveKObUMn/sXxr ul1dWqVMwITQedGm822/Wc7h2LfouWZ/3eUUzrOrBY58YVzT2uEg/G/PnuLbGjnv5u5XYinO SDTUYi4qTgQAbyVtHdUCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrGIsWRmVeSWpSXmKPExsVy+t9jQd0D+mlhBrc61CwO3q+32DxxK5vF z8v32S2aF69ns+havZPF4vKuOWwWi5a1Mlu8/tDEYnG7cQWbA6fHvJOBHvvnrmH36NuyitHj +I3tTB6fN8kFsEY1MNpkpCampBYppOYl56dk5qXbKnkHxzvHm5oZGOoaWlqYKynkJeam2iq5 +AToumXmAN2jpFCWmFMKFApILC5W0rfDNCE0xE3XAqYxQtc3JAiux8gADSSsYcz4f6qLteCH ZcWXiYuZGxi36HYxcnJICJhI3Hp+nQnCFpO4cG89WxcjF4eQwCxGiYMHt7OBJHgFBCV+TL7H 0sXIwcEsIC9x5FI2hKkuMWVKLkT5A0aJ1k9zGSHKtSQWtlxmBrFZBFQl9m95yQ5iswHF97+4 wQbSKyoQIdF9ohIkLCJgKrF2wkpWkDnMAnuZJBZvgFgrLGAvsX/ZDRaIBauZJCZ+6AIbxClg LrH+2yP2CYxAVyKcNwvhvFkI5y1gZF7FKJFakFxQnJSea5iXWq5XnJhbXJqXrpecn7uJERzn z6R2MB7c5X6IUYCDUYmH98Sx1DAh1sSy4srcQ4wSHMxKIrytumlhQrwpiZVVqUX58UWlOanF hxhNgf6byCwlmpwPTEF5JfGGxiZmRpZG5oYWRsbmSuK8tZciw4QE0hNLUrNTUwtSi2D6mDg4 pRoY6xe2vE31ceCVOLmLk/On78K5f9eHXvuq+YfnxZIONh4bL0mhY2bfJx68OW//KoMTx/W5 7tRdOxh06lspp+f0SXlucUX/1ptPPdn5P0Mtf9qDuS/cbKTu50rd/GfWs3Gefm2osW27TOCj vpoZWzZOn/DFW+9v+/G/z9f5haoIT3maJbZ8QYczsxJLcUaioRZzUXEiANVdgJEJAwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 2015년 12월 07일 21:52, Heikki Krogerus wrote: > Hi, > > On Mon, Dec 07, 2015 at 10:24:22AM +0900, Chanwoo Choi wrote: >> Hi, >> >> On 2015년 12월 04일 17:51, Heikki Krogerus wrote: >>> Hi, >>> >>>> I do never want to add some specific funtcion for only this driver. >>>> I think is not appropriate way. >>>> - intel_usb_mux_unregister >>>> - intel_usb_mux_register >>>> >>>> The client driver using extcon driver should use the standard extcon API >>>> for code consistency. Also, I'll do the more detailed review for this patch. >>> >>> The internal mux we are controlling here is physically separate >>> device. Ideally we could populate child device for it, but since that >>> is not possible because of the resource conflict, we use the library >>> approach, which is really not that uncommon. >> >> New added functions for only specific device driver is not library. >> >> The all device drivers which is included in some framework should >> connect to the other device driver through only framework API as following: >> -------------------- ---------------- >> | EXTCON framework |<-------->| USB framework | >> -------------------- ----------------- >> | | >> | | >> extcon-intel-usb.c pci-quirks.c >> >> But, in this case, added funticon is just direct call function >> without any standard API. The below case is never appropriate implementation. >> >> -------------------- ---------------- >> | EXTCON framework | | USB framework | >> -------------------- ----------------- >> | | >> | | >> extcon-intel-usb.c <-------- pci-quirks.c > > Man.. Cal it what you want, but like I said, exposing driver specific > API is not ideal, but it is acceptable in special cases like this > where we simply are not able to populate child device. If nothing > else, then at least the fact that the code for the mux would otherwise > need to be duplicated, is enough to justify it. > >>> I don't think I agree with your point even at general level. The >>> control required to handle this mux, even though simple, is enough to >>> deserve to be separated from xHCI code. xHCI should not need to care >>> about anything else expect does it have the mux, i.e. does it need to >>> register it or not. It should not need to care about how it needs to >>> be controlled or even what it is. We may decide to create something >>> else out of it instead of an extcon device later. >>> >>> But in any case, the mux is available on all new Intel platforms, but >>> it needs to be controlled by OS only in few "special" cases. We can >>> not force xHCI (or pci-quirks.c to be more precise) to be aware of >>> these "special" cases. The only way to make it work like that would >>> bet by using ifdefs, and we really really don't want that. >>> >>> And please also note that, though for now we only expect the mux >>> control registers to be part of xHCI MMIO, that is not always the >>> case. The control registers are part of the device controller MMIO on >>> some platforms. We do not want to duplicate the whole control of the >>> mux if/when we need the OS to be in control of it on a platform that >>> has those control registers mapped somewhere else then xHCI MMIO, >>> >>> So I would say that we have pretty good justification for separating >>> the mux control, which means unfortunately custom API in this case. >>> >>> But if you would prefer that we put the files somewhere else then >>> drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you >>> like, we can put it to drivers/usb/host/ as that is where >>> pci-quirks.c is. That way I think we can also put the header to >>> include/usb/. >> >> There are the two type of extcon drivers as following: >> - provider extcon driver which use the devm_extcon_dev_register() and extcon_set_cable_state(). >> - client extcon driver which use the extcon_register_notifier() and extcon_set_cable_state() usually. >> The drivers/extcon directory only includes the provider extcon driver. >> >> You make the extcon-intel-usb.c as provider extcon driver. >> At the same time, this driver is used for client extcon driver >> by using the extcon_register_notifier(). If you want to recevie >> the notification from extcon_register_notifier(), the extcon device >> should update the state of external connector throught extcon_set_cable_state(). >> But, this driver don' use the extcon_set_cable_state(). >> >> I think that you should divide it according to role. >> >> Again, the usage case of extcon have to consist of both provider extcon driver >> and client extcon driver. If there is no provider extcon driver, >> client extcon driver don't receive the any notification of external connector >> from provider extcon driver. > > What you are saying is that it is OK for both "client" and "provider" > to change the state, but only client is allowed to react to a state > change? You got to admit that the roles are pretty obscure here... Yes, only client driver is able to take some behavior after receiving the notification from extcon provider driver. > > In any case, I'm using the framework the way it allows itself to be > used. If you want your framework to be used in a particular way, then > you need to protect it from being used otherwise. Now anything with a > handle to the extcon_dev can use it in every way possible. You are not > documenting how you want the framework to be used. The only document > for extcon in Documentation/extcon/ is about porting stuff from some > Android specific "switch class" to extcon. Nowhere do you define what > is a "client" and what is a "provider" and what are they meant for. I agree. The extcon framework uses the 'extcon_dev' structure for both provider and client driver. As you mentioned, it is the problem. So, I'll resolve this issue to protect change the state of external connector from client driver. Unitl now, I'm focusing on expand the usage case of extcon framework and improve the extcon core feature. As you comment, It is necessary task for extcon framework. The improvement of extcon framework is going on. > > If you want to limit what a "client" can do, you need to separate the > API for it. Use the gpio API as an example. Check how the consumers > and drivers are separated into their own headers in > include/linux/gpio/. Keep the include/extcon.h header as legacy and > deprecate it. > > And if you do that, the framework should really be improved with a few > basic things regarding the "clients". At least start using some kind > of ref counting with them. Now nothing really prevents a "provider" > from being removed even if it has users (clients), or does it? This > basically also shows how obscure the line between a "client" and > "provider" is at the moment. I agree. There are not enough document for extcon framework. After updating the extcon framework above, I'll make the appropriate document for guide. > > Right now with what we have I see nothing wrong with my approach. I can't agree to add specific function for only one device driver. As I commented, it is not appropriate way. Thanks, Chanwoo Choi