Hi George,
extcon naming means various external connector device. e.g., usb, jack, etc...I will rename it to dra7xx_extcon.diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txtYou used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
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'?
So, I prefer 'dra7xx_usb' instead of 'dra7xx_extcon'. I have plan to divide
extcon device driver according to the kind of device.
OK.In dra7xx only ID pin is connected to the SoC gpio. There is no way, currently to detect the VBUS on/off.+static int id_poll_func(void *data)Should dra7xx_usb keep always connected state with either USB or USB-HOST cable?
+{
+ 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.
So I always default to either HOST/DEVICE mode solely depending on the ID pin value.
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.
You should define some constant variable to clarify '0' meaning instead of using '0' directly.+ 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) {
ok.devm_kzalloc itself should give some message.+ dra7xx_usb = devm_kzalloc(&pdev->dev, sizeof(*dra7xx_usb), GFP_KERNEL);You have to add error message with dev_err().
+ if (!dra7xx_usb)
+ return -ENOMEM;
ditto.okay+ 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;
+ }
+
+ dra7xx_usb->id_prev = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
+ if (dra7xx_usb->id_prev) {
You should define some constant variable to clarify 'dra7xx_usb->id_prev' meaning.
As I mentioned above, I don't prefer interrupt method for cable detection.There is no way, currently to detect the VBUS on/off.+ extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", false);why did you do keep always connected state?
+ 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);
+ }
So I always default to either HOST/DEVICE mode solely depending on the ID pin value.
If interrupt fails I fallback to polling thread.+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);
Yes kthread is optional. Some boards doenot have the ID pin hooked onto the GPIO.+ elseIf devm_request_threaded_irq() return success state, why did you directly call 'return'?
+ return 0;
kthread_create operation isn't necessary?
In such cases we will run the kthread and poll on the ID pin values.
I also prefer interrupt method. If any implementation does not have ID pin connected to GPIOs then still+ }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.
it could use the driver in polling mode.
Polling method for detection isn't appropriate for extcon device driver.
Instead, I will consider whether to support polling method or not on extcon core.
Thanks,
Chanwoo Choi