Re: [PATCH v2 06/13] ACPI: resources: Add wake_capable parameter to acpi_dev_irq_flags

[Raul E Rangel <rrangel@chromium.org>]

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(&params, ctx);
+
+	return AE_CTRL_TERMINATE;
+
 }
 
 /**