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

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

[Hans de Goede]

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;
 	}
+	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");
+		gpiod_direction_input(data->gpio_usb_id);
+	}
 
 	data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
 	if (data->usb_id_irq < 0) {
-- 
2.9.3

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

[Chanwoo Choi]

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.

> +	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.
We need to reduce the length of comment.

I modify the comment as following: If you ok, I'll apply it. 
	"ID pin isn't in input mode due to firmware bug"

Or if you make new comment under 80 char, please send v3 patch.

> +		gpiod_direction_input(data->gpio_usb_id);
> +	}
>  
>  	data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
>  	if (data->usb_id_irq < 0) {
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Hans de Goede]

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 (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.

Regards,

Hans



>
> Or if you make new comment under 80 char, please send v3 patch.
>
>> +		gpiod_direction_input(data->gpio_usb_id);
>> +	}
>>
>>  	data->usb_id_irq = gpiod_to_irq(data->gpio_usb_id);
>>  	if (data->usb_id_irq < 0) {
>>
>

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

[Chanwoo Choi]

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

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

[Andy Shevchenko]

On Mon, Mar 13, 2017 at 10:40 AM, Hans de Goede <hdegoede@redhat.com> 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

> +               dev_warn(dev, "firmware bug USB ID GPIO not in input mode, fixing\n");

Please, use FW_BUG macro.

-- 
With Best Regards,
Andy Shevchenko