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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> > 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) {