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

[Zijun Hu <zijun_hu@icloud.com>]

On 2024/8/1 00:21, Greg Kroah-Hartman wrote:
> 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@quicinc.com>
>>>>
>>>> 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@quicinc.com>
>>>> ---
>>>>  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.
> 

okay, thanks for these good suggestions.

let me mark this patch as abandoned now since i would like to do it by
another better way.

> thanks,
> 
> greg k-h