Re: [PATCH v2] firmware: arm_scmi: Delete the meaningless scmi_bus_id.

[Dan Carpenter <dan.carpenter@linaro.org>]

On Mon, Dec 16, 2024 at 03:37:45PM +0800, guomin_chen@sina.com wrote:
> From: guomin chen <guomin_chen@sina.com>
> 
> Currently, scmi_bus_id is only used to set scmi_dev.id,
> which in turn sets the SCMI device name. After removing
> scmi_bus_id, it is clearer and more meaningful to directly
> use the unique tuple [Parent name,device name, protocol] to 
> set the SCMI device name.
> 
> Signed-off-by: guomin chen <guomin_chen@sina.com>
> ---
>  drivers/firmware/arm_scmi/bus.c    | 17 +++--------------
>  drivers/firmware/arm_scmi/driver.c |  4 ++--
>  2 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 157172a5f2b5..800e8ec9357c 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -20,7 +20,6 @@
>  BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
>  EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
>  
> -static DEFINE_IDA(scmi_bus_id);
>  

You'll want to delet the blank line to avoid a checkpatch warning.
(checkpatch doesn't warn about the patch, but if you re-run it on
the file later, it will warn).

>  static DEFINE_IDR(scmi_requested_devices);
>  /* Protect access to scmi_requested_devices */
> @@ -341,7 +340,6 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
>  	if (scmi_dev->protocol_id == SCMI_PROTOCOL_SYSTEM)
>  		atomic_set(&scmi_syspower_registered, 0);
>  
> -	ida_free(&scmi_bus_id, scmi_dev->id);
>  	device_unregister(&scmi_dev->dev);
>  }
>  
> @@ -349,7 +347,7 @@ static struct scmi_device *
>  __scmi_device_create(struct device_node *np, struct device *parent,
>  		     int protocol, const char *name)
>  {
> -	int id, retval;
> +	int retval;
>  	struct scmi_device *scmi_dev;
>  
>  	/*
> @@ -387,20 +385,13 @@ __scmi_device_create(struct device_node *np, struct device *parent,
>  		return NULL;
>  	}
>  
> -	id = ida_alloc_min(&scmi_bus_id, 1, GFP_KERNEL);
> -	if (id < 0) {
> -		kfree_const(scmi_dev->name);
> -		kfree(scmi_dev);
> -		return NULL;
> -	}
> -
> -	scmi_dev->id = id;
>  	scmi_dev->protocol_id = protocol;
>  	scmi_dev->dev.parent = parent;
>  	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
>  	scmi_dev->dev.bus = &scmi_bus_type;
>  	scmi_dev->dev.release = scmi_device_release;
> -	dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);
> +	dev_set_name(&scmi_dev->dev, "scmi_dev.%s.%s.%d", dev_name(parent),
> +			scmi_dev->name, protocol);
>  
>  	retval = device_register(&scmi_dev->dev);
>  	if (retval)
> @@ -413,7 +404,6 @@ __scmi_device_create(struct device_node *np, struct device *parent,
>  	return scmi_dev;
>  put_dev:
>  	put_device(&scmi_dev->dev);
> -	ida_free(&scmi_bus_id, id);
>  	return NULL;
>  }
>  
> @@ -526,7 +516,6 @@ static void __exit scmi_bus_exit(void)
>  	 */
>  	scmi_devices_unregister();
>  	bus_unregister(&scmi_bus_type);
> -	ida_destroy(&scmi_bus_id);
>  }
>  module_exit(scmi_bus_exit);
>  
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 1b5fb2c4ce86..bbf1f05f2be3 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2641,8 +2641,8 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
>  	cinfo->max_msg_size = info->desc->max_msg_size;
>  
>  	/* Create a unique name for this transport device */

This comment should be updated.

> -	snprintf(name, 32, "__scmi_transport_device_%s_%02X",
> -		 idx ? "rx" : "tx", prot_id);
> +	snprintf(name, 32, "__scmi_transport_device_%s",
> +		 idx ? "rx" : "tx");

We used to create new transport devices for each port, but now we
only we only create one for recieve and one for transmit.  This is
unrelated to the commit message and needs to be done in separate commit
if at all.

regards,
dan carpenter