Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
Hi Stephen,
On 8/20/2013 10:23 PM, Stephen Warren wrote:
ID pins are connected to pcf8575, and the pcf8575's interrupt line is
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change.
In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
controller, as does the driver for the PCF8575. This binding should have
a single entry in the gpios property, and the driver can call
gpio_to_irq() on that so it knows which IRQ to request.
You meant some thing like this?
pcf_usb: pcf8575@21 {
compatible = ti,pcf8575;
reg = 0x21;
gpio-controller;
#gpio-cells = 2;
interrupt-parent = gpio6;
interrupts = 11 2;
interrupt-controller;
#interrupt-cells = 2;
};
usb_vid_gpio {
compatible = ti,dra7xx-usb;
gpios = pcf_usb 1 0;
};
--
-George
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/21/2013 07:06 AM, George Cherian wrote:
Hi Stephen,
On 8/20/2013 10:23 PM, Stephen Warren wrote:
ID pins are connected to pcf8575, and the pcf8575's interrupt line is
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin
change.
In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
controller, as does the driver for the PCF8575. This binding should have
a single entry in the gpios property, and the driver can call
gpio_to_irq() on that so it knows which IRQ to request.
You meant some thing like this?
pcf_usb: pcf8575@21 {
compatible = ti,pcf8575;
reg = 0x21;
gpio-controller;
#gpio-cells = 2;
interrupt-parent = gpio6;
interrupts = 11 2;
interrupt-controller;
#interrupt-cells = 2;
};
usb_vid_gpio {
compatible = ti,dra7xx-usb;
gpios = pcf_usb 1 0;
};
Yes.
Except that the compatible value for the usb_vid_gpio node still looks
wrong, since I think that node isn't anything to do with any particular SoC.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 8/21/2013 11:05 PM, Stephen Warren wrote:
On 08/21/2013 07:06 AM, George Cherian wrote:
Hi Stephen,
On 8/20/2013 10:23 PM, Stephen Warren wrote:
ID pins are connected to pcf8575, and the pcf8575's interrupt line is
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin
change.
In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
controller, as does the driver for the PCF8575. This binding should have
a single entry in the gpios property, and the driver can call
gpio_to_irq() on that so it knows which IRQ to request.
You meant some thing like this?
pcf_usb: pcf8575@21 {
compatible = ti,pcf8575;
reg = 0x21;
gpio-controller;
#gpio-cells = 2;
interrupt-parent = gpio6;
interrupts = 11 2;
interrupt-controller;
#interrupt-cells = 2;
};
usb_vid_gpio {
compatible = ti,dra7xx-usb;
gpios = pcf_usb 1 0;
};
Yes.
Except that the compatible value for the usb_vid_gpio node still looks
wrong, since I think that node isn't anything to do with any particular SoC.
Yes will fix that too in v2.
--
-George
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
Hi Stephen,
Thanks for your review.
On 8/20/2013 1:01 AM, Stephen Warren wrote:
On 08/16/2013 04:13 AM, George Cherian wrote:
Adding extcon driver for USB ID detection to dynamically
configure USB Host/Peripheral mode.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
+EXTCON FOR DRA7xx
+
+Required Properties:
Please at lest explain what a DRA7xxx is, and the purpose of the HW
module this binding describes.
DRA7xx is the SoC name and the USB VID detection is implemented via gpio's
Basically it does only ID detection via GPIO and there is no VBUS
detection in h/w.
+ - compatible : Should be ti,dra7xx-usb
If this is a USB VID detector rather than a full USB host controller,
then usb in the binding is a bit over-reaching. Perhaps -usb-vid or
-usb-vid-detector would be more accurate.
This will be renamed to dra7xx-extcon.
+ - gpios : phandle to ID pin and interrupt gpio.
This isn't just a phandle; it's phandle+args, or a GPIO specifier. Some
reference should be made to ../gpio/gpio.txt for the format.
okay
Why does the interrupt line need to be included in a list of GPIOs?
ID pins are connected to pcf8575, and the pcf8575's interrupt line is
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change.
If the DRA7xxx is a VID detector, why does it even need/have any GPIOs
associated with it; surely it has a dedicated VID input pin. Can you
provide more details re: how the HW is structured.
+Optional Properties:
+ - interrupt-parent : interrupt controller phandle
+ - interrupts : interrupt number
+
+
It's typical insert the following between those two blank lines:
Example:
... or delete one of the blank lines.
+dra7x_extcon1 {
+ compatible = ti,dra7xx-usb;
+ gpios = pcf_usb 1 0,
+ gpio6 11 2;
+ interrupt-parent = gpio6;
+ interrupts = 11;
+ };
+
No need for that trailing blank line.
okay
--
-George
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
Hi Chanwoo,
Thanks for your review.
On 8/20/2013 5:54 AM, Chanwoo Choi wrote:
Hi George,
On 08/16/2013 07:13 PM, George Cherian wrote:
Adding extcon driver for USB ID detection to dynamically
configure USB Host/Peripheral mode.
Signed-off-by: George Cherian george.cher...@ti.com
---
.../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 000..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 {
You used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
What is meaning 'dra7x_extcon1'?
I will rename it to dra7xx_extcon.
+ compatible = ti,dra7xx-usb;
+ gpios = pcf_usb 1 0,
+ gpio6 11 2;
+ interrupt-parent = gpio6;
+ interrupts = 11;
+ };
You have to keep indentation rule.
okay
+
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.
You should explain detailed description about pcf8575 on patch description
and change description of EXTCON_DRA7xx as following:
using DRA7XX extcon - using DRA7XX device or using DRA7XX usb
okay
+
+
Remove unnecessary blank line.
okay
endif # MULTISTATE_SWITCH
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 000..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.cher...@ti.com
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I kis...@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 dra7xx_usb {
+ struct device *dev;
+
+ struct extcon_dev edev;
+ struct task_struct *thread_task;
+
+ /*GPIO pin */
Add space between /* and GPIO.
okay
+ int id_gpio;
+ 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
+
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
Hi George,
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
new file mode 100644
index 000..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 {
You used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
What is meaning 'dra7x_extcon1'?
I will rename it to dra7xx_extcon.
extcon naming means various external connector device. e.g., usb, jack, etc...
So, I prefer 'dra7xx_usb' instead of 'dra7xx_extcon'. I have plan to divide
extcon device driver according to the kind of device.
+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);
+}
Should dra7xx_usb keep always connected state with either USB or USB-HOST
cable?
I don't understand. So please explain detailed operation method of
dra7xx_usb device.
In dra7xx only ID pin is connected to the SoC gpio. There is no way,
currently to detect the VBUS on/off.
So I always default to either HOST/DEVICE mode solely depending on the ID pin
value.
OK.
But I don't want to use kthread with polling method.
I recommend that you use interrupt method for cable detection.
All of extcon device driver have only used interrupt method without polling.
+dra7xx_usb-id_prev = dra7xx_usb-id_current;
+}
+
+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) {
You should define some constant variable to clarify '0' meaning instead of
using '0' directly.
+dra7xx_usb = devm_kzalloc(pdev-dev, sizeof(*dra7xx_usb), GFP_KERNEL);
+if (!dra7xx_usb)
+return -ENOMEM;
You have to add error message with dev_err().
devm_kzalloc itself should give some message.
ok.
+status = extcon_dev_register(dra7xx_usb-edev, dra7xx_usb-dev);
+if (status) {
+dev_err(pdev-dev, failed to register extcon device\n);
+return status;
You should restore previous operation about dra7xx_usb-irq_gpio.
okay
+}
+
+dra7xx_usb-id_prev = gpio_get_value_cansleep(dra7xx_usb-id_gpio);
+if (dra7xx_usb-id_prev) {
ditto.
You should define some constant variable to clarify 'dra7xx_usb-id_prev'
meaning.
+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);
+}
why did you do keep always connected state?
There is no way, currently to detect the VBUS on/off.
So I always default to either HOST/DEVICE mode solely depending on the ID pin
value.
+
+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);
If devm_request_threaded_irq() return fail state, why did not you do add
error exception?
If interrupt fails I fallback to polling thread.
+else
+return 0;
If devm_request_threaded_irq() return success state, why did you directly
call 'return'?
kthread_create operation isn't necessary?
Yes kthread is optional. Some boards doenot have the ID pin hooked onto the
GPIO.
In
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 8/20/2013 3:59 PM, Chanwoo Choi wrote:
Hi George,
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
new file mode 100644
index 000..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 {
You used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
What is meaning 'dra7x_extcon1'?
I will rename it to dra7xx_extcon.
extcon naming means various external connector device. e.g., usb, jack, etc...
So, I prefer 'dra7xx_usb' instead of 'dra7xx_extcon'. I have plan to divide
extcon device driver according to the kind of device.
okay will do it in v2
+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);
+}
Should dra7xx_usb keep always connected state with either USB or USB-HOST cable?
I don't understand. So please explain detailed operation method of dra7xx_usb
device.
In dra7xx only ID pin is connected to the SoC gpio. There is no way, currently
to detect the VBUS on/off.
So I always default to either HOST/DEVICE mode solely depending on the ID pin
value.
OK.
But I don't want to use kthread with polling method.
I recommend that you use interrupt method for cable detection.
All of extcon device driver have only used interrupt method without polling.
okay will remove the polling thread.
+dra7xx_usb-id_prev = dra7xx_usb-id_current;
+}
+
+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) {
You should define some constant variable to clarify '0' meaning instead of
using '0' directly.
okay
+dra7xx_usb = devm_kzalloc(pdev-dev, sizeof(*dra7xx_usb), GFP_KERNEL);
+if (!dra7xx_usb)
+return -ENOMEM;
You have to add error message with dev_err().
devm_kzalloc itself should give some message.
ok.
+status = extcon_dev_register(dra7xx_usb-edev, dra7xx_usb-dev);
+if (status) {
+dev_err(pdev-dev, failed to register extcon device\n);
+return status;
You should restore previous operation about dra7xx_usb-irq_gpio.
okay
+}
+
+dra7xx_usb-id_prev = gpio_get_value_cansleep(dra7xx_usb-id_gpio);
+if (dra7xx_usb-id_prev) {
ditto.
You should define some constant variable to clarify 'dra7xx_usb-id_prev'
meaning.
okay
+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);
+}
why did you do keep always connected state?
There is no way, currently to detect the VBUS on/off.
So I always default to either HOST/DEVICE mode solely depending on the ID pin
value.
+
+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);
If devm_request_threaded_irq() return fail state, why did not you do add error
exception?
If interrupt fails I fallback to polling thread.
+else
+return 0;
If devm_request_threaded_irq() return success state, why did you directly call
'return'?
kthread_create operation isn't necessary?
Yes kthread is optional. Some boards doenot have the ID pin hooked onto
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/20/2013 12:55 AM, George Cherian wrote: Hi Stephen, Thanks for your review. On 8/20/2013 1:01 AM, Stephen Warren wrote: On 08/16/2013 04:13 AM, George Cherian wrote: Adding extcon driver for USB ID detection to dynamically configure USB Host/Peripheral mode. diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt +EXTCON FOR DRA7xx + +Required Properties: Please at lest explain what a DRA7xxx is, and the purpose of the HW module this binding describes. DRA7xx is the SoC name and the USB VID detection is implemented via gpio's Basically it does only ID detection via GPIO and there is no VBUS detection in h/w. If there's no SoC-specific HW, then the binding has nothing to do with the SoC; it's entirely generic. I think instead, you need to define a generic gpio-usb-vid binding instead. Otherwise, ever SoC that doesn't have explicit USB VID detection HW is going to re-invent the exact same binding, with pointless differences, rather than using a single generic binding. + - compatible : Should be ti,dra7xx-usb If this is a USB VID detector rather than a full USB host controller, then usb in the binding is a bit over-reaching. Perhaps -usb-vid or -usb-vid-detector would be more accurate. This will be renamed to dra7xx-extcon. So no, that rename doesn't sound correct. + - gpios : phandle to ID pin and interrupt gpio. Why does the interrupt line need to be included in a list of GPIOs? ID pins are connected to pcf8575, and the pcf8575's interrupt line is inturn connected to gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change. In that case, the PCF8575 node needs to be a GPIO controller and an IRQ controller, as does the driver for the PCF8575. This binding should have a single entry in the gpios property, and the driver can call gpio_to_irq() on that so it knows which IRQ to request. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
+
+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);
If devm_request_threaded_irq() return fail state, why did not you do add
error exception?
If interrupt fails I fallback to polling thread.
+else
+return 0;
If devm_request_threaded_irq() return success state, why did you directly
call 'return'?
kthread_create operation isn't necessary?
Yes kthread is optional. Some boards doenot have the ID pin hooked onto the
GPIO.
In such cases we will run the kthread and poll on the ID pin values.
+}
+
+dra7xx_usb-thread_task = kthread_create(id_poll_func, dra7xx_usb,
+ dev_name(pdev-dev));
Should you use polling method with kthread? I think it isn't proper method.
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.
I also prefer interrupt method. If any implementation does not have ID pin
connected to GPIOs then still
it could use the driver in polling mode.
As I mentioned above, I don't prefer interrupt method for cable detection.
I hope you meant, you prefer interrupt method for cable detection over
polling .
Right, my mistake. I prefer interrupt method.
Thanks,
Chanwoo CHoi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/16/2013 04:13 AM, George Cherian wrote:
Adding extcon driver for USB ID detection to dynamically
configure USB Host/Peripheral mode.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
+EXTCON FOR DRA7xx
+
+Required Properties:
Please at lest explain what a DRA7xxx is, and the purpose of the HW
module this binding describes.
+ - compatible : Should be ti,dra7xx-usb
If this is a USB VID detector rather than a full USB host controller,
then usb in the binding is a bit over-reaching. Perhaps -usb-vid or
-usb-vid-detector would be more accurate.
+ - gpios : phandle to ID pin and interrupt gpio.
This isn't just a phandle; it's phandle+args, or a GPIO specifier. Some
reference should be made to ../gpio/gpio.txt for the format.
Why does the interrupt line need to be included in a list of GPIOs?
If the DRA7xxx is a VID detector, why does it even need/have any GPIOs
associated with it; surely it has a dedicated VID input pin. Can you
provide more details re: how the HW is structured.
+Optional Properties:
+ - interrupt-parent : interrupt controller phandle
+ - interrupts : interrupt number
+
+
It's typical insert the following between those two blank lines:
Example:
... or delete one of the blank lines.
+dra7x_extcon1 {
+ compatible = ti,dra7xx-usb;
+ gpios = pcf_usb 1 0,
+ gpio6 11 2;
+ interrupt-parent = gpio6;
+ interrupts = 11;
+ };
+
No need for that trailing blank line.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
Hi George,
On 08/16/2013 07:13 PM, George Cherian wrote:
Adding extcon driver for USB ID detection to dynamically
configure USB Host/Peripheral mode.
Signed-off-by: George Cherian george.cher...@ti.com
---
.../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 000..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 {
You used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
What is meaning 'dra7x_extcon1'?
+ compatible = ti,dra7xx-usb;
+ gpios = pcf_usb 1 0,
+ gpio6 11 2;
+ interrupt-parent = gpio6;
+ interrupts = 11;
+ };
You have to keep indentation rule.
+
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.
You should explain detailed description about pcf8575 on patch description
and change description of EXTCON_DRA7xx as following:
using DRA7XX extcon - using DRA7XX device or using DRA7XX usb
+
+
Remove unnecessary blank line.
endif # MULTISTATE_SWITCH
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 000..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.cher...@ti.com
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I kis...@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 dra7xx_usb {
+ struct device *dev;
+
+ struct extcon_dev edev;
+ struct task_struct *thread_task;
+
+ /*GPIO pin */
Add space between /* and GPIO.
+ int id_gpio;
+ 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) {
+
