| Message ID | 20220912160931.v2.6.I8092e417a8152475d13d8d638eb4c5d8ea12ac7b@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq | expand |
On Mon, Sep 12, 2022 at 04:13:10PM -0600, Raul E Rangel wrote: > ACPI IRQ/Interrupt resources contain a bit that describes if the > interrupt should wake the system. This change exposes that bit via > a new IORESOURCE_IRQ_WAKECAPABLE flag. Drivers should check this flag > before arming an IRQ to wake the system. ... > static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode, > u32 hwirq, u8 triggering, > u8 polarity, u8 shareable, > + u8 wake_capable, > struct acpi_irq_parse_one_ctx *ctx) This function is used only in scope of a single C-file. Why instead not converting it to use some internal structure and acpi_irq_parse_one_cb() becomes like: struct internal_struct s; switch (ares->type) { case ACPI_RESOURCE_TYPE_IRQ: ...fill internal_struct... break; case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: ...fill internal_struct... break; default: return AE_OK; acpi_irq_parse_one_match(&s); return AE_CTRL_TERMINATE; ? ... > + acpi_dev_get_irqresource(res, > + ext_irq->interrupts[index], > + ext_irq->triggering, > + ext_irq->polarity, > + ext_irq->shareable, > + ext_irq->wake_capable, false); Ditto. Actually it can be shared structure for these too.
On Tue, Sep 13, 2022 at 08:21:15PM +0300, Andy Shevchenko wrote: > On Mon, Sep 12, 2022 at 04:13:10PM -0600, Raul E Rangel wrote: > > ACPI IRQ/Interrupt resources contain a bit that describes if the > > interrupt should wake the system. This change exposes that bit via > > a new IORESOURCE_IRQ_WAKECAPABLE flag. Drivers should check this flag > > before arming an IRQ to wake the system. > > ... > > > static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode, > > u32 hwirq, u8 triggering, > > u8 polarity, u8 shareable, > > + u8 wake_capable, > > struct acpi_irq_parse_one_ctx *ctx) > > This function is used only in scope of a single C-file. Why instead not > converting it to use some internal structure and acpi_irq_parse_one_cb() > becomes like: > > struct internal_struct s; > > switch (ares->type) { > case ACPI_RESOURCE_TYPE_IRQ: > ...fill internal_struct... > break; > case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > ...fill internal_struct... > break; > default: > return AE_OK; > > acpi_irq_parse_one_match(&s); > return AE_CTRL_TERMINATE; > > ? > > ... > > > + acpi_dev_get_irqresource(res, > > + ext_irq->interrupts[index], > > + ext_irq->triggering, > > + ext_irq->polarity, > > + ext_irq->shareable, > > + ext_irq->wake_capable, false); > > Ditto. > > Actually it can be shared structure for these too. > I tried your suggestion, but I honestly think it hurts readability. It's also a little scary because the compiler doesn't guarantee all the members of the struct are filled out, unlike having the explicit parameters. Here is the patch: diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c index 5483cf9a28e3a0..8549ccefa5d03c 100644 --- a/drivers/acpi/irq.c +++ b/drivers/acpi/irq.c @@ -139,35 +139,37 @@ struct acpi_irq_parse_one_ctx { struct irq_fwspec *fwspec; }; +struct acpi_irq_parse_one_match_params { + struct fwnode_handle *fwnode; + u32 hwirq; + u8 triggering; + u8 polarity; + u8 shareable; + u8 wake_capable; +} + /** - * acpi_irq_parse_one_match - Handle a matching IRQ resource. - * @fwnode: matching fwnode - * @hwirq: hardware IRQ number - * @triggering: triggering attributes of hwirq - * @polarity: polarity attributes of hwirq - * @polarity: polarity attributes of hwirq - * @shareable: shareable attributes of hwirq - * @wake_capable: wake capable attribute of hwirq + * acpi_irq_parse_update_ctx - Handle a matching IRQ resource. + * @params: IRQ parameters * @ctx: acpi_irq_parse_one_ctx updated by this function * * Description: - * Handle a matching IRQ resource by populating the given ctx with - * the information passed. + * Update the given ctx with the IRQ information passed. */ -static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode, - u32 hwirq, u8 triggering, - u8 polarity, u8 shareable, - u8 wake_capable, - struct acpi_irq_parse_one_ctx *ctx) +static inline void +acpi_irq_parse_update_ctx(const struct parse_one_match *params, + struct acpi_irq_parse_one_ctx *ctx) { - if (!fwnode) + if (!params->fwnode) return; ctx->rc = 0; - *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable, - wake_capable); - ctx->fwspec->fwnode = fwnode; - ctx->fwspec->param[0] = hwirq; - ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity); + *ctx->res_flags = + acpi_dev_irq_flags(params->triggering, params->polarity, + params->shareable, params->wake_capable); + ctx->fwspec->fwnode = params->fwnode; + ctx->fwspec->param[0] = params->hwirq; + ctx->fwspec->param[1] = + acpi_dev_get_irq_type(params->triggering, params->polarity); ctx->fwspec->param_count = 2; } @@ -182,7 +184,7 @@ static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode, * might contain multiple interrupts we check if the index is within this * one's interrupt array, otherwise we subtract the current resource IRQ * count from the lookup index to prepare for the next resource. - * Once a match is found we call acpi_irq_parse_one_match to populate + * Once a match is found we call acpi_irq_parse_update_ctx to populate * the result and end the walk by returning AE_CTRL_TERMINATE. * * Return: @@ -195,7 +197,7 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares, struct acpi_irq_parse_one_ctx *ctx = context; struct acpi_resource_irq *irq; struct acpi_resource_extended_irq *eirq; - struct fwnode_handle *fwnode; + struct acpi_irq_parse_one_match_params params = {0}; switch (ares->type) { case ACPI_RESOURCE_TYPE_IRQ: @@ -204,11 +206,13 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares, ctx->index -= irq->interrupt_count; return AE_OK; } - fwnode = acpi_get_gsi_domain_id(irq->interrupts[ctx->index]); - acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index], - irq->triggering, irq->polarity, - irq->shareable, irq->wake_capable, - ctx); + params.fwnode = + acpi_get_gsi_domain_id(irq->interrupts[ctx->index]); + params.hwirq = irq->interrupts[ctx->index]; + params.triggering = irq->triggering; + params.polarity = irq->polarity; + params.shareable = irq->shareable; + params.wake_capable = irq->wake_capable; return AE_CTRL_TERMINATE; case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: eirq = &ares->data.extended_irq; @@ -218,16 +222,22 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares, ctx->index -= eirq->interrupt_count; return AE_OK; } - fwnode = acpi_get_irq_source_fwhandle(&eirq->resource_source, - eirq->interrupts[ctx->index]); - acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index], - eirq->triggering, eirq->polarity, - eirq->shareable, eirq->wake_capable, - ctx); - return AE_CTRL_TERMINATE; + params.fwnode = acpi_get_irq_source_fwhandle( + &eirq->resource_source, eirq->interrupts[ctx->index]); + params.hwirq = eirq->interrupts[ctx->index]; + params.triggering = eirq->triggering; + params.polarity = eirq->polarity; + params.shareable = eirq->shareable; + params.wake_capable = eirq->wake_capable; + + default: + return AE_OK; } - return AE_OK; + acpi_irq_parse_update_ctx(¶ms, ctx); + + return AE_CTRL_TERMINATE; + } /**
diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c index dabe45eba055d1..5483cf9a28e3a0 100644 --- a/drivers/acpi/irq.c +++ b/drivers/acpi/irq.c @@ -147,6 +147,7 @@ struct acpi_irq_parse_one_ctx { * @polarity: polarity attributes of hwirq * @polarity: polarity attributes of hwirq * @shareable: shareable attributes of hwirq + * @wake_capable: wake capable attribute of hwirq * @ctx: acpi_irq_parse_one_ctx updated by this function * * Description: @@ -156,12 +157,14 @@ struct acpi_irq_parse_one_ctx { static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode, u32 hwirq, u8 triggering, u8 polarity, u8 shareable, + u8 wake_capable, struct acpi_irq_parse_one_ctx *ctx) { if (!fwnode) return; ctx->rc = 0; - *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable); + *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable, + wake_capable); ctx->fwspec->fwnode = fwnode; ctx->fwspec->param[0] = hwirq; ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity); @@ -204,7 +207,8 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares, fwnode = acpi_get_gsi_domain_id(irq->interrupts[ctx->index]); acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index], irq->triggering, irq->polarity, - irq->shareable, ctx); + irq->shareable, irq->wake_capable, + ctx); return AE_CTRL_TERMINATE; case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: eirq = &ares->data.extended_irq; @@ -218,7 +222,8 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares, eirq->interrupts[ctx->index]); acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index], eirq->triggering, eirq->polarity, - eirq->shareable, ctx); + eirq->shareable, eirq->wake_capable, + ctx); return AE_CTRL_TERMINATE; } diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 510cdec375c4d8..6a1c008a348902 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -336,8 +336,10 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_ext_address_space); * @triggering: Triggering type as provided by ACPI. * @polarity: Interrupt polarity as provided by ACPI. * @shareable: Whether or not the interrupt is shareable. + * @wake_capable: Wake capability as provided by ACPI. */ -unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable) +unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable, + u8 wake_capable) { unsigned long flags; @@ -351,6 +353,9 @@ unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable) if (shareable == ACPI_SHARED) flags |= IORESOURCE_IRQ_SHAREABLE; + if (wake_capable == ACPI_WAKE_CAPABLE) + flags |= IORESOURCE_IRQ_WAKECAPABLE; + return flags | IORESOURCE_IRQ; } EXPORT_SYMBOL_GPL(acpi_dev_irq_flags); @@ -442,7 +447,7 @@ static bool acpi_dev_irq_override(u32 gsi, u8 triggering, u8 polarity, static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, u8 triggering, u8 polarity, u8 shareable, - bool check_override) + u8 wake_capable, bool check_override) { int irq, p, t; @@ -475,7 +480,8 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, } } - res->flags = acpi_dev_irq_flags(triggering, polarity, shareable); + res->flags = acpi_dev_irq_flags(triggering, polarity, shareable, + wake_capable); irq = acpi_register_gsi(NULL, gsi, triggering, polarity); if (irq >= 0) { res->start = irq; @@ -523,7 +529,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, } acpi_dev_get_irqresource(res, irq->interrupts[index], irq->triggering, irq->polarity, - irq->shareable, true); + irq->shareable, irq->wake_capable, + true); break; case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: ext_irq = &ares->data.extended_irq; @@ -532,9 +539,12 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, return false; } if (is_gsi(ext_irq)) - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], - ext_irq->triggering, ext_irq->polarity, - ext_irq->shareable, false); + acpi_dev_get_irqresource(res, + ext_irq->interrupts[index], + ext_irq->triggering, + ext_irq->polarity, + ext_irq->shareable, + ext_irq->wake_capable, false); else irqresource_disabled(res, 0); break; diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c index da78dc77aed32e..55b28fc0a94042 100644 --- a/drivers/pnp/pnpacpi/rsparser.c +++ b/drivers/pnp/pnpacpi/rsparser.c @@ -206,7 +206,8 @@ static acpi_status pnpacpi_allocated_resource(struct acpi_resource *res, if (i >= 0) { flags = acpi_dev_irq_flags(gpio->triggering, gpio->polarity, - gpio->shareable); + gpio->shareable, + gpio->wake_capable); } else { flags = IORESOURCE_DISABLED; } @@ -315,7 +316,8 @@ static __init void pnpacpi_parse_irq_option(struct pnp_dev *dev, if (p->interrupts[i]) __set_bit(p->interrupts[i], map.bits); - flags = acpi_dev_irq_flags(p->triggering, p->polarity, p->shareable); + flags = acpi_dev_irq_flags(p->triggering, p->polarity, p->shareable, + p->wake_capable); pnp_register_irq_resource(dev, option_flags, &map, flags); } @@ -339,7 +341,8 @@ static __init void pnpacpi_parse_ext_irq_option(struct pnp_dev *dev, } } - flags = acpi_dev_irq_flags(p->triggering, p->polarity, p->shareable); + flags = acpi_dev_irq_flags(p->triggering, p->polarity, p->shareable, + p->wake_capable); pnp_register_irq_resource(dev, option_flags, &map, flags); } diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 7ee946758c5bcc..10ecf935e59431 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -495,7 +495,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, struct resource_win *win); bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, struct resource_win *win); -unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable); +unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable, + u8 wake_capable); unsigned int acpi_dev_get_irq_type(int triggering, int polarity); bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, struct resource *res); diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 616b683563a970..3baeea4d903bfd 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -79,7 +79,8 @@ struct resource { #define IORESOURCE_IRQ_HIGHLEVEL (1<<2) #define IORESOURCE_IRQ_LOWLEVEL (1<<3) #define IORESOURCE_IRQ_SHAREABLE (1<<4) -#define IORESOURCE_IRQ_OPTIONAL (1<<5) +#define IORESOURCE_IRQ_OPTIONAL (1<<5) +#define IORESOURCE_IRQ_WAKECAPABLE (1<<6) /* PnP DMA specific bits (IORESOURCE_BITS) */ #define IORESOURCE_DMA_TYPE_MASK (3<<0)
ACPI IRQ/Interrupt resources contain a bit that describes if the interrupt should wake the system. This change exposes that bit via a new IORESOURCE_IRQ_WAKECAPABLE flag. Drivers should check this flag before arming an IRQ to wake the system. Signed-off-by: Raul E Rangel <rrangel@chromium.org> --- Changes in v2: - Added ability to extract wake bit from Interrupt/IRQ resources drivers/acpi/irq.c | 11 ++++++++--- drivers/acpi/resource.c | 24 +++++++++++++++++------- drivers/pnp/pnpacpi/rsparser.c | 9 ++++++--- include/linux/acpi.h | 3 ++- include/linux/ioport.h | 3 ++- 5 files changed, 35 insertions(+), 15 deletions(-)