Re: [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Thu, 13 Aug 2026 12:33:01 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:

> Move the protocol validation and device creation logic in scmi_probe()
> into a reusable scmi_device_check_create() helper.
> 
> The helper centralizes checks for the protocol ID range, implementation
> availability and duplicate activation before invoking
> scmi_create_protocol_devices(). This preserves the existing behavior
> while allowing the logic to be reused by the ACPI path, where protocol
> child fwnodes are absent.
> 
> No functional change intended.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
A trivial thing and one follow on from earlier.  For that just be consistent
with what you choose to do for the earlier comment.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
>  drivers/firmware/arm_scmi/driver.c | 60 ++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index aad678db0f6e..951ba3df6ba6 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3244,6 +3244,40 @@ static void scmi_enable_matching_quirks(struct scmi_info *info)
>  			   rev->sub_vendor_id, rev->impl_ver);
>  }
>  
> +static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
> +				     struct scmi_info *info)
> +{
> +	int ret;
> +	struct device *dev = info->dev;
> +	struct scmi_handle *handle = &info->handle;
> +
> +	if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
> +		dev_err(dev, "Out of range protocol %d\n", prot_id);
> +		return;
> +	}
> +
> +	if (!scmi_is_protocol_implemented(handle, prot_id)) {
> +		dev_err(dev, "SCMI protocol %d not implemented\n",
> +			prot_id);

Fits on one line. I didn't check if you modify this later in the series
though so if this is churn reduction, fine to leave it as it stands.

> +		return;
> +	}
> +
> +	/*
> +	 * Save this valid fwnode protocol descriptor amongst
> +	 * @active_protocols for this SCMI instance.
> +	 */
> +	ret = idr_alloc(&info->active_protocols, fwnode,
> +			prot_id, prot_id + 1, GFP_KERNEL);
> +	if (ret != prot_id) {
> +		dev_err(dev, "SCMI protocol %d already activated. Skip\n",
> +			prot_id);
> +		return;
> +	}
> +
> +	fwnode_handle_get(fwnode);
> +	scmi_create_protocol_devices(fwnode, info, prot_id, NULL);

If you take the suggest I made earlier to make the hand over of ownership
slightly more obvious by using the return value of fwmonde_handle_get()
then applies here too.

> +}