From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754126AbbCPNL1 (ORCPT ); Mon, 16 Mar 2015 09:11:27 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:49914 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751799AbbCPNLZ (ORCPT ); Mon, 16 Mar 2015 09:11:25 -0400 Message-ID: <5506D675.8010006@ti.com> Date: Mon, 16 Mar 2015 15:11:17 +0200 From: Roger Quadros User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: "Ivan T. Ivanov" CC: , , , , , , , , Subject: Re: [PATCH v4 1/1] extcon: usb-gpio: Introduce gpio usb extcon driver References: <1422274532-9488-1-git-send-email-rogerq@ti.com> <1422274532-9488-2-git-send-email-rogerq@ti.com> <54C8D2F4.9050404@ti.com> <54CF4FC7.2050309@ti.com> <1426509149.2330.5.camel@linaro.org> In-Reply-To: <1426509149.2330.5.camel@linaro.org> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ivan, On 16/03/15 14:32, Ivan T. Ivanov wrote: > Hi, > > On Mon, 2015-02-02 at 12:21 +0200, Roger Quadros wrote: >> This driver observes the USB ID pin connected over a GPIO and >> updates the USB cable extcon states accordingly. >> >> The existing GPIO extcon driver is not suitable for this purpose >> as it needs to be taught to understand USB cable states and it >> can't handle more than one cable per instance. >> >> For the USB case we need to handle 2 cable states. >> 1) USB (attach/detach) >> 2) USB-HOST (attach/detach) >> >> This driver can be easily updated in the future to handle VBUS >> events in case it happens to be available on GPIO for any platform. >> >> Signed-off-by: Roger Quadros >> --- >> v4: >> - got rid of id_irqwake flag. Fail if enable/disable_irq_wake() fails >> - changed host cable name to "USB-HOST" > > I am sorry that I am getting a bit little late into this. > > Isn't supposed that we have to use strings defined in > const char extcon_cable_name[][]? > > >> + >> +/* List of detectable cables */ >> +enum { >> + EXTCON_CABLE_USB = 0, >> + EXTCON_CABLE_USB_HOST, >> + > > Same here: duplicated with enum extcon_cable_name > >> + EXTCON_CABLE_END, >> +}; >> + >> +static const char *usb_extcon_cable[] = { >> + [EXTCON_CABLE_USB] = "USB", >> + [EXTCON_CABLE_USB_HOST] = "USB-HOST", >> + NULL, >> +}; I'm not exactly sure how else it is supposed to work if we support only a subset of cables from the global extcon_cable_name[][]. >> > > > >> + >> +static int usb_extcon_probe(struct platform_device *pdev) >> +{ >> > > > >> + >> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >> + usb_irq_handler, >> + IRQF_TRIGGER_RISING | >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > Shouldn't triggers be defined in DTS files? Could be but we're sure that we always need the trigger for both rising/falling edges in this case. So the usage is more appropriately decided from application point of view rather than h/w point of view. h/w is generic GPIO. cheers, -roger