Re: [PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Greg Kroah-Hartman]

On Sat, Jul 20, 2024 at 09:21:50AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> 
> Simplify device_find_child_by_name() implementation by using present
> driver APIs device_find_child() and device_match_name().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> ---
>  drivers/base/core.c    | 15 +++------------
>  include/linux/device.h |  4 ++++
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 730cae66607c..22ab4b8a2bcd 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
>  struct device *device_find_child_by_name(struct device *parent,
>  					 const char *name)
>  {
> -	struct klist_iter i;
> -	struct device *child;
> -
> -	if (!parent)
> -		return NULL;
> -
> -	klist_iter_init(&parent->p->klist_children, &i);
> -	while ((child = next_device(&i)))
> -		if (sysfs_streq(dev_name(child), name) && get_device(child))
> -			break;
> -	klist_iter_exit(&i);
> -	return child;
> +	/* TODO: remove type cast after const device_find_child() prototype */

I do not understand the TODO here.  Why is it needed?  Why not fix it up
now?

> +	return device_find_child(parent, (void *)name,
> +				 (int (*)(struct device *, void *))device_match_name);
>  }
>  EXPORT_SYMBOL_GPL(device_find_child_by_name);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 34eb20f5966f..685ffd2dc867 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -47,6 +47,9 @@ struct dev_pin_info;
>  struct dev_iommu;
>  struct msi_device_data;
>  
> +/* TODO: unify device match() parameter of driver APIs to this signature */

Same here, why have this?  Why not unify them and then fix this up?

> +typedef int (*device_match_t)(struct device *dev, const void *data);
> +
>  /**
>   * struct subsys_interface - interfaces to device functions
>   * @name:       name of the device function
> @@ -1073,6 +1076,7 @@ int device_for_each_child(struct device *dev, void *data,
>  			  int (*fn)(struct device *dev, void *data));
>  int device_for_each_child_reverse(struct device *dev, void *data,
>  				  int (*fn)(struct device *dev, void *data));
> +/* TODO: change type of @data to const void * and @match to device_match_t */

Again, why leave this here?

thanks,

greg k-h