From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967065AbdIZCC0 (ORCPT ); Mon, 25 Sep 2017 22:02:26 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:51791 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751377AbdIZCCX (ORCPT ); Mon, 25 Sep 2017 22:02:23 -0400 X-AuditID: b6c32a45-d3bff70000001088-75-59c9b52ccec4 MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="utf-8" Message-id: <59C9B52D.8050909@samsung.com> Date: Tue, 26 Sep 2017 11:02:21 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Linus Walleij , Rob Herring Cc: MyungJoo Ham , "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" , John Stultz , Guenter Roeck , "devicetree@vger.kernel.org" Subject: Re: [PATCH 1/8] extcon: gpio: Add DT bindings In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa2xLYRjHvee0p2ej27synowwRyS2aK3VdYeYOykmWfhgCHWsb7axtktP Jy5BhzATyy4u20yKjG2Ny26WDcuk5i5bjLmNIW4jkUiwEpfp6SH27cn/+T23fx6W1vxWRrEZ dhdx2oVMjglVNF6J4bUTz99cHnfr1Aje09au5O/80PIHflZSfL3nJ+LvXShn+NfH71B8d04V w/dfalLNZM113r2M+emDS4y52e9WmfMbvMj8uW50snIFmZZOBCtxRhN7qsOaYU9L5BYttcyx xJvi9Fr9FD6Bi7YLNpLIzU1K1s7PyAyswkVvEDKzA1KyIIrcpOnTnI5sF4lOd4iuRG6lXm/Q 6eMSdAaDQWecvGqqIT6ArCHpBX0nqaw+fmPDzv1KN6qYmIdCWMBGyD1/l8pDoawGNyHwVHyh pIQG+xHcPzD4H9Rx9AMjQzUI3uy4zUgJNY6Ab8U9ijzEsjQeA22d6yWZxjHQ+6VIIfM9CAqr qyiZj4Wrvl6VxCvweGi9HyrJTEBu7X0UbBmOx0LXt1dIiiNxCjR7+lRSPAwvgu6n32mpJ43r KDjzsjyYGIpNUHzOGywOwUvB33IxOBjwQwY+PamgpWGA50JNKSUfMxQ+XG9QyfFIeOutRTK/ B0F9ec/f4lwEz6tzlDI1GW7l5VDyaWGQe+WXSm6qhtzdGhkxw4VDu5Asz4KT75bIx9+lwPe4 UVGARpcN8Kvsv19lA/w6hmgvGk6yRFsaEQ1Zep0o2MRse5ou1WGrQ8Hfi53XhErak3wIs4gb onZTN5drlMIGcZPNh4CluWHqMfUBSW0VNm0mTofFmZ1JRB+KD9hdSEdFpjoCn2x3WfTGKXFG k8mQYOID/zVCvb3xQYoGpwkusp6QLOL8V0exIVFudPhMiGu2/fSp4UlFD40Nl7sWnv74++u6 e7v62ptveFKs+ZbEmS1zfnhmva+sGjTd6t5ftX3Bo1H+HSVrz3a2HWlvebd7S+NWS3/vhKgZ F3u+bnzdzS+rvXaClHb1N78IKz677ap23+KEfLMDWumFfkN457qCiFbXajXpeDbuYERGUs0K TiGmC/pY2ikKfwBlIQ7xkQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrPLMWRmVeSWpSXmKPExsVy+t9jQV2drScjDVZO5bCYf+Qcq8WZ37oW U/4sZ7LYPP8Po8XlXXPYLJ4sPMNkcbtxBZvF/z072B04PDat6mTzuHNtD5vHzu8N7B59W1Yx enzeJBfAGsVlk5Kak1mWWqRvl8CVMeHbUqaCbxYVW5p7WRsYl+h0MXJySAiYSJyf+4oNxBYS WMcoMWmHCYjNKyAo8WPyPZYuRg4OZgF5iSOXsiFMdYkpU3K7GLmAqh8wSpy++pUNolxL4uih F+wgNSwCqhL7r3CBhNmAwvtf3AAr4RdQlLj64zEjSImoQIRE94lKkLCIgLfE7Ts/mUFGMgts YpI487+fFSQhLGAmMXn9KjaIXReZJP71/AUbxCkQLNH5pJt1AqPALCSXzkK4dBbCpQsYmVcx SqYWFOem5xYbFRjlpZbrFSfmFpfmpesl5+duYgQG+7bDWv07GB8viT/EKMDBqMTDe4PlZKQQ a2JZcWXuIUYJDmYlEV75zUAh3pTEyqrUovz4otKc1OJDjNIcLErivPz5xyKFBNITS1KzU1ML UotgskwcnFINjOVap2fNsP+/udpxdqjZIU+NDdWbrM9bPUryz1N8WJ68Mnf13L/hGnOUSy5O erL/SsBvplIW5SNLJt1vNv399q1lX5G49Vu3hvuyryZ/b72cyxu0Qtn4p4q6ygc7DneT40yn coPvfFun4eCopSrxy/X8AgFZ923/Hqyw0Y8yf3xr5YOJF169OqHEUpyRaKjFXFScCABpnv4/ cgIAAA== X-CMS-MailID: 20170926020220epcas2p120a10b682bc55945cc757d2dccd0a0f4 X-Msg-Generator: CA X-Sender-IP: 182.195.42.143 X-Local-Sender: =?UTF-8?B?7LWc7LCs7JqwG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLw=?= =?UTF-8?B?7YSwKRvsgrzshLHsoITsnpAbU2VuaW9yIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?Q2hhbndvbyBDaG9pG1RpemVuIFBsYXRmb3JtIExhYi4bU2Ft?= =?UTF-8?B?c3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG1RFTEUbQzEwVjgxMTE=?= CMS-TYPE: 102P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20170926003941epcas4p2a1987500f6e213b6a2ffe973ab1db922 X-RootMTR: 20170926003941epcas4p2a1987500f6e213b6a2ffe973ab1db922 References: <20170924145622.4031-1-linus.walleij@linaro.org> <20170924145622.4031-2-linus.walleij@linaro.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rob, On 2017년 09월 26일 09:39, Linus Walleij wrote: > On Sun, Sep 24, 2017 at 9:56 PM, Rob Herring wrote: >> On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij wrote: >>> Add some reasonable device tree bindings and also add the cable defines >>> to the extcon include in since >>> the GPIO extcon definately need to specify which cable/connector it is >>> detecting. >>> >>> Adding the include file makes the (as it happens) Linux numbers into an >>> ABI, but I do not see any better method. It is possible to define strings >>> for all cable types but it seems like overkill, just reusing Linux' >>> enumerators seems like a good idea. >>> >>> The binding supports any number of GPIOs and connectors, but the driver >>> currently only supports one connector on one GPIO line. >> >> My view of extcon binding is it is fundamentally broken. I've >> expressed this multiple times before. > > Sorry, I'm a newcomer in this area, so I was not aware of this. > > Since this is a new binding consumer, is it something we can use > as role model to fix it? > > If I understand correctly from reading up on the mailing list the root of the > problem is something like this: > > "extcon" is a linuxism and ambiguous. > > This driver should probe to "gpio-connector" or "gpio-switch" or something > like that if it should be generic. Or use very domain-specific compatible > strings (as you describe below), all supported maybe by the same driver > in the end. > > The reason it is its own subsystem and not part of input (IIUC) is that > other drivers need to subscribe to events from these connectors, > they are not intended for userspace input such as keyboard or mice > or similar. > > In the DTS file you will find stuff using extcons as resources with > = <&extcon>; phandles so they can look them up and subscribe > to events. > > Input has a whole slew of "events" that correspond to some of these > but a different usecase, but that usecase is just a linuxism in turn, there > is nothing saying another operating system could have a more versatile > defintion of "input". > > Maybe from a hardware description PoV these should all move over > to devicetree/bindings/input - they all provice an input signal to the > system. The fact that Linux split a subset of these into "extcon" > is of no concern to the DT bindings. > > Analogous with that some of the stuff in input/ should likely be > moved to a new output/ directory. Such as pwm-beeper, > pwm-vibrator etc. The fact that these are in the "input" subsystem > in Linux is just another linuxism. > >> TL;DR: Anything extending the existing extcon binding will be NAKed. > > That can be a reasonable stance. > > I'm just trying to get this into a state where the code does not stand in > the way of kernel clean-up. (Especially I want to get rid of the call > to gpio_to_desc()) > > As stated in the cover letter the alternative will be to simply delete > the driver. But it's better if I can fix the situation, we can't have it > like this. > >>> +External Connector Using GPIO >> >> What kind of connector? All connectors external to something... And >> GPIO is not a kind of connector on its own, but an implementation. > > Yeah that is a problem with the whole subsystem I guess. Should > I add a paragraph describing the usecases? > > The whole thing is a bit > of Androidism and is named like this because Android named it like > this in their kernel tree. > >>> +Required properties: >>> +- compatible: should be "extcon-gpio" >>> +- extcon-gpios: the GPIO lines used for the external connectors >> >> This doesn't tell me what the GPIOs functions are and should. For >> example we have hpd-gpios for HDMI hotplug detect in HDMI connector >> binding. > > The idea is that this array corresponds to the extcon-connector-types > right below, I'll clarify that if you think the overall idea is OK. > >>> + See gpio/gpio.txt >>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types >>> + of this connector, matched to the corresponding GPIO lines in the previous array. >> >> This should be determined by the compatible string. > > So a GPIO connector is very versatile. It is "general purpose" by definition, > so it needs to be subclassed. > > One possibility is to allow just one GPIO and have one comptible-string per > use case. > compatible = "gpio-usb-connector"; > compatible = "gpio-charger-connector"; > compatible = "gpio-headphone-connector"; > etc etc > > These bindings on the other hand, assume that the driver will be able > to handle an array of gpios with an associated array of connector types, > which reflects how many of the existing extcon-type driver hardware works: > a single PMIC providing some 5 misc extcons for example. > > For this reason there is a generic compatible string and then the device > is subclassed per-gpio with the per-gpio connector type. > >>> +/* USB external connector */ >>> +#define EXTCON_USB 1 >>> +#define EXTCON_USB_HOST 2 >>> +#define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */ >>> +#define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */ >>> +#define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */ >>> +#define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */ >>> +#define EXTCON_CHG_USB_FAST 9 >>> +#define EXTCON_CHG_USB_SLOW 10 >>> +#define EXTCON_CHG_WPT 11 /* Wireless Power Transfer */ >>> +#define EXTCON_CHG_USB_PD 12 /* USB Power Delivery */ >> >> These don't all look to be mutually exclusive. >> >> For USB PD, isn't that discoverable? Currently, EXTCON_CHG_USB_PD is not used on any extcon provider drivers. I think that EXTCON_CHG_USB_PD is discoverable according to the H/W design such as using ADC. Also, The charger connectors of extcon are related to power_supply subsystem because power_supply is in charge of behavior when the connector is attached/detached. So, the extcon defines the EXTCON_CHG_USB_PD in order to detect this type. > > Someone else would have to answer, this is based on the current > connector types supported by the Linux kernel. > >>> +/* Display external connector */ >>> +#define EXTCON_DISP_HDMI 40 /* High-Definition Multimedia Interface */ >>> +#define EXTCON_DISP_MHL 41 /* Mobile High-Definition Link */ >>> +#define EXTCON_DISP_DVI 42 /* Digital Visual Interface */ >>> +#define EXTCON_DISP_VGA 43 /* Video Graphics Array */ >>> +#define EXTCON_DISP_DP 44 /* Display Port */ >>> +#define EXTCON_DISP_HMD 45 /* Head-Mounted Display */ >> >> We already have connector bindings for most of these. Use those as a >> model for whatever you want to do. > > I guess they are not in Documentation/devicetree/bindings/extcon/* > > Please point me in the right direction and I'll check it out. > > Yours, > Linus Walleij > > > -- Best Regards, Chanwoo Choi Samsung Electronics