Re: [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core
From: Jonathan Cameron <hidden>
Date: 2026-08-24 17:39:41
Also in:
arm-scmi
On Thu, 13 Aug 2026 12:32:58 +0100 Sudeep Holla [off-list ref] 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 <sudeep.holla@kernel.org>A few minor things - with those cleaned up. Only the cleanup.h one really matters. Reviewed-by: Jonathan Cameron <redacted>
quoted hunk ↗ jump to hunk
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
...
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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.
}
...
quoted hunk ↗ jump to hunk
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;