| Message ID | 1377711185-31238-2-git-send-email-george.cherian@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi George, You didn't modify this patchset about my comment on v1 patchset. Please pay attention to comment. On 08/29/2013 02:33 AM, George Cherian wrote: > Add a generic USB VBUS/ID detection EXTCON driver. This driver expects > the ID/VBUS pin are connected via GPIOs. This driver is tested on > DRA7x board were the ID pin is routed via GPIOs. The driver supports > both VBUS and ID pin configuration and ID pin only configuration. > > Signed-off-by: George Cherian <george.cherian@ti.com> > --- > .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ > drivers/extcon/Kconfig | 6 + > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-gpio-usbvid.c | 286 +++++++++++++++++++++ You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. Also, you should change the file name of extcon-gpio-usbvid.txt. Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c. It has caused the confusion that user would think extcon-gpio-usbvid.c driver support all of extcon driver using gpio irq pin. So I'd like you to use proper prefix including device name. > 4 files changed, 313 insertions(+) > create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt > create mode 100644 drivers/extcon/extcon-gpio-usbvid.c > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt > new file mode 100644 > index 0000000..eea0741 > --- /dev/null > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt > @@ -0,0 +1,20 @@ > +EXTCON FOR USB VIA GPIO > + > +Required Properties: > + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector > + using gpios or "ti,gpio-usb-id" for USB ID pin detector > + - gpios : phandle and args ID pin gpio and VBUS gpio. > + The first gpio used for ID pin detection > + and the second used for VBUS detection. > + ID pin gpio is mandatory and VBUS is optional > + depending on implementation. > + > +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings > + > +Example: > + > + gpio_usbvid_extcon1 { > + compatible = "ti,gpio-usb-vid"; > + gpios = <&gpio1 1 0>, > + <&gpio2 2 0>; > + }; > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index f1d54a3..8097398 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -64,4 +64,10 @@ config EXTCON_PALMAS > Say Y here to enable support for USB peripheral and USB host > detection by palmas usb. > > +config EXTCON_GPIO_USBVID > + tristate "Generic USB VBUS/ID detection using GPIO EXTCON support" > + help > + Say Y here to enable support for USB VBUS/ID deetction by GPIO. > + > + Remove blank line. > endif # MULTISTATE_SWITCH > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index e4fa8ba..0451f698 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o > obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o > +obj-$(CONFIG_EXTCON_GPIO_USBVID) += extcon-gpio-usbvid.o > diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c > new file mode 100644 > index 0000000..e9bc2a97 > --- /dev/null > +++ b/drivers/extcon/extcon-gpio-usbvid.c > @@ -0,0 +1,286 @@ > +/* > + * Generic USB VBUS-ID pin detection driver > + * > + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * Author: George Cherian <george.cherian@ti.com> > + * > + * Based on extcon-palmas.c > + * > + * Author: Kishon Vijay Abraham I <kishon@ti.com> > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/kthread.h> > +#include <linux/freezer.h> kthead.h, freezer.h headerfile is used in this file? > +#include <linux/platform_device.h> > +#include <linux/extcon.h> > +#include <linux/err.h> > +#include <linux/of.h> > +#include <linux/gpio.h> > +#include <linux/of_gpio.h> > +#include <linux/of_platform.h> Order headerfile alphabetically. > + > +struct gpio_usbvid { > + struct device *dev; > + > + struct extcon_dev edev; > + > + /*GPIO pin */ I commented previous your patch about this wrong coding style. Why did not you fix this coding style? > + int id_gpio; > + int vbus_gpio; > + > + int id_irq; > + int vbus_irq; > + int type; > +}; > + > +static const char *dra7xx_extcon_cable[] = { > + [0] = "USB", > + [1] = "USB-HOST", > + NULL, > +}; > + > +static const int mutually_exclusive[] = {0x3, 0x0}; > + > +/* Two types of support are provided. > + * Systems which has > + * 1) VBUS and ID pin connected via GPIO > + * 2) only ID pin connected via GPIO Remove blank between '2)' and 'only'. > + * For Case 1 both the gpios should be provided via DT > + * Always the first GPIO in dt is considered ID pin GPIO > + */ > + > +enum { > + UNKNOWN = 0, > + ID_DETECT, > + VBUS_ID_DETECT, > +}; > + > +#define ID_GND 0 > +#define ID_FLOAT 1 > +#define VBUS_OFF 0 > +#define VBUS_ON 1 I think you could only use two constant instead of four constant definition. > + > + This blank line isn't necessary. > +static irqreturn_t id_irq_handler(int irq, void *data) > +{ > + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; You should delete blank between ')' and 'data' as follwong: - (struct gpio_usbvid *)data; > + int id_current; > + > + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); > + if (id_current == ID_GND) { > + if (gpio_usbvid->type == ID_DETECT) > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", false); > + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); As else statement, you should set "USB-HOST" cable state to improve readability. extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); if (gpio_usbvid->type == ID_DETECT) extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); > + } else { > + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); > + if (gpio_usbvid->type == ID_DETECT) > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", true); > + } Add blank line. > + return IRQ_HANDLED; > +} > + > +static irqreturn_t vbus_irq_handler(int irq, void *data) > +{ > + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; ditto. > + int vbus_current; > + > + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); > + if (vbus_current == VBUS_OFF) > + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); > + else > + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); > + > + return IRQ_HANDLED; > +} > + > + This blank line isn't necessary. I commented unnecessary blank line on previous review. > +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) > +{ > + int id_current; > + int vbus_current; Define loacal variable on one line as following: int id_current, vbus_current; > + > + switch (gpio_usbvid->type) { > + case ID_DETECT: > + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); > + if (!!id_current == ID_FLOAT) { > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB-HOST", false); > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", true); > + } else { > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", false); > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB-HOST", true); > + } > + break; > + > + case VBUS_ID_DETECT: > + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); > + if (!!id_current == ID_FLOAT) > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB-HOST", false); > + else > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB-HOST", true); > + > + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); > + if (!!vbus_current == VBUS_ON) > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", true); > + else > + extcon_set_cable_state(&gpio_usbvid->edev, > + "USB", false); > + break; > + > + default: > + dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n"); > + } > +} > + > +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid) > +{ > + int ret; Add blank line. > + ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq, > + NULL, id_irq_handler, > + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, > + dev_name(gpio_usbvid->dev), > + (void *) gpio_usbvid); > + if (ret) { > + dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n", > + gpio_usbvid->id_irq); > + return ret; > + } > + if (gpio_usbvid->type == VBUS_ID_DETECT) { > + ret = devm_request_threaded_irq(gpio_usbvid->dev, > + gpio_usbvid->vbus_irq, NULL, > + vbus_irq_handler, > + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, > + dev_name(gpio_usbvid->dev), Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq? You should use characteristic interrupt name. > + (void *) gpio_usbvid); > + if (ret) > + dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n", > + gpio_usbvid->vbus_irq); > + } Add blank line. > + return ret; > +} > + > +static int gpio_usbvid_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct gpio_usbvid *gpio_usbvid; > + int ret; > + int gpio; Define loacal variable on one line as following: int ret, gpio; > + > + gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), > + GFP_KERNEL); If this statement over 80 line, you have to keep proper indentation as following: gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), GFP_KERNEL); > + if (!gpio_usbvid) > + return -ENOMEM; > + > + Remove blank line. > + gpio_usbvid->dev = &pdev->dev; Use space instead of tab. > + > + platform_set_drvdata(pdev, gpio_usbvid); > + > + gpio_usbvid->edev.name = dev_name(&pdev->dev); I add as follwong comment about your v1 patchset "If edev name is equal with device name, this line is unnecessary. Because extcon_dev_register() use dev_name(&pdev->dev) as edev name in extcon-class.c" Why did not apply for my comment to v3 patchset? Plesae pay attention for previous comment. > + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; > + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; > + > + if (of_device_is_compatible(node, "ti,gpio-usb-id")) > + gpio_usbvid->type = ID_DETECT; > + > + gpio = of_get_gpio(node, 0); > + if (gpio_is_valid(gpio)) { > + gpio_usbvid->id_gpio = gpio; > + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, > + "id_gpio"); > + if (ret) > + return ret; Add blank line. > + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); > + } else { > + dev_err(&pdev->dev, "failed to get id gpio\n"); > + return -ENODEV; > + } > + > + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { > + gpio_usbvid->type = VBUS_ID_DETECT; > + gpio = of_get_gpio(node, 1); > + if (gpio_is_valid(gpio)) { > + gpio_usbvid->vbus_gpio = gpio; > + ret = devm_gpio_request(&pdev->dev, > + gpio_usbvid->vbus_gpio, > + "vbus_gpio"); > + if (ret) > + return ret; Add blank line. > + gpio_usbvid->vbus_irq = > + gpio_to_irq(gpio_usbvid->vbus_gpio); > + } else { > + dev_err(&pdev->dev, "failed to get vbus gpio\n"); > + return -ENODEV; > + } > + } > + > + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); > + if (ret) { > + dev_err(&pdev->dev, "failed to register extcon device\n"); > + return ret; > + } > + > + gpio_usbvid_set_initial_state(gpio_usbvid); > + ret = gpio_usbvid_request_irq(gpio_usbvid); You should move gpio_usbvid_request_irq() call before extcon_dev_register(). > + if (ret) > + goto err0; ? As following previous comment about v1 patchset: I need correct meaning name as err_thread or etc ... > + > + return 0; > + > +err0: ditto. > + extcon_dev_unregister(&gpio_usbvid->edev); > + > + return ret; > +} > + > +static int gpio_usbvid_remove(struct platform_device *pdev) > +{ > + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); > + > + extcon_dev_unregister(&gpio_usbvid->edev); > + return 0; > +} > + > +static struct of_device_id of_gpio_usbvid_match_tbl[] = { > + { .compatible = "ti,gpio-usb-vid", }, > + { .compatible = "ti,gpio-usb-id", }, > + { /* end */ } > +}; > + > +static struct platform_driver gpio_usbvid_driver = { > + .probe = gpio_usbvid_probe, > + .remove = gpio_usbvid_remove, > + .driver = { > + .name = "gpio-usbvid", > + .of_match_table = of_gpio_usbvid_match_tbl, > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(gpio_usbvid_driver); > + > +MODULE_ALIAS("platform:gpio-usbvid"); > +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); > +MODULE_DESCRIPTION("GPIO based USB Connector driver"); > +MODULE_LICENSE("GPL"); > +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl); > Cheers, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo, Thanks for the review and sorry for all the trivial mistakes. On 8/29/2013 7:05 AM, Chanwoo Choi wrote: > Hi George, > > You didn't modify this patchset about my comment on v1 patchset. > Please pay attention to comment. > > On 08/29/2013 02:33 AM, George Cherian wrote: >> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects >> the ID/VBUS pin are connected via GPIOs. This driver is tested on >> DRA7x board were the ID pin is routed via GPIOs. The driver supports >> both VBUS and ID pin configuration and ID pin only configuration. >> >> Signed-off-by: George Cherian <george.cherian@ti.com> >> --- >> .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ >> drivers/extcon/Kconfig | 6 + >> drivers/extcon/Makefile | 1 + >> drivers/extcon/extcon-gpio-usbvid.c | 286 +++++++++++++++++++++ > You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. > - extcon-[device name].c > - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. > > Also, you should change the file name of extcon-gpio-usbvid.txt. > > Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c. > It has caused the confusion that user would think extcon-gpio-usbvid.c driver > support all of extcon driver using gpio irq pin. So I'd like you to use > proper prefix including device name. I meant to support all of extcon driver using gpio for USB VBUS/ID detection. > >> 4 files changed, 313 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt >> create mode 100644 drivers/extcon/extcon-gpio-usbvid.c [snip] >> index f1d54a3..8097398 100644 >> --- a/drivers/extcon/Kconfig >> +++ b/drivers/extcon/Kconfig >> @@ -64,4 +64,10 @@ config EXTCON_PALMAS >> Say Y here to enable support for USB peripheral and USB host >> detection by palmas usb. >> >> +config EXTCON_GPIO_USBVID >> + tristate "Generic USB VBUS/ID detection using GPIO EXTCON support" >> + help >> + Say Y here to enable support for USB VBUS/ID deetction by GPIO. >> + >> + > Remove blank line. okay >> endif # MULTISTATE_SWITCH [snip] >> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c >> new file mode 100644 >> index 0000000..e9bc2a97 >> --- /dev/null >> +++ b/drivers/extcon/extcon-gpio-usbvid.c >> @@ -0,0 +1,286 @@ >> +/* >> + * Generic USB VBUS-ID pin detection driver >> + * >> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * Author: George Cherian <george.cherian@ti.com> >> + * >> + * Based on extcon-palmas.c >> + * >> + * Author: Kishon Vijay Abraham I <kishon@ti.com> >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/interrupt.h> >> +#include <linux/kthread.h> >> +#include <linux/freezer.h> > kthead.h, freezer.h headerfile is used in this file? okay >> +#include <linux/platform_device.h> >> +#include <linux/extcon.h> >> +#include <linux/err.h> >> +#include <linux/of.h> >> +#include <linux/gpio.h> >> +#include <linux/of_gpio.h> >> +#include <linux/of_platform.h> > Order headerfile alphabetically. okay > >> + >> +struct gpio_usbvid { >> + struct device *dev; >> + >> + struct extcon_dev edev; >> + >> + /*GPIO pin */ > I commented previous your patch about this wrong coding style. > Why did not you fix this coding style? okay >> + int id_gpio; >> + int vbus_gpio; >> + >> + int id_irq; >> + int vbus_irq; >> + int type; >> +}; >> + >> +static const char *dra7xx_extcon_cable[] = { >> + [0] = "USB", >> + [1] = "USB-HOST", >> + NULL, >> +}; >> + >> +static const int mutually_exclusive[] = {0x3, 0x0}; >> + >> +/* Two types of support are provided. >> + * Systems which has >> + * 1) VBUS and ID pin connected via GPIO >> + * 2) only ID pin connected via GPIO > Remove blank between '2)' and 'only'. okay >> + * For Case 1 both the gpios should be provided via DT >> + * Always the first GPIO in dt is considered ID pin GPIO >> + */ >> + >> +enum { >> + UNKNOWN = 0, >> + ID_DETECT, >> + VBUS_ID_DETECT, >> +}; >> + >> +#define ID_GND 0 >> +#define ID_FLOAT 1 >> +#define VBUS_OFF 0 >> +#define VBUS_ON 1 > I think you could only use two constant instead of four constant definition. you mean only ID_GND and VBUS_OFF? >> + >> + > This blank line isn't necessary. > >> +static irqreturn_t id_irq_handler(int irq, void *data) >> +{ >> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; > You should delete blank between ')' and 'data' as follwong: > - (struct gpio_usbvid *)data; okay > >> + int id_current; >> + >> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >> + if (id_current == ID_GND) { >> + if (gpio_usbvid->type == ID_DETECT) >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", false); >> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > As else statement, you should set "USB-HOST" cable state to improve readability. > > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > if (gpio_usbvid->type == ID_DETECT) > extcon_set_cable_state(&gpio_usbvid->edev, > "USB", false); Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or VBUS and ID. >> + } else { >> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >> + if (gpio_usbvid->type == ID_DETECT) >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", true); >> + } > Add blank line. okay >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t vbus_irq_handler(int irq, void *data) >> +{ >> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; > ditto. okay >> + int vbus_current; >> + >> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >> + if (vbus_current == VBUS_OFF) >> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >> + else >> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >> + >> + return IRQ_HANDLED; >> +} >> + >> + > This blank line isn't necessary. > I commented unnecessary blank line on previous review. okay >> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >> +{ >> + int id_current; >> + int vbus_current; > Define loacal variable on one line as following: > int id_current, vbus_current; okay >> + >> + switch (gpio_usbvid->type) { >> + case ID_DETECT: >> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >> + if (!!id_current == ID_FLOAT) { >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB-HOST", false); >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", true); >> + } else { >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", false); >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB-HOST", true); >> + } >> + break; >> + >> + case VBUS_ID_DETECT: >> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >> + if (!!id_current == ID_FLOAT) >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB-HOST", false); >> + else >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB-HOST", true); >> + >> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >> + if (!!vbus_current == VBUS_ON) >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", true); >> + else >> + extcon_set_cable_state(&gpio_usbvid->edev, >> + "USB", false); >> + break; >> + >> + default: >> + dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n"); >> + } >> +} >> + >> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid) >> +{ >> + int ret; > Add blank line. okay >> + ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq, >> + NULL, id_irq_handler, >> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, >> + dev_name(gpio_usbvid->dev), >> + (void *) gpio_usbvid); >> + if (ret) { >> + dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n", >> + gpio_usbvid->id_irq); >> + return ret; >> + } >> + if (gpio_usbvid->type == VBUS_ID_DETECT) { >> + ret = devm_request_threaded_irq(gpio_usbvid->dev, >> + gpio_usbvid->vbus_irq, NULL, >> + vbus_irq_handler, >> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, >> + dev_name(gpio_usbvid->dev), > Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq? > You should use characteristic interrupt name. if I use dev_name it gives me 2 same name interrupts associated with a single extcon device. Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name since there will be 2 instances of extcon being used as of now. >> + (void *) gpio_usbvid); >> + if (ret) >> + dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n", >> + gpio_usbvid->vbus_irq); >> + } > Add blank line. okay >> + return ret; >> +} >> + >> +static int gpio_usbvid_probe(struct platform_device *pdev) >> +{ >> + struct device_node *node = pdev->dev.of_node; >> + struct gpio_usbvid *gpio_usbvid; >> + int ret; >> + int gpio; > Define loacal variable on one line as following: > int ret, gpio; > >> + >> + gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), >> + GFP_KERNEL); > If this statement over 80 line, you have to keep proper indentation as following: > gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), > GFP_KERNEL); okay > >> + if (!gpio_usbvid) >> + return -ENOMEM; >> + >> + > Remove blank line. okay >> + gpio_usbvid->dev = &pdev->dev; > Use space instead of tab. okay >> + >> + platform_set_drvdata(pdev, gpio_usbvid); >> + >> + gpio_usbvid->edev.name = dev_name(&pdev->dev); > I add as follwong comment about your v1 patchset > > "If edev name is equal with device name, this line is unnecessary. > Because extcon_dev_register() use dev_name(&pdev->dev) as edev name > in extcon-class.c" > > Why did not apply for my comment to v3 patchset? > Plesae pay attention for previous comment. I removed it but it gave me a NULL pointer dereference in extcon_get_extcon_dev (strcmp the sd->name was NULL). I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check. >> + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; >> + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; >> + >> + if (of_device_is_compatible(node, "ti,gpio-usb-id")) >> + gpio_usbvid->type = ID_DETECT; >> + >> + gpio = of_get_gpio(node, 0); >> + if (gpio_is_valid(gpio)) { >> + gpio_usbvid->id_gpio = gpio; >> + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, >> + "id_gpio"); >> + if (ret) >> + return ret; > Add blank line. > >> + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); >> + } else { >> + dev_err(&pdev->dev, "failed to get id gpio\n"); >> + return -ENODEV; >> + } >> + >> + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { >> + gpio_usbvid->type = VBUS_ID_DETECT; >> + gpio = of_get_gpio(node, 1); >> + if (gpio_is_valid(gpio)) { >> + gpio_usbvid->vbus_gpio = gpio; >> + ret = devm_gpio_request(&pdev->dev, >> + gpio_usbvid->vbus_gpio, >> + "vbus_gpio"); >> + if (ret) >> + return ret; > Add blank line. > >> + gpio_usbvid->vbus_irq = >> + gpio_to_irq(gpio_usbvid->vbus_gpio); >> + } else { >> + dev_err(&pdev->dev, "failed to get vbus gpio\n"); >> + return -ENODEV; >> + } >> + } >> + >> + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register extcon device\n"); >> + return ret; >> + } >> + >> + gpio_usbvid_set_initial_state(gpio_usbvid); >> + ret = gpio_usbvid_request_irq(gpio_usbvid); > You should move gpio_usbvid_request_irq() call before extcon_dev_register(). > >> + if (ret) >> + goto err0; > ? As following previous comment about v1 patchset: > I need correct meaning name as err_thread or etc ... okay > >> + >> + return 0; >> + >> +err0: > ditto. okay >> + extcon_dev_unregister(&gpio_usbvid->edev); >> + >> + return ret; >> +} >> + >> +static int gpio_usbvid_remove(struct platform_device *pdev) >> +{ >> + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); >> + >> + extcon_dev_unregister(&gpio_usbvid->edev); >> + return 0; >> +} >> + >> +static struct of_device_id of_gpio_usbvid_match_tbl[] = { >> + { .compatible = "ti,gpio-usb-vid", }, >> + { .compatible = "ti,gpio-usb-id", }, >> + { /* end */ } >> +}; >> + >> +static struct platform_driver gpio_usbvid_driver = { >> + .probe = gpio_usbvid_probe, >> + .remove = gpio_usbvid_remove, >> + .driver = { >> + .name = "gpio-usbvid", >> + .of_match_table = of_gpio_usbvid_match_tbl, >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +module_platform_driver(gpio_usbvid_driver); >> + >> +MODULE_ALIAS("platform:gpio-usbvid"); >> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); >> +MODULE_DESCRIPTION("GPIO based USB Connector driver"); >> +MODULE_LICENSE("GPL"); >> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl); >> > Cheers, > Chanwoo Choi
Hi George, On 08/29/2013 11:21 AM, George Cherian wrote: > Hi Chanwoo, > > Thanks for the review and sorry for all the trivial mistakes. > > On 8/29/2013 7:05 AM, Chanwoo Choi wrote: >> Hi George, >> >> You didn't modify this patchset about my comment on v1 patchset. >> Please pay attention to comment. >> >> On 08/29/2013 02:33 AM, George Cherian wrote: >>> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects >>> the ID/VBUS pin are connected via GPIOs. This driver is tested on >>> DRA7x board were the ID pin is routed via GPIOs. The driver supports >>> both VBUS and ID pin configuration and ID pin only configuration. >>> >>> Signed-off-by: George Cherian <george.cherian@ti.com> >>> --- >>> .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ >>> drivers/extcon/Kconfig | 6 + >>> drivers/extcon/Makefile | 1 + >>> drivers/extcon/extcon-gpio-usbvid.c | 286 +++++++++++++++++++++ >> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. >> - extcon-[device name].c >> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. > Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. > It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic > gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion > with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. Second, gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices would not control both "USB-HOST" and "USB" cable state at same time. Other extcon devices would support either "USB-HOST" or "USB" cable. Third, gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. In result, id_irq_handler() would control both "USB-HOST" and "USB" cable state. vbus_irq_handler() would control only "USB" cable state. Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state according to each gpio state at same time. Also, It include critical problem. >> >> Also, you should change the file name of extcon-gpio-usbvid.txt. >> >> Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c. >> It has caused the confusion that user would think extcon-gpio-usbvid.c driver >> support all of extcon driver using gpio irq pin. So I'd like you to use >> proper prefix including device name. > I meant to support all of extcon driver using gpio for USB VBUS/ID detection. >> >>> 4 files changed, 313 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt >>> create mode 100644 drivers/extcon/extcon-gpio-usbvid.c > [snip] >>> index f1d54a3..8097398 100644 >>> --- a/drivers/extcon/Kconfig >>> +++ b/drivers/extcon/Kconfig >>> @@ -64,4 +64,10 @@ config EXTCON_PALMAS >>> Say Y here to enable support for USB peripheral and USB host >>> detection by palmas usb. >>> +config EXTCON_GPIO_USBVID >>> + tristate "Generic USB VBUS/ID detection using GPIO EXTCON support" >>> + help >>> + Say Y here to enable support for USB VBUS/ID deetction by GPIO. >>> + >>> + >> Remove blank line. > okay >>> endif # MULTISTATE_SWITCH > [snip] >>> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c >>> new file mode 100644 >>> index 0000000..e9bc2a97 >>> --- /dev/null >>> +++ b/drivers/extcon/extcon-gpio-usbvid.c >>> @@ -0,0 +1,286 @@ >>> +/* >>> + * Generic USB VBUS-ID pin detection driver >>> + * >>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * Author: George Cherian <george.cherian@ti.com> >>> + * >>> + * Based on extcon-palmas.c >>> + * >>> + * Author: Kishon Vijay Abraham I <kishon@ti.com> >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + */ >>> + >>> +#include <linux/module.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/kthread.h> >>> +#include <linux/freezer.h> >> kthead.h, freezer.h headerfile is used in this file? > okay >>> +#include <linux/platform_device.h> >>> +#include <linux/extcon.h> >>> +#include <linux/err.h> >>> +#include <linux/of.h> >>> +#include <linux/gpio.h> >>> +#include <linux/of_gpio.h> >>> +#include <linux/of_platform.h> >> Order headerfile alphabetically. > okay >> >>> + >>> +struct gpio_usbvid { >>> + struct device *dev; >>> + >>> + struct extcon_dev edev; >>> + >>> + /*GPIO pin */ >> I commented previous your patch about this wrong coding style. >> Why did not you fix this coding style? > okay >>> + int id_gpio; >>> + int vbus_gpio; >>> + >>> + int id_irq; >>> + int vbus_irq; >>> + int type; >>> +}; >>> + >>> +static const char *dra7xx_extcon_cable[] = { >>> + [0] = "USB", >>> + [1] = "USB-HOST", >>> + NULL, >>> +}; >>> + >>> +static const int mutually_exclusive[] = {0x3, 0x0}; >>> + >>> +/* Two types of support are provided. >>> + * Systems which has >>> + * 1) VBUS and ID pin connected via GPIO >>> + * 2) only ID pin connected via GPIO >> Remove blank between '2)' and 'only'. > okay >>> + * For Case 1 both the gpios should be provided via DT >>> + * Always the first GPIO in dt is considered ID pin GPIO >>> + */ >>> + >>> +enum { >>> + UNKNOWN = 0, >>> + ID_DETECT, >>> + VBUS_ID_DETECT, >>> +}; >>> + >>> +#define ID_GND 0 >>> +#define ID_FLOAT 1 >>> +#define VBUS_OFF 0 >>> +#define VBUS_ON 1 >> I think you could only use two constant instead of four constant definition. > you mean only ID_GND and VBUS_OFF? you could only define two contant (0 and 1) to detect gpio state. >>> + >>> + >> This blank line isn't necessary. >> >>> +static irqreturn_t id_irq_handler(int irq, void *data) >>> +{ >>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >> You should delete blank between ')' and 'data' as follwong: >> - (struct gpio_usbvid *)data; > okay >> >>> + int id_current; >>> + >>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> + if (id_current == ID_GND) { >>> + if (gpio_usbvid->type == ID_DETECT) >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", false); >>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> As else statement, you should set "USB-HOST" cable state to improve readability. >> >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> if (gpio_usbvid->type == ID_DETECT) >> extcon_set_cable_state(&gpio_usbvid->edev, >> "USB", false); > Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio > and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or > VBUS and ID. I don't understand. Wht does not you change the order of function call as following? Before: if (gpio_usbvid->type == ID_DETECT) extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); After: extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); if (gpio_usbvid->type == ID_DETECT) extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value. And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value. >>> + } else { >>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>> + if (gpio_usbvid->type == ID_DETECT) >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", true); >>> + } >> Add blank line. > okay >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static irqreturn_t vbus_irq_handler(int irq, void *data) >>> +{ >>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >> ditto. > okay >>> + int vbus_current; >>> + >>> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> + if (vbus_current == VBUS_OFF) >>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>> + else >>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); First, vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> + >> This blank line isn't necessary. >> I commented unnecessary blank line on previous review. > okay >>> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >>> +{ >>> + int id_current; >>> + int vbus_current; >> Define loacal variable on one line as following: >> int id_current, vbus_current; > okay >>> + >>> + switch (gpio_usbvid->type) { >>> + case ID_DETECT: >>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> + if (!!id_current == ID_FLOAT) { >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", false); >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", true); >>> + } else { >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", false); >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", true); >>> + } Second, gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices would not control both "USB-HOST" and "USB" cable state at same time. Other extcon devices would support either "USB-HOST" or "USB" cable. >>> + break; >>> + >>> + case VBUS_ID_DETECT: >>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> + if (!!id_current == ID_FLOAT) >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", false); >>> + else >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", true); >>> + >>> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> + if (!!vbus_current == VBUS_ON) >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", true); >>> + else >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", false); >>> + break; >>> + >>> + default: >>> + dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n"); >>> + } >>> +} >>> + >>> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid) >>> +{ >>> + int ret; >> Add blank line. > okay >>> + ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq, >>> + NULL, id_irq_handler, >>> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, >>> + dev_name(gpio_usbvid->dev), >>> + (void *) gpio_usbvid); >>> + if (ret) { >>> + dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n", >>> + gpio_usbvid->id_irq); >>> + return ret; >>> + } >>> + if (gpio_usbvid->type == VBUS_ID_DETECT) { >>> + ret = devm_request_threaded_irq(gpio_usbvid->dev, >>> + gpio_usbvid->vbus_irq, NULL, >>> + vbus_irq_handler, >>> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, >>> + dev_name(gpio_usbvid->dev), >> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq? >> You should use characteristic interrupt name. > if I use dev_name it gives me 2 same name interrupts associated with a single extcon device. > Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name > since there will be 2 instances of extcon being used as of now. I can't agree. Single extcon driver can have various interrupt. If you use same interrupt name about different two interrupt, can we know the kind of interrupt which is happened on /proc/interrupts? We cannot count the number of each interrupt occurrences. >>> + (void *) gpio_usbvid); >>> + if (ret) >>> + dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n", >>> + gpio_usbvid->vbus_irq); >>> + } >> Add blank line. > okay >>> + return ret; >>> +} >>> + >>> +static int gpio_usbvid_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *node = pdev->dev.of_node; >>> + struct gpio_usbvid *gpio_usbvid; >>> + int ret; >>> + int gpio; >> Define loacal variable on one line as following: >> int ret, gpio; >> >>> + >>> + gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), >>> + GFP_KERNEL); >> If this statement over 80 line, you have to keep proper indentation as following: >> gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), >> GFP_KERNEL); > okay >> >>> + if (!gpio_usbvid) >>> + return -ENOMEM; >>> + >>> + >> Remove blank line. > okay >>> + gpio_usbvid->dev = &pdev->dev; >> Use space instead of tab. > okay >>> + >>> + platform_set_drvdata(pdev, gpio_usbvid); >>> + >>> + gpio_usbvid->edev.name = dev_name(&pdev->dev); >> I add as follwong comment about your v1 patchset >> >> "If edev name is equal with device name, this line is unnecessary. >> Because extcon_dev_register() use dev_name(&pdev->dev) as edev name >> in extcon-class.c" >> >> Why did not apply for my comment to v3 patchset? >> Plesae pay attention for previous comment. > I removed it but it gave me a NULL pointer dereference in extcon_get_extcon_dev (strcmp the sd->name was NULL). > I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check. Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content. This feature related to your NULL pointer issue will include v3.12. - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096 >>> + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; >>> + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; >>> + >>> + if (of_device_is_compatible(node, "ti,gpio-usb-id")) >>> + gpio_usbvid->type = ID_DETECT; >>> + >>> + gpio = of_get_gpio(node, 0); >>> + if (gpio_is_valid(gpio)) { >>> + gpio_usbvid->id_gpio = gpio; >>> + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, >>> + "id_gpio"); >>> + if (ret) >>> + return ret; >> Add blank line. >> >>> + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); >>> + } else { >>> + dev_err(&pdev->dev, "failed to get id gpio\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { >>> + gpio_usbvid->type = VBUS_ID_DETECT; >>> + gpio = of_get_gpio(node, 1); >>> + if (gpio_is_valid(gpio)) { >>> + gpio_usbvid->vbus_gpio = gpio; >>> + ret = devm_gpio_request(&pdev->dev, >>> + gpio_usbvid->vbus_gpio, >>> + "vbus_gpio"); >>> + if (ret) >>> + return ret; >> Add blank line. >> >>> + gpio_usbvid->vbus_irq = >>> + gpio_to_irq(gpio_usbvid->vbus_gpio); >>> + } else { >>> + dev_err(&pdev->dev, "failed to get vbus gpio\n"); >>> + return -ENODEV; >>> + } >>> + } >>> + >>> + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); >>> + if (ret) { >>> + dev_err(&pdev->dev, "failed to register extcon device\n"); >>> + return ret; >>> + } >>> + >>> + gpio_usbvid_set_initial_state(gpio_usbvid); >>> + ret = gpio_usbvid_request_irq(gpio_usbvid); Third, gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. In result, id_irq_handler() would control both "USB-HOST" and "USB" cable state. vbus_irq_handler() would control only "USB" cable state. Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state according to each gpio state at same time. Also, It include critical problem. >> You should move gpio_usbvid_request_irq() call before extcon_dev_register(). >> >>> + if (ret) >>> + goto err0; >> ? As following previous comment about v1 patchset: >> I need correct meaning name as err_thread or etc ... > okay >> >>> + >>> + return 0; >>> + >>> +err0: >> ditto. > okay >>> + extcon_dev_unregister(&gpio_usbvid->edev); >>> + >>> + return ret; >>> +} >>> + >>> +static int gpio_usbvid_remove(struct platform_device *pdev) >>> +{ >>> + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); >>> + >>> + extcon_dev_unregister(&gpio_usbvid->edev); >>> + return 0; >>> +} >>> + >>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = { >>> + { .compatible = "ti,gpio-usb-vid", }, >>> + { .compatible = "ti,gpio-usb-id", }, >>> + { /* end */ } >>> +}; >>> + >>> +static struct platform_driver gpio_usbvid_driver = { >>> + .probe = gpio_usbvid_probe, >>> + .remove = gpio_usbvid_remove, >>> + .driver = { >>> + .name = "gpio-usbvid", >>> + .of_match_table = of_gpio_usbvid_match_tbl, >>> + .owner = THIS_MODULE, >>> + }, >>> +}; >>> + >>> +module_platform_driver(gpio_usbvid_driver); >>> + >>> +MODULE_ALIAS("platform:gpio-usbvid"); >>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); >>> +MODULE_DESCRIPTION("GPIO based USB Connector driver"); >>> +MODULE_LICENSE("GPL"); >>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl); >>> >> Cheers, >> Chanwoo Choi > > Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo, On 8/29/2013 11:53 AM, Chanwoo Choi wrote: [snip] > You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. > - extcon-[device name].c > - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. >> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. >> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic >> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion >> with patch v1. > Would you guarantee that this driver support all of extcon devices using gpio pin? This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. Following is the basic assumption made, correct me if I am wrong. ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral) VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. and USB is in HOST mode when ID pin is ground and VBUS is OFF. In all above cases VBUS is referred to the external VBUS supply from an external HOST. > I can't agree. This driver has specific dependency on dra7x SoC. > > First, > vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. > If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' > But, it include potential problems. Other extcon device using gpio would set USB cable state > as 'true' when gpio state is 1. This way has dependency on specific SoC. no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. so if VBUS is zero means its definitely not in connected state. > > Second, > gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time > in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices > would not control both "USB-HOST" and "USB" cable state at same time. > > Other extcon devices would support either "USB-HOST" or "USB" cable. The driver has 2 configurations. 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). So if you take type as VBUS_ID_DETECT then it is as what you meant. > > Third, > gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. > and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. > In result, > id_irq_handler() would control both "USB-HOST" and "USB" cable state. only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. > vbus_irq_handler() would control only "USB" cable state. > > Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state > according to each gpio state at same time. Also, It include critical problem. No it depends on the configuration as explained above. [snip] > + > +#define ID_GND 0 > +#define ID_FLOAT 1 > +#define VBUS_OFF 0 > +#define VBUS_ON 1 >>> I think you could only use two constant instead of four constant definition. >> you mean only ID_GND and VBUS_OFF? > you could only define two contant (0 and 1) to detect gpio state. okay > >>>> + >>>> + >>> This blank line isn't necessary. >>> >>>> +static irqreturn_t id_irq_handler(int irq, void *data) >>>> +{ >>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>> You should delete blank between ')' and 'data' as follwong: >>> - (struct gpio_usbvid *)data; >> okay >>>> + int id_current; >>>> + >>>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> + if (id_current == ID_GND) { >>>> + if (gpio_usbvid->type == ID_DETECT) >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB", false); >>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> As else statement, you should set "USB-HOST" cable state to improve readability. >>> >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> if (gpio_usbvid->type == ID_DETECT) >>> extcon_set_cable_state(&gpio_usbvid->edev, >>> "USB", false); >> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio >> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or >> VBUS and ID. > I don't understand. Wht does not you change the order of function call as following? > > Before: > if (gpio_usbvid->type == ID_DETECT) > extcon_set_cable_state(&gpio_usbvid->edev, > "USB", false); > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); Basically these notifiers would go and change the UTMI modes which is s/w controlled. so we want to gracefully exit Device mode first and then enter HOST mode. this is only with type=ID_DETECT. > After: > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > if (gpio_usbvid->type == ID_DETECT) > extcon_set_cable_state(&gpio_usbvid->edev, > "USB", false); > > id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value. > And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value. > >>>> + } else { >>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>> + if (gpio_usbvid->type == ID_DETECT) >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB", true); >>>> + } >>> Add blank line. >> okay >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static irqreturn_t vbus_irq_handler(int irq, void *data) >>>> +{ >>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>> ditto. >> okay >>>> + int vbus_current; >>>> + >>>> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>> + if (vbus_current == VBUS_OFF) >>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>> + else >>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); > First, > vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. > If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' > But, it include potential problems. Other extcon device using gpio would set USB cable state > as 'true' when gpio state is 1. This way has dependency on specific SoC. see above [snip] >>> + >>> + switch (gpio_usbvid->type) { >>> + case ID_DETECT: >>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> + if (!!id_current == ID_FLOAT) { >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", false); >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", true); >>> + } else { >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB", false); >>> + extcon_set_cable_state(&gpio_usbvid->edev, >>> + "USB-HOST", true); >>> + } > Second, > gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time > in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices > would not control both "USB-HOST" and "USB" cable state at same time. > > Other extcon devices would support either "USB-HOST" or "USB" cable. see above [snip] > Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq? > You should use characteristic interrupt name. >> if I use dev_name it gives me 2 same name interrupts associated with a single extcon device. >> Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name >> since there will be 2 instances of extcon being used as of now. > I can't agree. Single extcon driver can have various interrupt. > If you use same interrupt name about different two interrupt, > can we know the kind of interrupt which is happened on /proc/interrupts? > We cannot count the number of each interrupt occurrences. ya so i will use something like "extcon_devname + VBUS/ID" otherwise if i have 3 instance of extcon device in type VBUS_ID_DETECT then i will have 2 entries named VBUS and 2 named ID. I will change it. [snip] > + > + platform_set_drvdata(pdev, gpio_usbvid); > + > + gpio_usbvid->edev.name = dev_name(&pdev->dev); >>> I add as follwong comment about your v1 patchset >>> >>> "If edev name is equal with device name, this line is unnecessary. >>> Because extcon_dev_register() use dev_name(&pdev->dev) as edev name >>> in extcon-class.c" >>> >>> Why did not apply for my comment to v3 patchset? >>> Plesae pay attention for previous comment. >> I removed it but it gave me a NULL pointer dereference in extcon_get_extcon_dev (strcmp the sd->name was NULL). >> I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check. > Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content. > > This feature related to your NULL pointer issue will include v3.12. > - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096 guilty as charged i will rebase. >>>> + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; >>>> + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; >>>> + >>>> + if (of_device_is_compatible(node, "ti,gpio-usb-id")) >>>> + gpio_usbvid->type = ID_DETECT; >>>> + >>>> + gpio = of_get_gpio(node, 0); >>>> + if (gpio_is_valid(gpio)) { >>>> + gpio_usbvid->id_gpio = gpio; >>>> + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, >>>> + "id_gpio"); >>>> + if (ret) >>>> + return ret; >>> Add blank line. >>> >>>> + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); >>>> + } else { >>>> + dev_err(&pdev->dev, "failed to get id gpio\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { >>>> + gpio_usbvid->type = VBUS_ID_DETECT; >>>> + gpio = of_get_gpio(node, 1); >>>> + if (gpio_is_valid(gpio)) { >>>> + gpio_usbvid->vbus_gpio = gpio; >>>> + ret = devm_gpio_request(&pdev->dev, >>>> + gpio_usbvid->vbus_gpio, >>>> + "vbus_gpio"); >>>> + if (ret) >>>> + return ret; >>> Add blank line. >>> >>>> + gpio_usbvid->vbus_irq = >>>> + gpio_to_irq(gpio_usbvid->vbus_gpio); >>>> + } else { >>>> + dev_err(&pdev->dev, "failed to get vbus gpio\n"); >>>> + return -ENODEV; >>>> + } >>>> + } >>>> + >>>> + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "failed to register extcon device\n"); >>>> + return ret; >>>> + } >>>> + >>>> + gpio_usbvid_set_initial_state(gpio_usbvid); >>>> + ret = gpio_usbvid_request_irq(gpio_usbvid); > Third, > gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. > and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. > In result, > id_irq_handler() would control both "USB-HOST" and "USB" cable state. > vbus_irq_handler() would control only "USB" cable state. > > Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state > according to each gpio state at same time. Also, It include critical problem. see above >>> You should move gpio_usbvid_request_irq() call before extcon_dev_register(). >>> >>>> + if (ret) >>>> + goto err0; >>> ? As following previous comment about v1 patchset: >>> I need correct meaning name as err_thread or etc ... >> okay >>>> + >>>> + return 0; >>>> + >>>> +err0: >>> ditto. >> okay >>>> + extcon_dev_unregister(&gpio_usbvid->edev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int gpio_usbvid_remove(struct platform_device *pdev) >>>> +{ >>>> + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); >>>> + >>>> + extcon_dev_unregister(&gpio_usbvid->edev); >>>> + return 0; >>>> +} >>>> + >>>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = { >>>> + { .compatible = "ti,gpio-usb-vid", }, >>>> + { .compatible = "ti,gpio-usb-id", }, >>>> + { /* end */ } >>>> +}; >>>> + >>>> +static struct platform_driver gpio_usbvid_driver = { >>>> + .probe = gpio_usbvid_probe, >>>> + .remove = gpio_usbvid_remove, >>>> + .driver = { >>>> + .name = "gpio-usbvid", >>>> + .of_match_table = of_gpio_usbvid_match_tbl, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(gpio_usbvid_driver); >>>> + >>>> +MODULE_ALIAS("platform:gpio-usbvid"); >>>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); >>>> +MODULE_DESCRIPTION("GPIO based USB Connector driver"); >>>> +MODULE_LICENSE("GPL"); >>>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl); >>>> >>> Cheers, >>> Chanwoo Choi >> > Thanks, > Chanwoo Choi >
On 08/29/2013 04:30 PM, George Cherian wrote: > Hi Chanwoo, > > On 8/29/2013 11:53 AM, Chanwoo Choi wrote: > [snip] >> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. >> - extcon-[device name].c >> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. >>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. >>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic >>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion >>> with patch v1. >> Would you guarantee that this driver support all of extcon devices using gpio pin? > This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. > Following is the basic assumption made, correct me if I am wrong. > ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) > ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) > VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral) > VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). > > So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. > and USB is in HOST mode when ID pin is ground and VBUS is OFF. > > In all above cases VBUS is referred to the external VBUS supply from an external HOST. > >> I can't agree. This driver has specific dependency on dra7x SoC. >> >> First, >> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. >> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' >> But, it include potential problems. Other extcon device using gpio would set USB cable state >> as 'true' when gpio state is 1. This way has dependency on specific SoC. > no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. > so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. >> >> Second, >> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >> would not control both "USB-HOST" and "USB" cable state at same time. >> >> Other extcon devices would support either "USB-HOST" or "USB" cable. > The driver has 2 configurations. > 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. > 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). > So if you take type as VBUS_ID_DETECT then it is as what you meant. "2) case" don't support the detection of "USB-HOST" cable. Only detect "USB" cable according to "vbus_gpio" value. If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file? But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method. >> >> Third, >> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. >> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. >> In result, >> id_irq_handler() would control both "USB-HOST" and "USB" cable state. > only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states > if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. I have some confusion. I need additional your explanation. Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? or Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >> vbus_irq_handler() would control only "USB" cable state. >> >> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state >> according to each gpio state at same time. Also, It include critical problem. > No it depends on the configuration as explained above. > > [snip] > >> + >> +#define ID_GND 0 >> +#define ID_FLOAT 1 >> +#define VBUS_OFF 0 >> +#define VBUS_ON 1 >>>> I think you could only use two constant instead of four constant definition. >>> you mean only ID_GND and VBUS_OFF? >> you could only define two contant (0 and 1) to detect gpio state. > okay >> >>>>> + >>>>> + >>>> This blank line isn't necessary. >>>> >>>>> +static irqreturn_t id_irq_handler(int irq, void *data) >>>>> +{ >>>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>>> You should delete blank between ')' and 'data' as follwong: >>>> - (struct gpio_usbvid *)data; >>> okay >>>>> + int id_current; >>>>> + >>>>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>>> + if (id_current == ID_GND) { >>>>> + if (gpio_usbvid->type == ID_DETECT) >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>>> + "USB", false); >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> As else statement, you should set "USB-HOST" cable state to improve readability. >>>> >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> if (gpio_usbvid->type == ID_DETECT) >>>> extcon_set_cable_state(&gpio_usbvid->edev, >>>> "USB", false); >>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio >>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or >>> VBUS and ID. >> I don't understand. Wht does not you change the order of function call as following? >> >> Before: >> if (gpio_usbvid->type == ID_DETECT) >> extcon_set_cable_state(&gpio_usbvid->edev, >> "USB", false); >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > Basically these notifiers would go and change the UTMI modes which is s/w controlled. > so we want to gracefully exit Device mode first and then enter HOST mode. > this is only with type=ID_DETECT. Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, you need this setting order between "USB-HOST" and "USB" cable. Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. and can't agree as generic extcon gpio driver. >> After: >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> if (gpio_usbvid->type == ID_DETECT) >> extcon_set_cable_state(&gpio_usbvid->edev, >> "USB", false); >> >> id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value. >> And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value. >> >>>>> + } else { >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>>> + if (gpio_usbvid->type == ID_DETECT) >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>>> + "USB", true); >>>>> + } >>>> Add blank line. >>> okay >>>>> + return IRQ_HANDLED; >>>>> +} >>>>> + >>>>> +static irqreturn_t vbus_irq_handler(int irq, void *data) >>>>> +{ >>>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>>> ditto. >>> okay >>>>> + int vbus_current; >>>>> + >>>>> + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>>> + if (vbus_current == VBUS_OFF) >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>>> + else >>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >> First, >> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. >> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' >> But, it include potential problems. Other extcon device using gpio would set USB cable state >> as 'true' when gpio state is 1. This way has dependency on specific SoC. > see above > [snip] >>>> + >>>> + switch (gpio_usbvid->type) { >>>> + case ID_DETECT: >>>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> + if (!!id_current == ID_FLOAT) { >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB-HOST", false); >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB", true); >>>> + } else { >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB", false); >>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>> + "USB-HOST", true); >>>> + } >> Second, >> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >> would not control both "USB-HOST" and "USB" cable state at same time. >> >> Other extcon devices would support either "USB-HOST" or "USB" cable. > see above > [snip] >> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq? >> You should use characteristic interrupt name. >>> if I use dev_name it gives me 2 same name interrupts associated with a single extcon device. >>> Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name >>> since there will be 2 instances of extcon being used as of now. >> I can't agree. Single extcon driver can have various interrupt. >> If you use same interrupt name about different two interrupt, >> can we know the kind of interrupt which is happened on /proc/interrupts? >> We cannot count the number of each interrupt occurrences. > ya so i will use something like "extcon_devname + VBUS/ID" > otherwise if i have 3 instance of extcon device in type VBUS_ID_DETECT then i will have > 2 entries named VBUS and 2 named ID. > I will change it. > [snip] >> + >> + platform_set_drvdata(pdev, gpio_usbvid); >> + >> + gpio_usbvid->edev.name = dev_name(&pdev->dev); >>>> I add as follwong comment about your v1 patchset >>>> >>>> "If edev name is equal with device name, this line is unnecessary. >>>> Because extcon_dev_register() use dev_name(&pdev->dev) as edev name >>>> in extcon-class.c" >>>> >>>> Why did not apply for my comment to v3 patchset? >>>> Plesae pay attention for previous comment. >>> I removed it but it gave me a NULL pointer dereference in extcon_get_extcon_dev (strcmp the sd->name was NULL). >>> I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check. >> Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content. >> >> This feature related to your NULL pointer issue will include v3.12. >> - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096 > guilty as charged i will rebase. >>>>> + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; >>>>> + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; >>>>> + >>>>> + if (of_device_is_compatible(node, "ti,gpio-usb-id")) >>>>> + gpio_usbvid->type = ID_DETECT; >>>>> + >>>>> + gpio = of_get_gpio(node, 0); >>>>> + if (gpio_is_valid(gpio)) { >>>>> + gpio_usbvid->id_gpio = gpio; >>>>> + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, >>>>> + "id_gpio"); >>>>> + if (ret) >>>>> + return ret; >>>> Add blank line. >>>> >>>>> + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); >>>>> + } else { >>>>> + dev_err(&pdev->dev, "failed to get id gpio\n"); >>>>> + return -ENODEV; >>>>> + } >>>>> + >>>>> + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { >>>>> + gpio_usbvid->type = VBUS_ID_DETECT; >>>>> + gpio = of_get_gpio(node, 1); >>>>> + if (gpio_is_valid(gpio)) { >>>>> + gpio_usbvid->vbus_gpio = gpio; >>>>> + ret = devm_gpio_request(&pdev->dev, >>>>> + gpio_usbvid->vbus_gpio, >>>>> + "vbus_gpio"); >>>>> + if (ret) >>>>> + return ret; >>>> Add blank line. >>>> >>>>> + gpio_usbvid->vbus_irq = >>>>> + gpio_to_irq(gpio_usbvid->vbus_gpio); >>>>> + } else { >>>>> + dev_err(&pdev->dev, "failed to get vbus gpio\n"); >>>>> + return -ENODEV; >>>>> + } >>>>> + } >>>>> + >>>>> + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "failed to register extcon device\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + gpio_usbvid_set_initial_state(gpio_usbvid); >>>>> + ret = gpio_usbvid_request_irq(gpio_usbvid); >> Third, >> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. >> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. >> In result, >> id_irq_handler() would control both "USB-HOST" and "USB" cable state. >> vbus_irq_handler() would control only "USB" cable state. >> >> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state >> according to each gpio state at same time. Also, It include critical problem. > see above >>>> You should move gpio_usbvid_request_irq() call before extcon_dev_register(). >>>> >>>>> + if (ret) >>>>> + goto err0; >>>> ? As following previous comment about v1 patchset: >>>> I need correct meaning name as err_thread or etc ... >>> okay >>>>> + >>>>> + return 0; >>>>> + >>>>> +err0: >>>> ditto. >>> okay >>>>> + extcon_dev_unregister(&gpio_usbvid->edev); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int gpio_usbvid_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); >>>>> + >>>>> + extcon_dev_unregister(&gpio_usbvid->edev); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = { >>>>> + { .compatible = "ti,gpio-usb-vid", }, >>>>> + { .compatible = "ti,gpio-usb-id", }, >>>>> + { /* end */ } >>>>> +}; >>>>> + >>>>> +static struct platform_driver gpio_usbvid_driver = { >>>>> + .probe = gpio_usbvid_probe, >>>>> + .remove = gpio_usbvid_remove, >>>>> + .driver = { >>>>> + .name = "gpio-usbvid", >>>>> + .of_match_table = of_gpio_usbvid_match_tbl, >>>>> + .owner = THIS_MODULE, >>>>> + }, >>>>> +}; >>>>> + >>>>> +module_platform_driver(gpio_usbvid_driver); >>>>> + >>>>> +MODULE_ALIAS("platform:gpio-usbvid"); >>>>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); >>>>> +MODULE_DESCRIPTION("GPIO based USB Connector driver"); >>>>> +MODULE_LICENSE("GPL"); >>>>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl); >>>>> >>>> Cheers, >>>> Chanwoo Choi >>> >> Thanks, >> Chanwoo Choi >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/29/2013 4:07 PM, Chanwoo Choi wrote: > On 08/29/2013 04:30 PM, George Cherian wrote: >> Hi Chanwoo, >> >> On 8/29/2013 11:53 AM, Chanwoo Choi wrote: >> [snip] >>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. >>> - extcon-[device name].c >>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. >>>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. >>>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic >>>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion >>>> with patch v1. >>> Would you guarantee that this driver support all of extcon devices using gpio pin? >> This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. >> Following is the basic assumption made, correct me if I am wrong. >> ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) >> ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) >> VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral) >> VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). >> >> So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. >> and USB is in HOST mode when ID pin is ground and VBUS is OFF. >> >> In all above cases VBUS is referred to the external VBUS supply from an external HOST. >> >>> I can't agree. This driver has specific dependency on dra7x SoC. >>> >>> First, >>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. >>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' >>> But, it include potential problems. Other extcon device using gpio would set USB cable state >>> as 'true' when gpio state is 1. This way has dependency on specific SoC. >> no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. >> so if VBUS is zero means its definitely not in connected state. > I tested various development board based on Samsung Exynos series SoC. > Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, > this gpio state could mean off state, disconnected or un-powered state according to gpio. > Of course, above explanation about specific gpio don't include all gpios. > This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,"inverted_gpio)) gpio_usbvid->gpio_inv = 1; And always check on this before deciding? >>> Second, >>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>> would not control both "USB-HOST" and "USB" cable state at same time. >>> >>> Other extcon devices would support either "USB-HOST" or "USB" cable. >> The driver has 2 configurations. >> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). > As you explained about case 1, it is only used on dra7x SoC. No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. > Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. > > >> 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). >> So if you take type as VBUS_ID_DETECT then it is as what you meant. I think i didnt explain it properly last time. In perfect world you will have both VBUS and ID pin routed via gpios for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states USB-HOST (true), USB(true) and both false. Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed. But still we need to differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used 1 irq handler is installed and it will control both USB-HOST and USB cables. In this case its possible to have only 2 states USB-HOST (true) or USB(true). Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin. > "2) case" don't support the detection of "USB-HOST" cable. > Only detect "USB" cable according to "vbus_gpio" value. > > If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file? > But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method. > >>> Third, >>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. >>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. >>> In result, >>> id_irq_handler() would control both "USB-HOST" and "USB" cable state. >> only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states >> if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. > I have some confusion. I need additional your explanation. > Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST > or > Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>> vbus_irq_handler() would control only "USB" cable state. >>> >>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state >>> according to each gpio state at same time. Also, It include critical problem. >> No it depends on the configuration as explained above. >> >> [snip] >> >>> + >>> +#define ID_GND 0 >>> +#define ID_FLOAT 1 >>> +#define VBUS_OFF 0 >>> +#define VBUS_ON 1 >>>>> I think you could only use two constant instead of four constant definition. >>>> you mean only ID_GND and VBUS_OFF? >>> you could only define two contant (0 and 1) to detect gpio state. >> okay >>>>>> + >>>>>> + >>>>> This blank line isn't necessary. >>>>> >>>>>> +static irqreturn_t id_irq_handler(int irq, void *data) >>>>>> +{ >>>>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>>>> You should delete blank between ')' and 'data' as follwong: >>>>> - (struct gpio_usbvid *)data; >>>> okay >>>>>> + int id_current; >>>>>> + >>>>>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>>>> + if (id_current == ID_GND) { >>>>>> + if (gpio_usbvid->type == ID_DETECT) >>>>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>>>> + "USB", false); >>>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>>> As else statement, you should set "USB-HOST" cable state to improve readability. >>>>> >>>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>>> if (gpio_usbvid->type == ID_DETECT) >>>>> extcon_set_cable_state(&gpio_usbvid->edev, >>>>> "USB", false); >>>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio >>>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or >>>> VBUS and ID. >>> I don't understand. Wht does not you change the order of function call as following? >>> >>> Before: >>> if (gpio_usbvid->type == ID_DETECT) >>> extcon_set_cable_state(&gpio_usbvid->edev, >>> "USB", false); >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> Basically these notifiers would go and change the UTMI modes which is s/w controlled. >> so we want to gracefully exit Device mode first and then enter HOST mode. >> this is only with type=ID_DETECT. > Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, > you need this setting order between "USB-HOST" and "USB" cable. yes > Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? No, even if a physical cable is not connected it should default to either USB-HOST or USB > I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. > and can't agree as generic extcon gpio driver. > > [snip]
On 08/29/2013 08:48 PM, George Cherian wrote: > On 8/29/2013 4:07 PM, Chanwoo Choi wrote: >> On 08/29/2013 04:30 PM, George Cherian wrote: >>> Hi Chanwoo, >>> >>> On 8/29/2013 11:53 AM, Chanwoo Choi wrote: >>> [snip] >>>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. >>>> - extcon-[device name].c >>>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. >>>>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. >>>>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic >>>>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion >>>>> with patch v1. >>>> Would you guarantee that this driver support all of extcon devices using gpio pin? >>> This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. >>> Following is the basic assumption made, correct me if I am wrong. >>> ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) >>> ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) >>> VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral) >>> VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). >>> >>> So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. >>> and USB is in HOST mode when ID pin is ground and VBUS is OFF. >>> >>> In all above cases VBUS is referred to the external VBUS supply from an external HOST. >>> >>>> I can't agree. This driver has specific dependency on dra7x SoC. >>>> >>>> First, >>>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. >>>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' >>>> But, it include potential problems. Other extcon device using gpio would set USB cable state >>>> as 'true' when gpio state is 1. This way has dependency on specific SoC. >>> no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. >>> so if VBUS is zero means its definitely not in connected state. >> I tested various development board based on Samsung Exynos series SoC. >> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >> this gpio state could mean off state, disconnected or un-powered state according to gpio. >> Of course, above explanation about specific gpio don't include all gpios. >> This is meaning that there is possibility. > okay then how about adding a flag for inverted logic too. something like this > > if(of_property_read_bool(np,"inverted_gpio)) > gpio_usbvid->gpio_inv = 1; > And always check on this before deciding? > >>>> Second, >>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>> >>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>> The driver has 2 configurations. >>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >> As you explained about case 1, it is only used on dra7x SoC. > No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. I need your answer about above my opinion for other SoC. >> >> >>> 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). >>> So if you take type as VBUS_ID_DETECT then it is as what you meant. > I think i didnt explain it properly last time. > In perfect world you will have both VBUS and ID pin routed via gpios > for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used > 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states > USB-HOST (true), USB(true) and both false. > > Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed. > But still we need to differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used > 1 irq handler is installed and it will control both USB-HOST and USB cables. In this case its possible to have only 2 states > USB-HOST (true) or USB(true). > > Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin > and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin. >> "2) case" don't support the detection of "USB-HOST" cable. >> Only detect "USB" cable according to "vbus_gpio" value. >> >> If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file? >> But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method. >> >>>> Third, >>>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. >>>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. >>>> In result, >>>> id_irq_handler() would control both "USB-HOST" and "USB" cable state. >>> only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states >>> if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. >> I have some confusion. I need additional your explanation. >> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? > If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >> or >> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? > If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. This extcon driver is only suitable dra7x SoC. >>>> vbus_irq_handler() would control only "USB" cable state. >>>> >>>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state >>>> according to each gpio state at same time. Also, It include critical problem. >>> No it depends on the configuration as explained above. >>> >>> [snip] >>> >>>> + >>>> +#define ID_GND 0 >>>> +#define ID_FLOAT 1 >>>> +#define VBUS_OFF 0 >>>> +#define VBUS_ON 1 >>>>>> I think you could only use two constant instead of four constant definition. >>>>> you mean only ID_GND and VBUS_OFF? >>>> you could only define two contant (0 and 1) to detect gpio state. >>> okay >>>>>>> + >>>>>>> + >>>>>> This blank line isn't necessary. >>>>>> >>>>>>> +static irqreturn_t id_irq_handler(int irq, void *data) >>>>>>> +{ >>>>>>> + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; >>>>>> You should delete blank between ')' and 'data' as follwong: >>>>>> - (struct gpio_usbvid *)data; >>>>> okay >>>>>>> + int id_current; >>>>>>> + >>>>>>> + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>>>>> + if (id_current == ID_GND) { >>>>>>> + if (gpio_usbvid->type == ID_DETECT) >>>>>>> + extcon_set_cable_state(&gpio_usbvid->edev, >>>>>>> + "USB", false); >>>>>>> + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>>>> As else statement, you should set "USB-HOST" cable state to improve readability. >>>>>> >>>>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>>>> if (gpio_usbvid->type == ID_DETECT) >>>>>> extcon_set_cable_state(&gpio_usbvid->edev, >>>>>> "USB", false); >>>>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio >>>>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or >>>>> VBUS and ID. >>>> I don't understand. Wht does not you change the order of function call as following? >>>> >>>> Before: >>>> if (gpio_usbvid->type == ID_DETECT) >>>> extcon_set_cable_state(&gpio_usbvid->edev, >>>> "USB", false); >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> Basically these notifiers would go and change the UTMI modes which is s/w controlled. >>> so we want to gracefully exit Device mode first and then enter HOST mode. >>> this is only with type=ID_DETECT. >> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >> you need this setting order between "USB-HOST" and "USB" cable. > yes I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? > No, even if a physical cable is not connected it should default to either USB-HOST or USB It isn't general. If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable should certainly be zero. Because The extcon consumer driver could set proper operation according to cable state. > >> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. I need your answer about above my opinion. >> and can't agree as generic extcon gpio driver. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo, On 8/29/2013 5:42 PM, Chanwoo Choi wrote: [big snip ] >>> I tested various development board based on Samsung Exynos series SoC. >>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>> Of course, above explanation about specific gpio don't include all gpios. >>> This is meaning that there is possibility. >> okay then how about adding a flag for inverted logic too. something like this >> >> if(of_property_read_bool(np,"inverted_gpio)) >> gpio_usbvid->gpio_inv = 1; >> And always check on this before deciding? Is this fine ? >> >>>>> Second, >>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>> >>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>> The driver has 2 configurations. >>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>> As you explained about case 1, it is only used on dra7x SoC. >> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable its not proper? > I need your answer about above my opinion for other SoC. So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) { int id_current, vbus_current; id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); if (!!id_current == ID_FLOAT) extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); else extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); if (!!vbus_current == VBUS_ON) extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); else extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); } and the irq handlers like this static irqreturn_t id_irq_handler(int irq, void *data) { struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; int id_current; id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); if (id_current == ID_GND) extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); else extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); return IRQ_HANDLED; } static irqreturn_t vbus_irq_handler(int irq, void *data) { struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; int vbus_current; vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); if (vbus_current == VBUS_OFF) extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); else extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); return IRQ_HANDLED; } [snip] >>> I have some confusion. I need additional your explanation. >>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>> or >>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. > As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. > This extcon driver is only suitable dra7x SoC. Do you still feel its dra7x specific if i modify it as above? > Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, > you need this setting order between "USB-HOST" and "USB" cable. >> yes > I think that the setting order between cables isn't general. It is specific method for dra7x SoC. So if i remove that part then? >>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >> No, even if a physical cable is not connected it should default to either USB-HOST or USB > It isn't general. > > If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable > should certainly be zero. Because The extcon consumer driver could set proper operation > according to cable state. okay > >> >>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. > I need your answer about above my opinion. Hope i could answer you :-) >>> and can't agree as generic extcon gpio driver. >
On 08/29/2013 05:48 AM, George Cherian wrote: > On 8/29/2013 4:07 PM, Chanwoo Choi wrote: ... >> I tested various development board based on Samsung Exynos series SoC. >> Although some gpio of Exynos series SoC set high state(non zero, 1) as >> default value, >> this gpio state could mean off state, disconnected or un-powered state >> according to gpio. >> Of course, above explanation about specific gpio don't include all gpios. >> This is meaning that there is possibility. > > okay then how about adding a flag for inverted logic too. something > like this > > if(of_property_read_bool(np,"inverted_gpio)) > gpio_usbvid->gpio_inv = 1; > > And always check on this before deciding? Don't do this. GPIO specifiers in DT typically include a "flags" cell that describes certain GPIO properties. One of those properties is signal inversion. You can use of_get_named_gpio_flags() to retrieve those flags. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2013 11:33 AM, George Cherian wrote: > Add a generic USB VBUS/ID detection EXTCON driver. This driver expects > the ID/VBUS pin are connected via GPIOs. This driver is tested on > DRA7x board were the ID pin is routed via GPIOs. The driver supports > both VBUS and ID pin configuration and ID pin only configuration. > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt > +EXTCON FOR USB VIA GPIO Please write a brief explanation of the HW that this binding represents. "Extcon" is a Linux-specific term. Binding documentation should be written in an OS-agnostic manner. Bindings should not reference OS-specific terminology, and should be a pure description of the HW. > +Required Properties: > + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector > + using gpios or "ti,gpio-usb-id" for USB ID pin detector Those souond like two rather different things. Should they be separate bindings, or at the least, separate sections in the document? If this binding is meant to represent some generic hardware, the vendor prefix probably isn't required. So, remove "ti," from the compatible values. > + - gpios : phandle and args ID pin gpio and VBUS gpio. s/and args/and specifier/. > + The first gpio used for ID pin detection > + and the second used for VBUS detection. > + ID pin gpio is mandatory and VBUS is optional > + depending on implementation. It'd be better to use named GPIO properties here, rather than having multiple different kinds of GPIO in the same property. How about "id-gpios" and "vbus-gpios" as the property names? The semantics of each property should be specified. Are these input GPIOs that reflect the ID and VBUS state? Do they directly represent the signal state on the connector, or are they processed somehow? Does the VBUS GPIO control the board's VBUS output, or is it an input? If it controls VBUS output, it probably should be represented as a regulator rather than a GPIO. I think the following might work: Required properties: id-gpios: GPIO specifier for the connector's ID pin input. This directly represents the value present on the ID pin at the connector. Optional properties: vbus-gpios: GPIO specifier for the connector's VBUS signal. This directly represents whether VBUS being driven by the USB cable itself. (although perhaps that isn't an optional property, but rather a required property of the second compatible value?) > +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings > + > +Example: > + > + gpio_usbvid_extcon1 { > + compatible = "ti,gpio-usb-vid"; > + gpios = <&gpio1 1 0>, > + <&gpio2 2 0>; > + }; -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi George, On 08/29/2013 10:45 PM, George Cherian wrote: > Hi Chanwoo, > > > On 8/29/2013 5:42 PM, Chanwoo Choi wrote: > [big snip ] >>>> I tested various development board based on Samsung Exynos series SoC. >>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>> Of course, above explanation about specific gpio don't include all gpios. >>>> This is meaning that there is possibility. >>> okay then how about adding a flag for inverted logic too. something like this >>> >>> if(of_property_read_bool(np,"inverted_gpio)) >>> gpio_usbvid->gpio_inv = 1; >>> And always check on this before deciding? > Is this fine ? OK. But, as Stephen commented on other mail, you should use proper DT helper function. >>> >>>>>> Second, >>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>> >>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>> The driver has 2 configurations. >>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>> As you explained about case 1, it is only used on dra7x SoC. >>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. > I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable > its not proper? It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. Almost device driver including in kernel/drivers control gpio pin on specific device driver instead of generic driver. >> I need your answer about above my opinion for other SoC. > So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) > > static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) > { > int id_current, vbus_current; > > id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); > if (!!id_current == ID_FLOAT) > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); > else > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > > vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); > if (!!vbus_current == VBUS_ON) > extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); > else > extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); > } > > and the irq handlers like this > > static irqreturn_t id_irq_handler(int irq, void *data) > { > struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; > int id_current; > > id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); > if (id_current == ID_GND) > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); > else > extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); > return IRQ_HANDLED; > } > > static irqreturn_t vbus_irq_handler(int irq, void *data) > { > struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; > int vbus_current; > > vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); > if (vbus_current == VBUS_OFF) > extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); > else > extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); > > return IRQ_HANDLED; > } I know your intention dividing interrupt handler for each cable. But I don't think this driver must guarantee all of extcon device using gpio. For example, below three SoC wish to detect USB/USB-HOST cable with each different methods. "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. In addition, if "SoC A/C" wish to write some data to own specific registers for proper opeating, Could we write some data to register on generic driver? Finally, "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin - one gpio may detect either USB or USB-HOST and another may detect JIG cable or one gpio may detect either USb or JIG and another may detect USB-HOST cable. That way, there are many cases we cannot guarantee all of extcon devices. I think this driver could support dra7x series SoC but as I mentioned, this driver may not guarantee all of cases. > [snip] >>>> I have some confusion. I need additional your explanation. >>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>> or >>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >> This extcon driver is only suitable dra7x SoC. > Do you still feel its dra7x specific if i modify it as above? I commented above about your modification. >> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >> you need this setting order between "USB-HOST" and "USB" cable. >>> yes >> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. > So if i remove that part then? The setting order should be removed in generic driver. >>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >> It isn't general. >> >> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >> should certainly be zero. Because The extcon consumer driver could set proper operation >> according to cable state. > okay >> >>> >>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >> I need your answer about above my opinion. > Hope i could answer you :-) >>>> and can't agree as generic extcon gpio driver. >> > Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo, On 8/30/2013 5:41 AM, Chanwoo Choi wrote: > Hi George, > > On 08/29/2013 10:45 PM, George Cherian wrote: >> Hi Chanwoo, >> >> >> On 8/29/2013 5:42 PM, Chanwoo Choi wrote: >> [big snip ] >>>>> I tested various development board based on Samsung Exynos series SoC. >>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>>> Of course, above explanation about specific gpio don't include all gpios. >>>>> This is meaning that there is possibility. >>>> okay then how about adding a flag for inverted logic too. something like this >>>> >>>> if(of_property_read_bool(np,"inverted_gpio)) >>>> gpio_usbvid->gpio_inv = 1; >>>> And always check on this before deciding? >> Is this fine ? > OK. > But, as Stephen commented on other mail, you should use proper DT helper function. okay >>>>>>> Second, >>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>>> >>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>>> The driver has 2 configurations. >>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>>> As you explained about case 1, it is only used on dra7x SoC. >>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. >> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable >> its not proper? > It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. > As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. > Almost device driver including in kernel/drivers control gpio pin on specific device driver > instead of generic driver. But without reading the gpio value how can we set the cable states? for this driver the assumption is the dedicated gpio is always DIR_IN and in the driver we just read the value. >>> I need your answer about above my opinion for other SoC. >> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) >> >> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >> { >> int id_current, vbus_current; >> >> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >> if (!!id_current == ID_FLOAT) >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >> else >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> >> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >> if (!!vbus_current == VBUS_ON) >> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >> else >> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >> } >> >> and the irq handlers like this >> >> static irqreturn_t id_irq_handler(int irq, void *data) >> { >> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >> int id_current; >> >> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >> if (id_current == ID_GND) >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >> else >> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >> return IRQ_HANDLED; >> } >> >> static irqreturn_t vbus_irq_handler(int irq, void *data) >> { >> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >> int vbus_current; >> >> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >> if (vbus_current == VBUS_OFF) >> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >> else >> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >> >> return IRQ_HANDLED; >> } > I know your intention dividing interrupt handler for each cable. > But I don't think this driver must guarantee all of extcon device using gpio. > > For example, > below three SoC wish to detect USB/USB-HOST cable with each different methods. > > "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. You mean to say that both USB ID pin and VBUS are connected to same gpio? If so that is a really bad h/w design and it will not fly. Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables? If so thats what exactly the v3 driver did with compatible gpio-usb-id. > "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver. > "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. Whatever modifications above should meet this need in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed. But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly. > > In addition, > if "SoC A/C" wish to write some data to own specific registers for proper opeating, > Could we write some data to register on generic driver? Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler. > > Finally, > "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have a different driver. > - one gpio may detect either USB or USB-HOST and another may detect JIG cable I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST is detected. > or one gpio may detect either USb or JIG and another may detect USB-HOST cable. As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc) then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios. > That way, there are many cases we cannot guarantee all of extcon devices. > > I think this driver could support dra7x series SoC but as I mentioned, > this driver may not guarantee all of cases. I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard. > >> [snip] >>>>> I have some confusion. I need additional your explanation. >>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>>> or >>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >>> This extcon driver is only suitable dra7x SoC. >> Do you still feel its dra7x specific if i modify it as above? > I commented above about your modification. > >>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >>> you need this setting order between "USB-HOST" and "USB" cable. >>>> yes >>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >> So if i remove that part then? > The setting order should be removed in generic driver. Yes I agree and should be done by the subscriber to the notifier. > >>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >>> It isn't general. >>> >>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >>> should certainly be zero. Because The extcon consumer driver could set proper operation >>> according to cable state. >> okay >>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >>> I need your answer about above my opinion. >> Hope i could answer you :-) >>>>> and can't agree as generic extcon gpio driver. > Thanks, > Chanwoo Choi
Hi George, On 08/30/2013 03:15 PM, George Cherian wrote: > Hi Chanwoo, > > On 8/30/2013 5:41 AM, Chanwoo Choi wrote: >> Hi George, >> >> On 08/29/2013 10:45 PM, George Cherian wrote: >>> Hi Chanwoo, >>> >>> >>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote: >>> [big snip ] >>>>>> I tested various development board based on Samsung Exynos series SoC. >>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>>>> Of course, above explanation about specific gpio don't include all gpios. >>>>>> This is meaning that there is possibility. >>>>> okay then how about adding a flag for inverted logic too. something like this >>>>> >>>>> if(of_property_read_bool(np,"inverted_gpio)) >>>>> gpio_usbvid->gpio_inv = 1; >>>>> And always check on this before deciding? >>> Is this fine ? >> OK. >> But, as Stephen commented on other mail, you should use proper DT helper function. > okay >>>>>>>> Second, >>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>>>> >>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>>>> The driver has 2 configurations. >>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>>>> As you explained about case 1, it is only used on dra7x SoC. >>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. >>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable >>> its not proper? >> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. >> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. >> Almost device driver including in kernel/drivers control gpio pin on specific device driver >> instead of generic driver. > But without reading the gpio value how can we set the cable states? for this driver the assumption is the dedicated gpio > is always DIR_IN and in the driver we just read the value. >>>> I need your answer about above my opinion for other SoC. >>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) >>> >>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >>> { >>> int id_current, vbus_current; >>> >>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> if (!!id_current == ID_FLOAT) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> >>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> if (!!vbus_current == VBUS_ON) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>> } >>> >>> and the irq handlers like this >>> >>> static irqreturn_t id_irq_handler(int irq, void *data) >>> { >>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>> int id_current; >>> >>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> if (id_current == ID_GND) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>> return IRQ_HANDLED; >>> } >>> >>> static irqreturn_t vbus_irq_handler(int irq, void *data) >>> { >>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>> int vbus_current; >>> >>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> if (vbus_current == VBUS_OFF) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>> >>> return IRQ_HANDLED; >>> } >> I know your intention dividing interrupt handler for each cable. >> But I don't think this driver must guarantee all of extcon device using gpio. >> >> For example, >> below three SoC wish to detect USB/USB-HOST cable with each different methods. >> >> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. > > You mean to say that both USB ID pin and VBUS are connected to same gpio? > If so that is a really bad h/w design and it will not fly. > Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables? > If so thats what exactly the v3 driver did with compatible gpio-usb-id. My original question intention, I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C" without othe implementation? > >> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. > > Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver. No, we cannot implement many generic extcon device driver whenever this is the case. >> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. > > Whatever modifications above should meet this need in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed. > But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly. I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C"? If we should implement extcon device driver according each case, Could you tell me that this driver is generic? I think NO. >> >> In addition, >> if "SoC A/C" wish to write some data to own specific registers for proper opeating, >> Could we write some data to register on generic driver? > > Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler. No. there are many extcon consumer driver. We cannot order the sequence of notification reception among all of extcon consumer driver. Only, extcon consumer driver get the changed state of cable and can never set cable H/W setting in extcon consumer driver. Certainly, we have to set cabe H/W setting in extcon producer driver. >> >> Finally, >> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin > Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have > a different driver. No, we cannot guarantee that some cable would be connected to SoC. If this dirver is generic, it have to certainly support above all case in this driver. >> - one gpio may detect either USB or USB-HOST and another may detect JIG cable > I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST is detected. >> or one gpio may detect either USb or JIG and another may detect USB-HOST cable. > As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc) > then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios. > >> That way, there are many cases we cannot guarantee all of extcon devices. >> >> I think this driver could support dra7x series SoC but as I mentioned, >> this driver may not guarantee all of cases. > I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard. I've never seen spec documentation that USB/USB-HOST cable have to be only connected to SoC. As I mentioned above, there are many cases we cannot think about various extcon devices. >> >>> [snip] >>>>>> I have some confusion. I need additional your explanation. >>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>>>> or >>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >>>> This extcon driver is only suitable dra7x SoC. >>> Do you still feel its dra7x specific if i modify it as above? >> I commented above about your modification. >> >>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >>>> you need this setting order between "USB-HOST" and "USB" cable. >>>>> yes >>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >>> So if i remove that part then? >> The setting order should be removed in generic driver. > Yes I agree and should be done by the subscriber to the notifier. OK. >> >>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>>>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >>>> It isn't general. >>>> >>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >>>> should certainly be zero. Because The extcon consumer driver could set proper operation >>>> according to cable state. >>> okay >>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >>>> I need your answer about above my opinion. >>> Hope i could answer you :-) >>>>>> and can't agree as generic extcon gpio driver. >> Thanks, >> Chanwoo Choi Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi George, In addition, I add answer about that device driver control gpio pin directly. On 08/30/2013 03:15 PM, George Cherian wrote: > Hi Chanwoo, > > On 8/30/2013 5:41 AM, Chanwoo Choi wrote: >> Hi George, >> >> On 08/29/2013 10:45 PM, George Cherian wrote: >>> Hi Chanwoo, >>> >>> >>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote: >>> [big snip ] >>>>>> I tested various development board based on Samsung Exynos series SoC. >>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>>>> Of course, above explanation about specific gpio don't include all gpios. >>>>>> This is meaning that there is possibility. >>>>> okay then how about adding a flag for inverted logic too. something like this >>>>> >>>>> if(of_property_read_bool(np,"inverted_gpio)) >>>>> gpio_usbvid->gpio_inv = 1; >>>>> And always check on this before deciding? >>> Is this fine ? >> OK. >> But, as Stephen commented on other mail, you should use proper DT helper function. > okay >>>>>>>> Second, >>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>>>> >>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>>>> The driver has 2 configurations. >>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>>>> As you explained about case 1, it is only used on dra7x SoC. >>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. >>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable >>> its not proper? >> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. >> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. >> Almost device driver including in kernel/drivers control gpio pin on specific device driver >> instead of generic driver. > But without reading the gpio value how can we set the cable states? hmm. I mentioned above my answer as following: >> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. >> Almost device driver including in kernel/drivers control gpio pin on specific device driver Because your extcon driver directly control gpio pin so I think your extcon driver isn't generic. for this driver the assumption is the dedicated gpio > is always DIR_IN and in the driver we just read the value. What is DIR_IN? >>>> I need your answer about above my opinion for other SoC. >>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) >>> >>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >>> { >>> int id_current, vbus_current; >>> >>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> if (!!id_current == ID_FLOAT) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> >>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> if (!!vbus_current == VBUS_ON) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>> } >>> >>> and the irq handlers like this >>> >>> static irqreturn_t id_irq_handler(int irq, void *data) >>> { >>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>> int id_current; >>> >>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>> if (id_current == ID_GND) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>> return IRQ_HANDLED; >>> } >>> >>> static irqreturn_t vbus_irq_handler(int irq, void *data) >>> { >>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>> int vbus_current; >>> >>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>> if (vbus_current == VBUS_OFF) >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>> else >>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>> >>> return IRQ_HANDLED; >>> } >> I know your intention dividing interrupt handler for each cable. >> But I don't think this driver must guarantee all of extcon device using gpio. >> >> For example, >> below three SoC wish to detect USB/USB-HOST cable with each different methods. >> >> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. > > You mean to say that both USB ID pin and VBUS are connected to same gpio? > If so that is a really bad h/w design and it will not fly. > Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables? > If so thats what exactly the v3 driver did with compatible gpio-usb-id. > >> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. > > Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver. >> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. > > Whatever modifications above should meet this need in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed. > But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly. >> >> In addition, >> if "SoC A/C" wish to write some data to own specific registers for proper opeating, >> Could we write some data to register on generic driver? > > Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler. >> >> Finally, >> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin > Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have > a different driver. >> - one gpio may detect either USB or USB-HOST and another may detect JIG cable > I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST is detected. >> or one gpio may detect either USb or JIG and another may detect USB-HOST cable. > As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc) > then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios. > >> That way, there are many cases we cannot guarantee all of extcon devices. >> >> I think this driver could support dra7x series SoC but as I mentioned, >> this driver may not guarantee all of cases. > I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard. >> >>> [snip] >>>>>> I have some confusion. I need additional your explanation. >>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>>>> or >>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >>>> This extcon driver is only suitable dra7x SoC. >>> Do you still feel its dra7x specific if i modify it as above? >> I commented above about your modification. >> >>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >>>> you need this setting order between "USB-HOST" and "USB" cable. >>>>> yes >>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >>> So if i remove that part then? >> The setting order should be removed in generic driver. > Yes I agree and should be done by the subscriber to the notifier. >> >>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>>>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >>>> It isn't general. >>>> >>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >>>> should certainly be zero. Because The extcon consumer driver could set proper operation >>>> according to cable state. >>> okay >>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >>>> I need your answer about above my opinion. >>> Hope i could answer you :-) >>>>>> and can't agree as generic extcon gpio driver. >> Thanks, >> Chanwoo Choi > > Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/30/2013 12:23 PM, Chanwoo Choi wrote: > Hi George, > > On 08/30/2013 03:15 PM, George Cherian wrote: >> Hi Chanwoo, >> >> On 8/30/2013 5:41 AM, Chanwoo Choi wrote: >>> Hi George, >>> >>> On 08/29/2013 10:45 PM, George Cherian wrote: >>>> Hi Chanwoo, >>>> >>>> >>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote: >>>> [big snip ] >>>>>>> I tested various development board based on Samsung Exynos series SoC. >>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>>>>> Of course, above explanation about specific gpio don't include all gpios. >>>>>>> This is meaning that there is possibility. >>>>>> okay then how about adding a flag for inverted logic too. something like this >>>>>> >>>>>> if(of_property_read_bool(np,"inverted_gpio)) >>>>>> gpio_usbvid->gpio_inv = 1; >>>>>> And always check on this before deciding? >>>> Is this fine ? >>> OK. >>> But, as Stephen commented on other mail, you should use proper DT helper function. >> okay >>>>>>>>> Second, >>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>>>>> >>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>>>>> The driver has 2 configurations. >>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>>>>> As you explained about case 1, it is only used on dra7x SoC. >>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. >>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable >>>> its not proper? >>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. >>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. >>> Almost device driver including in kernel/drivers control gpio pin on specific device driver >>> instead of generic driver. >> But without reading the gpio value how can we set the cable states? for this driver the assumption is the dedicated gpio >> is always DIR_IN and in the driver we just read the value. >>>>> I need your answer about above my opinion for other SoC. >>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) >>>> >>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >>>> { >>>> int id_current, vbus_current; >>>> >>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> if (!!id_current == ID_FLOAT) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> >>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>> if (!!vbus_current == VBUS_ON) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>> } >>>> >>>> and the irq handlers like this >>>> >>>> static irqreturn_t id_irq_handler(int irq, void *data) >>>> { >>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>>> int id_current; >>>> >>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> if (id_current == ID_GND) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>> return IRQ_HANDLED; >>>> } >>>> >>>> static irqreturn_t vbus_irq_handler(int irq, void *data) >>>> { >>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>>> int vbus_current; >>>> >>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>> if (vbus_current == VBUS_OFF) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>>> >>>> return IRQ_HANDLED; >>>> } >>> I know your intention dividing interrupt handler for each cable. >>> But I don't think this driver must guarantee all of extcon device using gpio. >>> >>> For example, >>> below three SoC wish to detect USB/USB-HOST cable with each different methods. >>> >>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. >> You mean to say that both USB ID pin and VBUS are connected to same gpio? >> If so that is a really bad h/w design and it will not fly. >> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables? >> If so thats what exactly the v3 driver did with compatible gpio-usb-id. > My original question intention, > I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C" without othe implementation? > Definitely supports SoC A and SoC C. this is neither a generic extcon driver nor a generic gpio driver. This is a generic driver for USB VBUS-ID detection connected via gpios. so doesnot address ADC (SOC B) >>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. >> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver. > No, we cannot implement many generic extcon device driver whenever this is the case. then how can I make this one generic? Pointers? > >>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. >> Whatever modifications above should meet this need in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed. >> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly. > I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C"? Answered above. > If we should implement extcon device driver according each case, > Could you tell me that this driver is generic? I think NO. If for each case separate extcon driver, then its not generic. But How would I make one or how can I modify this one to be more generic? >>> In addition, >>> if "SoC A/C" wish to write some data to own specific registers for proper opeating, >>> Could we write some data to register on generic driver? >> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler. > No. there are many extcon consumer driver. We cannot order the sequence of notification reception among all of extcon consumer driver. > Only, extcon consumer driver get the changed state of cable and can never set cable H/W setting in extcon consumer driver. > Certainly, we have to set cabe H/W setting in extcon producer driver. I dont want the consumer driver to change the cable state. I wanted to mention that the cable state change might need a register write specifc to the H/W which could be done in consumers call back. For eg:- When the USB cable is connected I need to set the UTMI config register to Peripheral mode and when USB-HOST is connected the same need to be set to HOST mode, these writes will be done in the call back functions. >>> Finally, >>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin >> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have >> a different driver. > No, we cannot guarantee that some cable would be connected to SoC. > If this dirver is generic, it have to certainly support above all case in this driver. okay, but at the same time there are tons of devices running linux which doesnot support USB JIG cable. Are those all attributed to linux and its drivers? > >>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable >> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST is detected. >>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable. >> As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc) >> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios. >> >>> That way, there are many cases we cannot guarantee all of extcon devices. >>> >>> I think this driver could support dra7x series SoC but as I mentioned, >>> this driver may not guarantee all of cases. >> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard. > I've never seen spec documentation that USB/USB-HOST cable have to be only connected to SoC. > As I mentioned above, there are many cases we cannot think about various extcon devices. Okay but we have to start with some known working combinations and then increase the number of stuffs supported. >>>> [snip] >>>>>>> I have some confusion. I need additional your explanation. >>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>>>>> or >>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >>>>> This extcon driver is only suitable dra7x SoC. >>>> Do you still feel its dra7x specific if i modify it as above? >>> I commented above about your modification. >>> >>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >>>>> you need this setting order between "USB-HOST" and "USB" cable. >>>>>> yes >>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >>>> So if i remove that part then? >>> The setting order should be removed in generic driver. >> Yes I agree and should be done by the subscriber to the notifier. > OK. > >>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>>>>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >>>>> It isn't general. >>>>> >>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >>>>> should certainly be zero. Because The extcon consumer driver could set proper operation >>>>> according to cable state. >>>> okay >>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >>>>> I need your answer about above my opinion. >>>> Hope i could answer you :-) >>>>>>> and can't agree as generic extcon gpio driver. >>> Thanks, >>> Chanwoo Choi > > Thanks, > Chanwoo Choi >
On 8/30/2013 12:44 PM, Chanwoo Choi wrote: > Hi George, > > In addition, I add answer about that device driver control gpio pin directly. > > On 08/30/2013 03:15 PM, George Cherian wrote: >> Hi Chanwoo, >> >> On 8/30/2013 5:41 AM, Chanwoo Choi wrote: >>> Hi George, >>> >>> On 08/29/2013 10:45 PM, George Cherian wrote: >>>> Hi Chanwoo, >>>> >>>> >>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote: >>>> [big snip ] >>>>>>> I tested various development board based on Samsung Exynos series SoC. >>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, >>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio. >>>>>>> Of course, above explanation about specific gpio don't include all gpios. >>>>>>> This is meaning that there is possibility. >>>>>> okay then how about adding a flag for inverted logic too. something like this >>>>>> >>>>>> if(of_property_read_bool(np,"inverted_gpio)) >>>>>> gpio_usbvid->gpio_inv = 1; >>>>>> And always check on this before deciding? >>>> Is this fine ? >>> OK. >>> But, as Stephen commented on other mail, you should use proper DT helper function. >> okay >>>>>>>>> Second, >>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time >>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices >>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time. >>>>>>>>> >>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable. >>>>>>>> The driver has 2 configurations. >>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). >>>>>>> As you explained about case 1, it is only used on dra7x SoC. >>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. >>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable >>>> its not proper? >>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio. >>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. >>> Almost device driver including in kernel/drivers control gpio pin on specific device driver >>> instead of generic driver. >> But without reading the gpio value how can we set the cable states? > hmm. I mentioned above my answer as following: > >> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin. > >> Almost device driver including in kernel/drivers control gpio pin on specific device driver > Because your extcon driver directly control gpio pin so I think your extcon driver isn't generic. > > for this driver the assumption is the dedicated gpio Obviously when I am writing a generic driver for USB VBUS-ID detetction which is via gpio then i assume I have a dedicated gpio. what is wrong in that assumption? How else can you detect ID pin change and VBUS change via gpio if not you have them dedicated? >> is always DIR_IN and in the driver we just read the value. > What is DIR_IN? Direction IN gpio. >>>>> I need your answer about above my opinion for other SoC. >>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id) >>>> >>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) >>>> { >>>> int id_current, vbus_current; >>>> >>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> if (!!id_current == ID_FLOAT) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> >>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>> if (!!vbus_current == VBUS_ON) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>> } >>>> >>>> and the irq handlers like this >>>> >>>> static irqreturn_t id_irq_handler(int irq, void *data) >>>> { >>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>>> int id_current; >>>> >>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); >>>> if (id_current == ID_GND) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); >>>> return IRQ_HANDLED; >>>> } >>>> >>>> static irqreturn_t vbus_irq_handler(int irq, void *data) >>>> { >>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data; >>>> int vbus_current; >>>> >>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); >>>> if (vbus_current == VBUS_OFF) >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); >>>> else >>>> extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); >>>> >>>> return IRQ_HANDLED; >>>> } >>> I know your intention dividing interrupt handler for each cable. >>> But I don't think this driver must guarantee all of extcon device using gpio. >>> >>> For example, >>> below three SoC wish to detect USB/USB-HOST cable with each different methods. >>> >>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin. >> You mean to say that both USB ID pin and VBUS are connected to same gpio? >> If so that is a really bad h/w design and it will not fly. >> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables? >> If so thats what exactly the v3 driver did with compatible gpio-usb-id. >> >>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin. >> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver. >>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another. >> Whatever modifications above should meet this need in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed. >> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly. >>> In addition, >>> if "SoC A/C" wish to write some data to own specific registers for proper opeating, >>> Could we write some data to register on generic driver? >> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler. >>> Finally, >>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin >> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have >> a different driver. >>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable >> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST is detected. >>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable. >> As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc) >> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios. >> >>> That way, there are many cases we cannot guarantee all of extcon devices. >>> >>> I think this driver could support dra7x series SoC but as I mentioned, >>> this driver may not guarantee all of cases. >> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard. >>>> [snip] >>>>>>> I have some confusion. I need additional your explanation. >>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? >>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST >>>>>>> or >>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB. >>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver. >>>>> This extcon driver is only suitable dra7x SoC. >>>> Do you still feel its dra7x specific if i modify it as above? >>> I commented above about your modification. >>> >>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time, >>>>> you need this setting order between "USB-HOST" and "USB" cable. >>>>>> yes >>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC. >>>> So if i remove that part then? >>> The setting order should be removed in generic driver. >> Yes I agree and should be done by the subscriber to the notifier. >>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable? >>>>>> No, even if a physical cable is not connected it should default to either USB-HOST or USB >>>>> It isn't general. >>>>> >>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable >>>>> should certainly be zero. Because The extcon consumer driver could set proper operation >>>>> according to cable state. >>>> okay >>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC. >>>>> I need your answer about above my opinion. >>>> Hope i could answer you :-) >>>>>>> and can't agree as generic extcon gpio driver. >>> Thanks, >>> Chanwoo Choi >> > Thanks, > Chanwoo Choi >
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt new file mode 100644 index 0000000..eea0741 --- /dev/null +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt @@ -0,0 +1,20 @@ +EXTCON FOR USB VIA GPIO + +Required Properties: + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector + using gpios or "ti,gpio-usb-id" for USB ID pin detector + - gpios : phandle and args ID pin gpio and VBUS gpio. + The first gpio used for ID pin detection + and the second used for VBUS detection. + ID pin gpio is mandatory and VBUS is optional + depending on implementation. + +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings + +Example: + + gpio_usbvid_extcon1 { + compatible = "ti,gpio-usb-vid"; + gpios = <&gpio1 1 0>, + <&gpio2 2 0>; + }; diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index f1d54a3..8097398 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -64,4 +64,10 @@ config EXTCON_PALMAS Say Y here to enable support for USB peripheral and USB host detection by palmas usb. +config EXTCON_GPIO_USBVID + tristate "Generic USB VBUS/ID detection using GPIO EXTCON support" + help + Say Y here to enable support for USB VBUS/ID deetction by GPIO. + + endif # MULTISTATE_SWITCH diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index e4fa8ba..0451f698 100644 --- a/drivers/extcon/Makefile +++ b/drivers/extcon/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o +obj-$(CONFIG_EXTCON_GPIO_USBVID) += extcon-gpio-usbvid.o diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c new file mode 100644 index 0000000..e9bc2a97 --- /dev/null +++ b/drivers/extcon/extcon-gpio-usbvid.c @@ -0,0 +1,286 @@ +/* + * Generic USB VBUS-ID pin detection driver + * + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * Author: George Cherian <george.cherian@ti.com> + * + * Based on extcon-palmas.c + * + * Author: Kishon Vijay Abraham I <kishon@ti.com> + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/kthread.h> +#include <linux/freezer.h> +#include <linux/platform_device.h> +#include <linux/extcon.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/gpio.h> +#include <linux/of_gpio.h> +#include <linux/of_platform.h> + +struct gpio_usbvid { + struct device *dev; + + struct extcon_dev edev; + + /*GPIO pin */ + int id_gpio; + int vbus_gpio; + + int id_irq; + int vbus_irq; + int type; +}; + +static const char *dra7xx_extcon_cable[] = { + [0] = "USB", + [1] = "USB-HOST", + NULL, +}; + +static const int mutually_exclusive[] = {0x3, 0x0}; + +/* Two types of support are provided. + * Systems which has + * 1) VBUS and ID pin connected via GPIO + * 2) only ID pin connected via GPIO + * For Case 1 both the gpios should be provided via DT + * Always the first GPIO in dt is considered ID pin GPIO + */ + +enum { + UNKNOWN = 0, + ID_DETECT, + VBUS_ID_DETECT, +}; + +#define ID_GND 0 +#define ID_FLOAT 1 +#define VBUS_OFF 0 +#define VBUS_ON 1 + + +static irqreturn_t id_irq_handler(int irq, void *data) +{ + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; + int id_current; + + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); + if (id_current == ID_GND) { + if (gpio_usbvid->type == ID_DETECT) + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", false); + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true); + } else { + extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false); + if (gpio_usbvid->type == ID_DETECT) + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", true); + } + return IRQ_HANDLED; +} + +static irqreturn_t vbus_irq_handler(int irq, void *data) +{ + struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data; + int vbus_current; + + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); + if (vbus_current == VBUS_OFF) + extcon_set_cable_state(&gpio_usbvid->edev, "USB", false); + else + extcon_set_cable_state(&gpio_usbvid->edev, "USB", true); + + return IRQ_HANDLED; +} + + +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid) +{ + int id_current; + int vbus_current; + + switch (gpio_usbvid->type) { + case ID_DETECT: + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); + if (!!id_current == ID_FLOAT) { + extcon_set_cable_state(&gpio_usbvid->edev, + "USB-HOST", false); + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", true); + } else { + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", false); + extcon_set_cable_state(&gpio_usbvid->edev, + "USB-HOST", true); + } + break; + + case VBUS_ID_DETECT: + id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio); + if (!!id_current == ID_FLOAT) + extcon_set_cable_state(&gpio_usbvid->edev, + "USB-HOST", false); + else + extcon_set_cable_state(&gpio_usbvid->edev, + "USB-HOST", true); + + vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio); + if (!!vbus_current == VBUS_ON) + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", true); + else + extcon_set_cable_state(&gpio_usbvid->edev, + "USB", false); + break; + + default: + dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n"); + } +} + +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid) +{ + int ret; + ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq, + NULL, id_irq_handler, + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, + dev_name(gpio_usbvid->dev), + (void *) gpio_usbvid); + if (ret) { + dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n", + gpio_usbvid->id_irq); + return ret; + } + if (gpio_usbvid->type == VBUS_ID_DETECT) { + ret = devm_request_threaded_irq(gpio_usbvid->dev, + gpio_usbvid->vbus_irq, NULL, + vbus_irq_handler, + IRQF_ONESHOT | IRQF_TRIGGER_FALLING, + dev_name(gpio_usbvid->dev), + (void *) gpio_usbvid); + if (ret) + dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n", + gpio_usbvid->vbus_irq); + } + return ret; +} + +static int gpio_usbvid_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + struct gpio_usbvid *gpio_usbvid; + int ret; + int gpio; + + gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid), + GFP_KERNEL); + if (!gpio_usbvid) + return -ENOMEM; + + + gpio_usbvid->dev = &pdev->dev; + + platform_set_drvdata(pdev, gpio_usbvid); + + gpio_usbvid->edev.name = dev_name(&pdev->dev); + gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable; + gpio_usbvid->edev.mutually_exclusive = mutually_exclusive; + + if (of_device_is_compatible(node, "ti,gpio-usb-id")) + gpio_usbvid->type = ID_DETECT; + + gpio = of_get_gpio(node, 0); + if (gpio_is_valid(gpio)) { + gpio_usbvid->id_gpio = gpio; + ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio, + "id_gpio"); + if (ret) + return ret; + gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio); + } else { + dev_err(&pdev->dev, "failed to get id gpio\n"); + return -ENODEV; + } + + if (of_device_is_compatible(node, "ti,gpio-usb-vid")) { + gpio_usbvid->type = VBUS_ID_DETECT; + gpio = of_get_gpio(node, 1); + if (gpio_is_valid(gpio)) { + gpio_usbvid->vbus_gpio = gpio; + ret = devm_gpio_request(&pdev->dev, + gpio_usbvid->vbus_gpio, + "vbus_gpio"); + if (ret) + return ret; + gpio_usbvid->vbus_irq = + gpio_to_irq(gpio_usbvid->vbus_gpio); + } else { + dev_err(&pdev->dev, "failed to get vbus gpio\n"); + return -ENODEV; + } + } + + ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev); + if (ret) { + dev_err(&pdev->dev, "failed to register extcon device\n"); + return ret; + } + + gpio_usbvid_set_initial_state(gpio_usbvid); + ret = gpio_usbvid_request_irq(gpio_usbvid); + if (ret) + goto err0; + + return 0; + +err0: + extcon_dev_unregister(&gpio_usbvid->edev); + + return ret; +} + +static int gpio_usbvid_remove(struct platform_device *pdev) +{ + struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev); + + extcon_dev_unregister(&gpio_usbvid->edev); + return 0; +} + +static struct of_device_id of_gpio_usbvid_match_tbl[] = { + { .compatible = "ti,gpio-usb-vid", }, + { .compatible = "ti,gpio-usb-id", }, + { /* end */ } +}; + +static struct platform_driver gpio_usbvid_driver = { + .probe = gpio_usbvid_probe, + .remove = gpio_usbvid_remove, + .driver = { + .name = "gpio-usbvid", + .of_match_table = of_gpio_usbvid_match_tbl, + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(gpio_usbvid_driver); + +MODULE_ALIAS("platform:gpio-usbvid"); +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>"); +MODULE_DESCRIPTION("GPIO based USB Connector driver"); +MODULE_LICENSE("GPL"); +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects the ID/VBUS pin are connected via GPIOs. This driver is tested on DRA7x board were the ID pin is routed via GPIOs. The driver supports both VBUS and ID pin configuration and ID pin only configuration. Signed-off-by: George Cherian <george.cherian@ti.com> --- .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ drivers/extcon/Kconfig | 6 + drivers/extcon/Makefile | 1 + drivers/extcon/extcon-gpio-usbvid.c | 286 +++++++++++++++++++++ 4 files changed, 313 insertions(+) create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt create mode 100644 drivers/extcon/extcon-gpio-usbvid.c