| Message ID | 20231226192149.1830592-1-markhas@chromium.org |
|---|---|
| Headers | show |
| Series | Improve IRQ wake capability reporting and update the cros_ec driver to use it | expand |
On Tue, Dec 26, 2023 at 12:21:06PM -0700, Mark Hasemeyer wrote: > Other information besides wake capability can be provided about GPIO > IRQs such as triggering, polarity, and sharability. Use resource flags > to provide this information to the caller if they want it. > > This should keep the API more robust over time as flags are added, > modified, or removed. It also more closely matches acpi_irq_get() which > take a resource as an argument. > > Rename the function to acpi_dev_get_gpio_irq_resource() to better > describe the function's new behavior. ... > + res_flags = acpi_dev_irq_flags(info.triggering, info.polarity, > + info.shareable, info.wake_capable); Broken indentation of the second line. ... > + *r = DEFINE_RES_NAMED(irq, 1, NULL, res_flags); So? The whole exercise with the first patch is to have here: *r = DEFINE_RES_IRQ_NAMED_FLAGS(irq, NULL, res_flags); ... > + struct resource r; I prefer to see struct resource r = {}; even if it makes no difference. This allows to have robust code. > + ret = acpi_dev_get_gpio_irq_resource(adev, name, index, &r); > + return ret ?: r.start; Btw, this function requires header to include ioport.h. I'm not sure if it's good for ACPI. I would prefer safest approach, i.e. exporting this from a C code, i.e. gpiolib-acpi.c.
> > > + *r = DEFINE_RES_NAMED(irq, 1, NULL, res_flags); > > So? The whole exercise with the first patch is to have here: > > *r = DEFINE_RES_IRQ_NAMED_FLAGS(irq, NULL, res_flags); Thanks. I was staring at the macro changes in ioport.h for too long... > > > + ret = acpi_dev_get_gpio_irq_resource(adev, name, index, &r); > > + return ret ?: r.start; > > Btw, this function requires header to include ioport.h. I'm not sure > if it's good for ACPI. I would prefer safest approach, i.e. exporting > this from a C code, i.e. gpiolib-acpi.c. It actually just needs 'struct resource' declared. I removed the dependency on ioport.h, but it may make sense to leave the definition of 'acpi_dev_gpio_irq_get_by()' here because non ACPI based systems need a default implementation anyway.
On Tue Dec 26, 2023 at 8:21 PM CET, Mark Hasemeyer wrote: > Currently the cros_ec driver assumes that its associated interrupt is > wake capable. This is an incorrect assumption as some Chromebooks use a > separate wake pin, while others overload the interrupt for wake and IO. > This patch train updates the driver to query the underlying ACPI/DT data > to determine whether or not the IRQ should be enabled for wake. > > Both the device tree and ACPI systems have methods for reporting IRQ > wake capability. In device tree based systems, a node can advertise > itself as a 'wakeup-source'. In ACPI based systems, GpioInt and > Interrupt resource descriptors can use the 'SharedAndWake' or > 'ExclusiveAndWake' share types. > > Some logic is added to the platform, ACPI, and DT subsystems to more > easily pipe wakeirq information up to the driver. > > Changes in v3: > -Rebase on linux-next > -See each patch for patch specific changes > > Changes in v2: > -Rebase on linux-next > -Add cover letter > -See each patch for patch specific changes > > Mark Hasemeyer (24): [...] > ARM: dts: tegra: Enable cros-ec-spi as wake source [...] > arm64: dts: tegra: Enable cros-ec-spi as wake source [...] Both patches applied, thanks. Thierry