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

[Chanwoo Choi <cw00.choi@samsung.com>]

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@linux.intel.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.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