Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 10/21/20 6:38 PM, Andy Shevchenko wrote:
> On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote:
>> On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote:
>>> In some cases the GpioInt() resource is coming with bias settings
>>> which may affect system functioning. Respect bias settings for
>>> GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API
>>> in acpi_dev_gpio_irq_get().
>>>
>>> While at it, refactor to configure flags first and, only when succeeded,
>>> map the IRQ descriptor.
> 
> ...
> 
>>> -			irq = gpiod_to_irq(desc);
>>> -			if (irq < 0)
>>> -				return irq;
>>> +			acpi_gpio_update_gpiod_flags(&dflags, &info);
>>> +			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
>>>  
>>>  			snprintf(label, sizeof(label), "GpioInt() %d", index);
>>> -			ret = gpiod_configure_flags(desc, label, lflags, info.flags);
>>> +			ret = gpiod_configure_flags(desc, label, lflags, dflags);
>>>  			if (ret < 0)
>>>  				return ret;
>>>  
>>> +			irq = gpiod_to_irq(desc);
>>> +			if (irq < 0)
>>> +				return irq;
>>
>> Should the above be undone if the conversion here fails?
> 
> But wouldn't it be not good if we changed direction, for example, and then
> change it back? (IRQ requires input, which is safer, right?)
> 
> This makes me think what gpiod_to_irq() may do for physical state of the pin.
> On the brief search it seems there is no side effect on the pin with that
> function, so, perhaps the original order has that in mind to not shuffle with
> line if mapping can't be established. But if setting flags fail, we may got
> into the state which is not equal to the initial one, right?
> 
> So, in either case I see no good way to roll back the physical pin state
> changes. But I can return ordering of the calls in next version.
> 
> What do you think?

I think it would be good to do a new version where you keep the original
ordering.

Also if you decide to keep the ordering change, that really should be
in a separate commit and not squashed into this one, so that e.g. a bisect
can determine the difference between the ordering change or the flags
changes causing any issues.

Regards,

Hans