| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Aug 2013 18:54:42 +0530
From: George Cherian <george.cherian@...com>
To: Chanwoo Choi <cw00.choi@...sung.com>
CC: <balbi@...com>, <myungjoo.ham@...sung.com>,
<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <grant.likely@...aro.org>,
<rob@...dley.net>, <ian.campbell@...rix.com>,
<swarren@...dotorg.org>, <mark.rutland@....com>,
<pawel.moll@....com>, <rob.herring@...xeda.com>,
<linux-omap@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<bcousson@...libre.com>
Subject: 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 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.
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 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 .
> 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
>
>
--
-George
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists