Re: [PATCH] gpiolib: acpi: Only trigger ActiveBoth interrupts on boot

[Hans de Goede <hansg@kernel.org>]

Hi,

On 29-Apr-26 12:24, Andy Shevchenko wrote:
> On Wed, Apr 29, 2026 at 11:48:13AM +0200, Hans de Goede wrote:
>> On 29-Apr-26 04:52, Mario Limonciello wrote:
> 
> ...
> 
>>> +	/*
>>> +	 * Make sure we trigger the initial state of ActiveBoth IRQs.
>>> +	 *
>>> +	 * According to the Microsoft GPIO documentation, triggering GPIO
>>> +	 * interrupts marked as ActiveBoth during initialization is correct
>>> +	 * as long as the associated GPIO line is already "asserted"
>>> +	 * (logic level low). We should not trigger edge-based GPIO
>>> +	 * interrupts not marked as ActiveBoth.
>>> +	 *
>>> +	 * See: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/general-purpose-i-o--gpio-
>>> +	 * Section: "GPIO controllers and ActiveBoth interrupts"
>>> +	 */
>>>  	if (acpi_gpio_need_run_edge_events_on_boot() &&
>>> -	    (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>>> +	    ((event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) ==
>>> +	     (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>>>  		value = gpiod_get_raw_value_cansleep(event->desc);
>>> -		if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>>> -		    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>>> +		if (value == 0)
>>>  			event->handler(event->irq, event);
>>>  	}
>>>  }
>>
>> One nitpick, which can be a follow-up patch since Andy has already picked this
>> one up.
>>
>> I think that now that the second if condition has been simplified to just
>> value == 0, it can be added to the first if as " && value == 0" dropping
>> the nested if.
> 
> But we need to get that value first (unconditionally!). I think it wouldn't
> be desirable change.

Ah right, I read over the line getting the value I thought this was already
done, never mind:

FWIW:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans