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

[Greg Kroah-Hartman]

On Thu, Aug 01, 2024 at 12:04:40AM +0800, Zijun Hu wrote:
> On 2024/7/31 20:53, Greg Kroah-Hartman wrote:
> > 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?
> > 
> 
> i have below findings during trying to simplify this API.
> 
> there are a type of driver APIs for finding device, for example,
> (bus|driver|class)_find_device() which all take below type for
> parameter @match:
> int (*match)(struct device *, const void *match_data)
> but device_find_child() take below type with void * for @match_data:
> int (*match)(struct device *, void *match_data).
> 
> @match's type of device_find_child() is not good as other finding APIs
> since nothing will be touched for finding operations normally.
> 
> i want to introduce a dedicate type for device match.
> typedef int (*device_match_t)(struct device *dev, const void *data);

Yes, that would be good.

> advantages:
> 1) device_match_t is simpler for finding APIs declarations and definitions
> 2) maybe stop further driver APIs from using bad match type as
> device_find_child()
> 
> TODO:
> 1) introduce device_match_t and use it for current present APIs
> (bus|driver|class)_find_device()
> 2) change API device_find_child() to take device_match_t, more jobs to
> do since need to touch many drivers
> 3) correct this change by by remove all TODO inline comments and force cast.
> 
> not sure if my ideas is good, what is your opinions?

That's great, but evolve it properly, don't add TODO lines here, there's
no real need for that.  Should end up with a lot of good cleanup
changes, and might not be all that bad overall.

thanks,

greg k-h