linux-kernel - Re: [PATCH v2] extcon: int3496: Set the id pin to direction-input if necessary

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <58C67C9F.7010704@samsung.com>
Date:   Mon, 13 Mar 2017 20:03:59 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chen-Yu Tsai <wens@...e.org>
Cc:     linux-kernel@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v2] extcon: int3496: Set the id pin to direction-input if
 necessary

On 2017년 03월 13일 19:51, Hans de Goede wrote:
> Hi,
> 
> On 13-03-17 11:30, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2017년 03월 13일 17:40, Hans de Goede wrote:
>>> With the new more strict ACPI gpio code the dsdt's IoRestriction
>>> flags are honored on gpiod_get, but in some dsdt's it is wrong,
>>> so explicitly call gpiod_direction_input on the id gpio if
>>> necessary.
>>>
>>> This fixes the following errors when the int3496 code is used
>>> together with the new more strict ACPI gpio code:
>>>
>>> [ 2382.484415] gpio gpiochip1: (INT33FF:01): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
>>> [ 2382.484425] gpio gpiochip1: (INT33FF:01): unable to lock HW IRQ 3 for IRQ
>>> [ 2382.484429] genirq: Failed to request resources for INT3496:00 (irq 174) on irqchip chv-gpio
>>> [ 2382.484518] intel-int3496 INT3496:00: can't request IRQ for USB ID GPIO: -22
>>> [ 2382.500359] intel-int3496: probe of INT3496:00 failed with error -22
>>>
>>> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>> ---
>>> Changes in v2:
>>> -Warn about firmware bug when the dsdt's IoRestriction does not allow input
>>> ---
>>>  drivers/extcon/extcon-intel-int3496.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-intel-int3496.c b/drivers/extcon/extcon-intel-int3496.c
>>> index b8ac947..18801eb 100644
>>> --- a/drivers/extcon/extcon-intel-int3496.c
>>> +++ b/drivers/extcon/extcon-intel-int3496.c
>>> @@ -113,6 +113,10 @@ static int int3496_probe(struct platform_device *pdev)
>>>          dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
>>>          return ret;
>>>      }
>>
>> Need to add one blank line at here.
> 
> I grouped it together with the earlier error check since it is still
> checking the returned gpiodesc is valid, but if you prefer to add a line
> that is fine by me.

If you want to group them, you should add it as following:

   } else if (gpiod_get_direction(data->gpio_usb_id) != GPIOF_DIR_IN) {

> 
>>
>>> +    if (gpiod_get_direction(data->gpio_usb_id) != GPIOF_DIR_IN) {
>>> +        dev_warn(dev, "firmware bug USB ID GPIO not in input mode, fixing\n");
>>
>> The length of warning comment is over 80 char.
> 
> Which is allowed, try running checkpatch.pl on the patch. log messages may
> cross the 80 char limit, to avoid splitting them over multiple lines which
> would make grepping for them hard.
> 
>> We need to reduce the length of comment.
> 
> Nope, not needed, as said this is allowed.
> 
>> I modify the comment as following: If you ok, I'll apply it.
>>     "ID pin isn't in input mode due to firmware bug"
> 
> I prefer my original text which better describes what is happening
> and as said before, the text going over the 80 char limit is allowed.

Right. The over the 80 char is possible. But, I prefer to write
the comment under the 80 char. But if you want to use more detailed
comment, I think that you can make your sentence more formal.

[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics