From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752027AbbJWF4R (ORCPT ); Fri, 23 Oct 2015 01:56:17 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:33492 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbbJWF4N (ORCPT ); Fri, 23 Oct 2015 01:56:13 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68e-f791c6d000001498-de-5629cbfa5903 Content-transfer-encoding: 8BIT Message-id: <5629CBFA.4020605@samsung.com> Date: Fri, 23 Oct 2015 14:56:10 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Sergei Shtylyov Cc: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, myungjoo.ham@samsung.com, grant.likely@linaro.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH] extcon: add MAX3355 driver References: <14631865.01KLlU2iKL@wasted.cogentembedded.com> <5490CEE2.1030900@samsung.com> <5491FC69.7090203@cogentembedded.com> <562685EF.7010504@cogentembedded.com> In-reply-to: <562685EF.7010504@cogentembedded.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrNIsWRmVeSWpSXmKPExsWyRsSkUPfXac0wg4471hbzj5xjteh/s5DV 4sCfHYwW516tZLS4vGsOm8WcP1OYLRYta2W2WHr9IpPF7cYVbBYTpq9lsWjde4Td4syqW+wO PB5r5q1h9Ljc18vk8WDqfyaPlcu/sHlsWtXJ5nHn2h42j74tqxg9Pm+SC+CI4rJJSc3JLEst 0rdL4MqY3judveCRWcWzpnesDYxbtLoYOTkkBEwk5nZsY4KwxSQu3FvP1sXIxSEksIJRYu/r iawwRYc2HWaHSCxllOg718kGkuAVEJT4MfkeSxcjBwezgLzEkUvZIGFmAXWJSfMWMUPUP2CU eNI6iRGiXkviyuTnYL0sAqoS/T92gW1mA4rvf3GDDWSOqECERPeJSpCwiICFxN3GRSwgc5gF 5jNJTDywAaxXWEBf4sXF11CXrmKU+P39JtggTgEjiQnLloJdKiEwkUNi5vNjLBDbBCS+TT4E dqmEgKzEpgPMEJ9JShxccYNlAqPYLCT/zEL4ZxaSfxYwMq9iFE0tSC4oTkovMtIrTswtLs1L 10vOz93ECIzj0/+e9e1gvHnA+hCjAAejEg/viw7NMCHWxLLiytxDjKZAR0xklhJNzgcmi7yS eENjMyMLUxNTYyNzSzMlcd4EqZ/BQgLpiSWp2ampBalF8UWlOanFhxiZODilGhgV7E+GCbwv DHre7Zzk1++i/vfPX+4/D/f3ZFU8KH6ZuyU6Yu+/CwuYAs8sLgvT9/edHHPkkqBjhc+3ZSLd TDJHikqd9WSvSensyl/4mYl3VqD8psWahVet1Cos2HxiDt/ZprLy59rfU944/plrUHtLiGlJ dsHipXaSmQ9ZG6dXO/xruT9TPVGJpTgj0VCLuag4EQD3efT13gIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprBKsWRmVeSWpSXmKPExsVy+t9jAd1fpzXDDKauMrWYf+Qcq0X/m4Ws Fgf+7GC0OPdqJaPF5V1z2Czm/JnCbLFoWSuzxdLrF5ksbjeuYLOYMH0ti0Xr3iPsFmdW3WJ3 4PFYM28No8flvl4mjwdT/zN5rFz+hc1j06pONo871/awefRtWcXo8XmTXABHVAOjTUZqYkpq kUJqXnJ+SmZeuq2Sd3C8c7ypmYGhrqGlhbmSQl5ibqqtkotPgK5bZg7QvUoKZYk5pUChgMTi YiV9O0wTQkPcdC1gGiN0fUOC4HqMDNBAwhrGjOm909kLHplVPGt6x9rAuEWri5GTQ0LAROLQ psPsELaYxIV769m6GLk4hASWMkr0netkA0nwCghK/Jh8j6WLkYODWUBe4silbJAws4C6xKR5 i5gh6h8wSjxpncQIUa8lcWXyc7BeFgFVif4fu5hAbDag+P4XN9hA5ogKREh0n6gECYsIWEjc bVzEAjKHWWA+k8TEAxvAeoUF9CVeXHwNddAqRonf32+CDeIUMJKYsGwp+wRGgVlI7puFcN8s JPctYGQG6k0tSC4oTkrPNcpLLdcrTswtLs1L10vOz93ECE4Vz6R3MB7e5X6IUYCDUYmH90WH ZpgQa2JZcWXuIUYJDmYlEd6QZqAQb0piZVVqUX58UWlOavEhRlOgBycyS4km5wPTWF5JvKGx iZmRpZG5oYWRsbmSOO+FDI0wIYH0xJLU7NTUgtQimD4mDk6pBkaOjqMx/75PvLp+76JZbcrL J6QLJff+tOs5lmWxPiJ4O9uO4i/flC9wSJ7fPOWX2vv2j6c4P3b7st3YkGHboMO4K0Bv1yOT wv4OkcVv/bN9f0v7VDxuYPlhEpDraa/2Se2ATrZ49dUJSS2ifwVnK646+e/KkeT0Z4np12I6 xX0XnpEO6cpO1lNiKc5INNRiLipOBAB6VLJiKwMAAA== 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 On 2015년 10월 21일 03:20, Sergei Shtylyov wrote: > Hello. > > On 12/18/2014 12:58 AM, Sergei Shtylyov wrote: > >>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>> hence >>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>> GPIO, which should be high for normal operation. >> >>>> Signed-off-by: Sergei Shtylyov >> >>>> --- >>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >> >>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>> drivers/extcon/Kconfig | 6 >>>> drivers/extcon/Makefile | 1 >>>> drivers/extcon/extcon-max3355.c | 122 >>>> ++++++++++++ >>>> 4 files changed, 150 insertions(+) >> >>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> =================================================================== >>>> --- /dev/null >>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> @@ -0,0 +1,21 @@ >>>> +MAX3355 USB OTG chip >> >>> Need manufactor information as following : >>> -> Maxim MAX3355 >> >> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? > > You haven't replied to my questions. > >>>> +-------------------- >>>> + >>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> +device. >>>> + >>>> +Required properties: >>>> +- compatible: should be "maxim,max3355"; >>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>> GPIO pin >>>> + connected to the MAX3355's SHDN# pin; >>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>> + connected to the MAX3355's ID_OUT pin. >>>> + >>>> +Example (Koelsch board): >>>> + >>>> + usb-otg { >>>> + compatible = "maxim,max3355"; >>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >>> Kernel already supported the gpio helper function to get gpio from devicetree. >>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>> include/linux/of_gpio.h. >> >>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >> to insist on using this one... > > Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... OK. But, I recommend you use the 'gpiod' with devm_gpiod_get instead of using devm_gpio_request_one() directly as following: You can refer drivers/extcon/extcon-usb-gpio.c about using gpiod. For example, data->shdn_gpiod = devm_gpiod_get(dev, "maxim,shdn", GPIOD_IN); data->id_gpiod = devm_gpiod_get(dev, "maxim,id", GPIOD_IN); > > [...] > >>>> +static irqreturn_t max3355_id_irq(int irq, void *dev_id) >>>> +{ >>>> + struct max3355_data *data = dev_id; >>>> + int id = gpio_get_value(data->id_gpio); >>>> + >>>> + extcon_set_cable_state(data->edev, "USB-HOST", !id); >> >>> You have to get the gpio flag in the devicetree by using of_get_gpio_flags() >>> function >>> and then you would set the cable state according to gpio state and flag. >> >> I'm sorry but I just don't see why I have to do it. This is not a generic >> GPIO driver, and the polarities of the GPIOs are determined solely by the >> MAX3355 specs. > > Again, got not reply... OK. you don't need to consider the gpio flag from devicetree. > >> [...] >>>> +static int max3355_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct max3355_data *data; >>>> + int gpio, irq, ret; >>>> + >>>> + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), >>>> + GFP_KERNEL); >>>> + if (!data) >>>> + return -ENOMEM; >>>> + >>>> + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); >>>> + if (IS_ERR(data->edev)) { >>>> + dev_err(&pdev->dev, "failed to allocate extcon device\n"); >>>> + return PTR_ERR(data->edev); >>>> + } >>>> + data->edev->name = kstrdup(np->name, GFP_KERNEL); >>>> + >>>> + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); >> >>> Use of_get_gpio() or of_get_gpio_flags(). >> >> OK, I'll use the first one. > > No, I won't due to not being able to use "gpios" anymore. OK. I add the comment on upper. You can use the gpiod API. > > [...] > >>>> + >>>> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, >>>> + IRQF_TRIGGER_RISING | >>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> >>> Is it right to add both RISING and FALLING trigger? >> >> How else I'm supposed to know when the OTG ID signal goes low and high? >> >>> and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt >>> happen. >> >> Hm, are you sure we want to wake up on plugging another kind of USB cable? > > No reply to that as well... I'm sure. If external connector (USB, Charger cable, Earjack etc..) is attached or detached in the suspended state, system should be wake-up from suspended state. If kernel don't handle the any interrupt in suspended state, after wakeup from suspended state, there is mismatch data between 'before entering suspend state' and 'after wakeup from suspend state'. > > [...] >>>> +static struct platform_driver max3355_driver = { >>>> + .probe = max3355_probe, >>>> + .driver = { >>>> + .name = "extcon-max3355", >>>> + .of_match_table = max3355_match_table, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(max3355_driver); >>>> + >> >>> Don't need un-used blank line. >> >> I don't understand. There's empty line in such case in extcon-gpio.c... > > No reply... OK. No problem. Ignore this comment. Thanks, Chanwoo Choi