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 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 { >> 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 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. 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html