This flag is added in D47239.

Can we check this object exists? I have a board that uses an _EVT method so will likely fail trying to call a method that doesn't exist.

Ah, I didn't have access to any hardware using the newer _EVT option. We should implement support for that (necessary for the pins > 255 case I flagged a few lines earlier as well) but without hardware I can't do that right now. Can you add that support?

Until we implement support for _EVT methods it's probably better to print an error when we fail to invoke a nonexistent _Exx method than to simply do nothing though?

I'd like to get this driver into the tree -- with or without _EVT method support -- in time for 14.2 because it's a major pain point for EC2.

Actually, "GPIO resources" works for both. Not sure why I thought it didn't :p
Nevermind.

Thanks!

I don't understand why this is necessary -- we reach here from AcpiWalkResources(handle, "_AEI", acpi_gpiobus_enumerate_aei, context); which is looking at resources under the provided handle, right?

If ACPI tells us to use flags which the GPIO controller's driver doesn't accept, it seems that failing to attach a driver is the right solution here. (Unless it's a case like in EC2 where we need a "the ACPI tables are buggy" quirk.)

Yes, but acpi_gpiobus_enumerate itself gets called once for every ACPI device we have, which can include multiple controllers. This is necessary for _CRS since it can appear anywhere.

Taking a look at the spec, it looks like _AEI can only appear under the GPIO controller it's referencing, so the AcpiWalkResources call can actually be moved to the attach function and use that handle directly. Note that nothing stops buggy ACPI implementations from having this outside of the controller's scope.

This object appears within the scope of the GPIO controller device whose pins are to be used as GPIO-signaled events.

ACPI Specification Release 6.5 - §5.6.5.2

Hmm, I assumed it was failing because changing the pin to either pull-up or pull-down wasn't supported (even if the pin is already a pull-up one).
Though, looking at the acpidump of this machine, it seems to be inconsistent when it comes to setting the pin config. And the example I gave (bytgpio) doesn't seem to have pins with varying pin configs either.

it seems that failing to attach a driver is the right solution here.

I'm not sure how common this inconsistency is, but I think failing is too harsh. Maybe we could output a warning instead?

Aha, I didn't realize that _CRS could appear anywhere. I've fixed the _AEI enumeration to run from acpi_gpiobus_attach.

If this turns out to be a problem later we can always change it later. I'd prefer to "fail safe" by not attaching the device rather than enabling something which might not do what is intended. (For example, in EC2 the _AEI is used to signal a control plane shutdown request; if that got accidentally asserted we could end up with instances shutting down immediately after booting, a situation which would be rather difficult for users to fix.)

I have a server where the _AEI data has the GPIOs a pull-up, however this is a pl061 that doesn't support that so we fail adding the child here.

Could we have this return a list of pins, and have an ivar with the size? I would like to rework the driver to support that in a single gpio_aei device, but am unlikely to have time for 14.2 so would like to at least have that as a future option.

I don't think this is a useful test. It will always be gpio_aei because we've set the devclass to do the probe.
You have to rely on the bus driver to only add this when it's valid.

How do we know pin isn't NULL?

We specified that we're a gpio_aei device hanging off a gpiobus, but a future gpiobus might add children with a devclass of NULL in which case the matching logic would still call our probe routine, yes? So I wanted to make sure we only matched when the parent bus said explicitly that the child device is a gpio_aei.

Because any parent bus which creates a gpio_aei child guarantees that pin won't be NULL. (Mechanically: Because pin is a pointer into the ivars structure.)

Ah, that's the same problem as Amazon has. You can set debug.acpi.quirks="8" to work around the ACPI bug.

Oh, I thought it was cleaner to have a device per _AEI. In EC2 there's two, but they invoke different ACPI "buttons"; we have acpi_button0 and acpi_button1 so I figured it made sense to have gpio_aei0 and gpio_aei1 which invoked them.

But the devclass is set just before probe is called. So this will never do anything.

Aha, now I understand. Fixed.

Having a look again I don't think this should exist as gpiobus.c already stores the pin information.

We could add gpio_pin_get_by_idx to get the pin similar to how we already do it in ofw_gpiobus.c

gpiobus.c stores the pin number but not the flags. And we can't even use gpio_pin_get_by_child_index to get a gpio_pin_t structure without the flags, because using that results in gpiobus_acquire_pin being called twice (which fails).

Why M_NOWAIT? Can we not sleep here?

In ofw_gpiobus.c we populate the pin list directly in ofw_gpiobus_setup_devinfo so don't see the acquire issue. I'm not sure why @avg added GPIOBUS_IVAR_PINS in D20459, id doesn't seem to be used.

Could gpio_pin_get_by_bus_pinnum call GPIO_PIN_GETFLAGS to set the flags?

That's pure cargo culting; gpiobus_add_child uses M_NOWAIT and I wasn't sure why so I copied it.

That said, boot-time device probing happens early enough in the boot process that if we're already failing to allocate memory we're probably pretty much screwed; we can't swap and I'm not sure if we even have other threads running yet.

Yeah, we could set the pin information directly. I figured that since there was an API for doing it we were supposed to use it. ;-)

And no we can't use GPIO_PIN_GETFLAGS; that gets passed down to the driver which reports back *some* of the flags but not all of them.

I've pushed a change on top of this to add and use gpio_pin_get_by_acpi_index to be consistent with other gpiobus code to https://github.com/zxombie/freebsd/commit/8c3f8aba9c616dd1f0e1e3aaf76fe9630b616e00.

That looks fine to me aside from possibly moving a declaration into acpi_gpiobusvar.h (https://github.com/zxombie/freebsd/commit/8c3f8aba9c616dd1f0e1e3aaf76fe9630b616e00#r148595677). I assume the for (i = 0; i < npins; i++) loops with npins hard-coded to 1 are because it might not be fixed at 1 in the future.

Are you ok with pushing this into the src tree as two commits?

There could be more than one pin per GpioInt, but as gpio_aei only supports a single event I limited it to 1 for now.

Regarding GPIOBUS_IVAR_PINS, it's been a long time but my recollection is that it was for a kind of a "platform" driver.
The machine was x86 and ACPI based, but there was no advertisement for its GPIO capabilities in ACPI.
And, if I recall correctly, ACPI GPIO support was kind of very new at the time.
So, the idea was that the platform driver would configure pins for child devices on a gpio bus based on the external knowledge (hardcoded).

Then later, a new version of firmware / BIOS for that machine came out and it had ACPIO GPIO information for the pins of interest.
So, there was no need for the "platform driver".

So, that ivar is indeed unused, but theoretically it can be useful for cases where we know that there is something on a GPIO, but the platform does not advertise it and it is not easy to add an overlay (i.e., it's not FDT or alike).

FWIW, D26311 is what I came up when the firmware started advertising GPIO.

I think that gpioled_acpi should be much easier and cleaner to implement now than it was back then.