Hi George,
On 08/16/2013 07:13 PM, George Cherian wrote:Adding extcon driver for USB ID detection to dynamicallyYou used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
configure USB Host/Peripheral mode.
Signed-off-by: George Cherian <george.cherian@xxxxxx>
---
.../devicetree/bindings/extcon/extcon-dra7xx.txt | 19 ++
drivers/extcon/Kconfig | 7 +
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-dra7xx.c | 234 +++++++++++++++++++++
4 files changed, 261 insertions(+)
create mode 100644 Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
create mode 100644 drivers/extcon/extcon-dra7xx.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
new file mode 100644
index 0000000..37e4c22
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
@@ -0,0 +1,19 @@
+EXTCON FOR DRA7xx
+
+Required Properties:
+ - compatible : Should be "ti,dra7xx-usb"
+ - gpios : phandle to ID pin and interrupt gpio.
+
+Optional Properties:
+ - interrupt-parent : interrupt controller phandle
+ - interrupts : interrupt number
+
+
+dra7x_extcon1 {
What is meaning 'dra7x_extcon1'?
+ compatible = "ti,dra7xx-usb";You have to keep indentation rule.
+ gpios = <&pcf_usb 1 0>,
+ <&gpio6 11 2>;
+ interrupt-parent = <&gpio6>;
+ interrupts = <11>;
+ };
+You should explain detailed description about pcf8575 on patch description
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index f1d54a3..b9cf0b2 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,11 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_DRA7XX
+ tristate "DRA7XX EXTCON support"
+ help
+ Say Y here to enable support for USB peripheral and USB host
+ detection by pcf8575 using DRA7XX extcon.
and change description of EXTCON_DRA7xx as following:
"using DRA7XX extcon" -> "using DRA7XX device" or "using DRA7XX usb"
+Remove unnecessary blank line.
+
endif # MULTISTATE_SWITCHAdd space between "/*" and "GPIO".
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index e4fa8ba..e4778f9 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_DRA7XX) += extcon-dra7xx.o
diff --git a/drivers/extcon/extcon-dra7xx.c b/drivers/extcon/extcon-dra7xx.c
new file mode 100644
index 0000000..268c25e
--- /dev/null
+++ b/drivers/extcon/extcon-dra7xx.c
@@ -0,0 +1,234 @@
+/*
+ * DRA7XX USB 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>
+#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 dra7xx_usb {
+ struct device *dev;
+
+ struct extcon_dev edev;
+ struct task_struct *thread_task;
+
+ /*GPIO pin */
+ int id_gpio;Should dra7xx_usb keep always connected state with either USB or USB-HOST cable?
+ int irq_gpio;
+
+ int id_prev;
+ int id_current;
+
+};
+
+static const char *dra7xx_extcon_cable[] = {
+ [0] = "USB",
+ [1] = "USB-HOST",
+ NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+static int id_poll_func(void *data)
+{
+ struct dra7xx_usb *dra7xx_usb = (struct dra7xx_usb *) data;
+
+ allow_signal(SIGINT);
+ allow_signal(SIGTERM);
+ allow_signal(SIGKILL);
+ allow_signal(SIGUSR1);
+
+ set_freezable();
+
+ while (!kthread_should_stop()) {
+ dra7xx_usb->id_current = gpio_get_value_cansleep
+ (dra7xx_usb->id_gpio);
+ if (dra7xx_usb->id_current == dra7xx_usb->id_prev) {
+ schedule_timeout_interruptible
+ (msecs_to_jiffies(2*1000));
+ continue;
+ } else if (dra7xx_usb->id_current == 0) {
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", false);
+ extcon_set_cable_state(&dra7xx_usb->edev,
+ "USB-HOST", true);
+ } else {
+ extcon_set_cable_state(&dra7xx_usb->edev,
+ "USB-HOST", false);
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", true);
+ }
I don't understand. So please explain detailed operation method of dra7xx_usb device.
+ dra7xx_usb->id_prev = dra7xx_usb->id_current;why did you do keep always connected state?
+ }
+
+ return 0;
+}
+
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+ struct dra7xx_usb *dra7xx_usb = (struct dra7xx_usb *) data;
+
+ dra7xx_usb->id_current = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
+
+ if (dra7xx_usb->id_current == dra7xx_usb->id_prev) {
+ return IRQ_NONE;
+ } else if (dra7xx_usb->id_current == 0) {
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", false);
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", true);
+ } else {
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", false);
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", true);
+ }
+Add blank line.
+ dra7xx_usb->id_prev = dra7xx_usb->id_current;
+ return IRQ_HANDLED;Remove unnecessary blank line.
+
+}'status' local variable is used for return value.
+
+static int dra7xx_usb_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct dra7xx_usb *dra7xx_usb;
+ int status;
So, I prefer 'ret' variable instead of 'status' name.
+ int irq_num;You have to add error message with dev_err().
+ int gpio;
+
+ dra7xx_usb = devm_kzalloc(&pdev->dev, sizeof(*dra7xx_usb), GFP_KERNEL);
+ if (!dra7xx_usb)
+ return -ENOMEM;
+Remove unnecessary blank line.
+
+ dra7xx_usb->dev = &pdev->dev;If edev name is equal with device name, this line is unnecessary.
+
+ platform_set_drvdata(pdev, dra7xx_usb);
+
+ dra7xx_usb->edev.name = dev_name(&pdev->dev);
Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
in extcon-class.c
+ dra7xx_usb->edev.supported_cable = dra7xx_extcon_cable;You have to add error message with dev_err().
+ dra7xx_usb->edev.mutually_exclusive = mutually_exclusive;
+
+ gpio = of_get_gpio(node, 0);
+ if (gpio_is_valid(gpio)) {
+ dra7xx_usb->id_gpio = gpio;
+ status = gpio_request(dra7xx_usb->id_gpio, "id_gpio");
+ if (status)
+ return status;
+ } else {Remove unnecessary blank line.
+ dev_err(&pdev->dev, "failed to get id gpio\n");
+ return -ENODEV;
+ }
+
+ gpio = of_get_gpio(node, 1);
+ if (gpio_is_valid(gpio)) {
+ dra7xx_usb->irq_gpio = gpio;
+ irq_num = gpio_to_irq(dra7xx_usb->irq_gpio);
+ if (irq_num < 0)
+ dra7xx_usb->irq_gpio = 0;
+ } else {
+ dev_err(&pdev->dev, "failed to get irq gpio\n");
+ }
+
+
+ status = extcon_dev_register(&dra7xx_usb->edev, dra7xx_usb->dev);You should restore previous operation about dra7xx_usb->irq_gpio.
+ if (status) {
+ dev_err(&pdev->dev, "failed to register extcon device\n");
+ return status;
+ }why did you do keep always connected state?
+
+ dra7xx_usb->id_prev = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
+ if (dra7xx_usb->id_prev) {
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", false);
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", true);
+ } else {
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB", false);
+ extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", true);
+ }
+If devm_request_threaded_irq() return fail state, why did not you do add error exception?
+ if (dra7xx_usb->irq_gpio) {
+ status = devm_request_threaded_irq(dra7xx_usb->dev, irq_num,
+ NULL, id_irq_handler, IRQF_SHARED |
+ IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+ dev_name(&pdev->dev), (void *) dra7xx_usb);
+ if (status)
+ dev_err(dra7xx_usb->dev, "failed to request irq #%d\n",
+ irq_num);
+ elseIf devm_request_threaded_irq() return success state, why did you directly call 'return'?
+ return 0;
kthread_create operation isn't necessary?
+ }Should you use polling method with kthread? I think it isn't proper method.
+
+ dra7xx_usb->thread_task = kthread_create(id_poll_func, dra7xx_usb,
+ dev_name(&pdev->dev));
You did get the irq number by using DT helper function and register irq handler
with devm_request_threaded_irq(). I prefer interrupt method for detection of cable state.
Also, you set delay time as 2000 millisecond for kthread. kthread don't guarantee rapid response.+ schedule_timeout_interruptible
+ (msecs_to_jiffies(2*1000));
+ if (!dra7xx_usb->thread_task) {I need correct meaning name as err_thread or etc ...
+ dev_err(dra7xx_usb->dev, "failed to create thread for %s\n"
+ , dev_name(&pdev->dev));
+ goto err0;
+ }ditto.
+
+ wake_up_process(dra7xx_usb->thread_task);
+
+ return 0;
+
+err0:
+ gpio_free(dra7xx_usb->id_gpio);Thanks,
+ extcon_dev_unregister(&dra7xx_usb->edev);
+
+ return status;
+}
+
+static int dra7xx_usb_remove(struct platform_device *pdev)
+{
+ struct dra7xx_usb *dra7xx_usb = platform_get_drvdata(pdev);
+
+ if (!dra7xx_usb->irq_gpio)
+ kthread_stop(dra7xx_usb->thread_task);
+
+ gpio_free(dra7xx_usb->id_gpio);
+ extcon_dev_unregister(&dra7xx_usb->edev);
+
+ return 0;
+}
+
+static struct of_device_id of_dra7xx_match_tbl[] = {
+ { .compatible = "ti,dra7xx-usb", },
+ { /* end */ }
+};
+
+static struct platform_driver dra7xx_usb_driver = {
+ .probe = dra7xx_usb_probe,
+ .remove = dra7xx_usb_remove,
+ .driver = {
+ .name = "dra7xx-usb",
+ .of_match_table = of_dra7xx_match_tbl,
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(dra7xx_usb_driver);
+
+MODULE_ALIAS("platform:dra7xx-usb");
+MODULE_AUTHOR("George Cherian <george.cherian@xxxxxx>");
+MODULE_DESCRIPTION("DRA7x USB Connector driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_dra7xx_match_tbl);
Chanwoo Choi