Re: [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core

On Thu, 13 Aug 2026 12:32:58 +0100
Sudeep Holla <[email protected]> wrote:

> Switch SCMI core plumbing from struct device_node * to struct
> fwnode_handle * so the core can describe SCMI instances and protocols using
> firmware nodes rather than OF nodes directly.
> 
> This change:
>   - Replaces core OF property lookups with fwnode_property_*() helpers.
>   - Switches child enumeration to
>     fwnode_for_each_available_child_node_scoped().

Why the scoped version? See inline for more on this.  Probably just needs
a comment here (or use the non _scoped variant)

>   - Plumbs fwnode through the SCMI device creation and channel setup paths.
>   - Updates transport ->chan_available() callbacks to take a fwnode.
>   - Stores per-protocol child fwnodes in info->active_protocols so the core
>     can later locate the descriptor for a given protocol ID.
>   - Updates mailbox/optee/smc/virtio transports to accept fwnodes and map
>     back to OF nodes where their existing parsing remains DT-specific.
> 
> DT-only transports such as mailbox, OP-TEE and SMC still parse DT
> properties by mapping the fwnode back to an OF node. On non-DT systems
> these transports report no channel available.
> 
> This is a mechanical step towards firmware-node neutrality and prepares the
> SCMI core for non-DT transports, such as an ACPI/PCC transport. DT users
> continue to work unchanged; no non-DT transport is enabled by this patch.
> 
> Signed-off-by: Sudeep Holla <[email protected]>

A few minor things - with those cleaned up.  Only the cleanup.h one really
matters.

Reviewed-by: Jonathan Cameron <[email protected]>

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index ef29fd223287..9014723e0f7f 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c

...

> @@ -3352,10 +3354,10 @@ static int scmi_probe(struct platform_device *pdev)
>  
>  	scmi_enable_matching_quirks(info);
>  
> -	for_each_available_child_of_node(np, child) {
> +	fwnode_for_each_available_child_node_scoped(dev_fwnode(dev), child) {

There are no early exits yet and I can't see any added later in the series.
So the scoped bit is irrelevant.  I don't mind the change on basis of
hardening or similar but it needs a comment in the patch description to 
justify that.

>  		u32 prot_id;
>  
> -		if (of_property_read_u32(child, "reg", &prot_id))
> +		if (fwnode_property_read_u32(child, "reg", &prot_id))
>  			continue;
>  
>  		if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
> @@ -3370,8 +3372,8 @@ static int scmi_probe(struct platform_device *pdev)
>  		}
>  
>  		/*
> -		 * Save this valid DT protocol descriptor amongst
> -		 * @active_protocols for this SCMI instance/
> +		 * Save this valid fwnode protocol descriptor amongst
> +		 * @active_protocols for this SCMI instance.
>  		 */
>  		ret = idr_alloc(&info->active_protocols, child,
>  				prot_id, prot_id + 1, GFP_KERNEL);
> @@ -3381,7 +3383,7 @@ static int scmi_probe(struct platform_device *pdev)
>  			continue;
>  		}
>  
> -		of_node_get(child);
> +		fwnode_handle_get(child);
>  		scmi_create_protocol_devices(child, info, prot_id, NULL);

Give this is handing over ownership I'd make that explicit and do

		scmi_create_protcol_devices(fwnode_handle_get(child), info,
					    prot_id, NULL);
but that is a slightly unrelated change - so I don't mind if you don't change
this one.

>  	}
>  
...



> diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
> index 1fce3ccdeb7f..1079cd01190b 100644
> --- a/drivers/firmware/arm_scmi/transports/smc.c
> +++ b/drivers/firmware/arm_scmi/transports/smc.c
> @@ -84,10 +84,15 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static bool smc_chan_available(struct device_node *of_node, int idx)
> +static bool smc_chan_available(struct fwnode_handle *fwnode, int idx)
>  {
> -	struct device_node *np __free(device_node) =
> -					of_parse_phandle(of_node, "shmem", 0);
> +	struct device_node *of_node = to_of_node(fwnode);
> +	struct device_node *np __free(device_node) = NULL;

See guidance in cleanup.h.  Basically says constructor and destructor
must be together (that was after some grumpy replies from Linus to early
cleanup.h users!)

> +
> +	if (!of_node)
> +		return false;
> +
> +	np = of_parse_phandle(of_node, "shmem", 0);
	struct device_node *np __free(device_node) =
		of_parse_phandle(of_node, "shmem", 0);

And don't worry about the inline declaration - this case is specifically
allowed.

>  	if (!np)
>  		return false;