Hi George,Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
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 expectsYou should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
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@xxxxxx>
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-gpio-usbvid.c | 286 +++++++++++++++++++++
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
I meant to support all of extcon driver using gpio for USB VBUS/ID detection.
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.
[snip]
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
okayindex f1d54a3..8097398 100644Remove blank line.
--- 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.
+
+
[snip]endif # MULTISTATE_SWITCH
okaydiff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.ckthead.h, freezer.h headerfile is used in this file?
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@xxxxxx>
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I <kishon@xxxxxx>
+ *
+ * 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>
okay+#include <linux/platform_device.h>Order headerfile alphabetically.
+#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>
okay
+I commented previous your patch about this wrong coding style.
+struct gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
Why did not you fix this coding style?
okay+ int id_gpio;Remove blank between '2)' and 'only'.
+ 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
you mean only ID_GND and VBUS_OFF?+ * For Case 1 both the gpios should be provided via DTI think you could only use two constant instead of four constant definition.
+ * 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
okay+This blank line isn't necessary.
+
+static irqreturn_t id_irq_handler(int irq, void *data)You should delete blank between ')' and 'data' as follwong:
+{
+ struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
- (struct gpio_usbvid *)data;
Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
+ int id_current;As else statement, you should set "USB-HOST" cable state to improve readability.
+
+ 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);
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);
okay+ } else {Add blank line.
+ 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);
+ }
okay+ return IRQ_HANDLED;ditto.
+}
+
+static irqreturn_t vbus_irq_handler(int irq, void *data)
+{
+ struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
okay+ int vbus_current;This blank line isn't necessary.
+
+ 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 commented unnecessary blank line on previous review.
okay+static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)Define loacal variable on one line as following:
+{
+ int id_current;
+ int vbus_current;
int id_current, vbus_current;
okay+Add blank line.
+ 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;
if I use dev_name it gives me 2 same name interrupts associated with a single extcon device.+ ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_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),
You should use characteristic interrupt name.
okay+ (void *) gpio_usbvid);Add blank line.
+ if (ret)
+ dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
+ gpio_usbvid->vbus_irq);
+ }
okay+ return ret;Define loacal variable on one line as following:
+}
+
+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;
int ret, gpio;
+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);
gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
GFP_KERNEL);
okay
+ if (!gpio_usbvid)Remove blank line.
+ return -ENOMEM;
+
+
okay+ gpio_usbvid->dev = &pdev->dev;Use space instead of tab.
I removed it but it gave me a NULL pointer dereference in extcon_get_extcon_dev (strcmp the sd->name was NULL).+I add as follwong comment about your v1 patchset
+ platform_set_drvdata(pdev, gpio_usbvid);
+
+ gpio_usbvid->edev.name = dev_name(&pdev->dev);
"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.
okay+ gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;Add blank line.
+ 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);Add blank line.
+ } 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 =You should move gpio_usbvid_request_irq() call before extcon_dev_register().
+ 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)? As following previous comment about v1 patchset:
+ goto err0;
I need correct meaning name as err_thread or etc ...
okay
+ditto.
+ return 0;
+
+err0:
+ extcon_dev_unregister(&gpio_usbvid->edev);Cheers,
+
+ 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@xxxxxx>");
+MODULE_DESCRIPTION("GPIO based USB Connector driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
Chanwoo Choi