Re: [PATCH v4 04/11] driver core: Constify API device_find_child() then adapt for various usages

On 2024/12/24 04:33, Jonathan Cameron wrote:
> On Wed, 11 Dec 2024 08:08:06 +0800
> Zijun Hu <[email protected]> wrote:
> 
>> From: Zijun Hu <[email protected]>
>>
>> Constify the following API:
>> struct device *device_find_child(struct device *dev, void *data,
>> 		int (*match)(struct device *dev, void *data));
>> To :
>> struct device *device_find_child(struct device *dev, const void *data,
>>                                  device_match_t match);
>> typedef int (*device_match_t)(struct device *dev, const void *data);
>> with the following reasons:
>>
>> - Protect caller's match data @*data which is for comparison and lookup
>>   and the API does not actually need to modify @*data.
>>
>> - Make the API's parameters (@match)() and @data have the same type as
>>   all of other device finding APIs (bus|class|driver)_find_device().
>>
>> - All kinds of existing device match functions can be directly taken
>>   as the API's argument, they were exported by driver core.
>>
>> Constify the API and adapt for various existing usages by simply making
>> various match functions take 'const void *' as type of match data @data.
> 
> There are a couple of places I noticed where you changed types
> that aren't related to the specific change this is making.
> They are almost certainly fine but I'd either like a specific note
> on that in this patch description or just change the elements
> that are directly affected by this change.
> 

okay, will correct commit message in v5.

v5 will mention these changes will bring extra code improvement as side
effects during achieving main purpose.

>>
>> Reviewed-by: Alison Schofield <[email protected]>
>> Reviewed-by: Takashi Sakamoto <[email protected]>
>> Signed-off-by: Zijun Hu <[email protected]>
> 
> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index d778996507984a759bbe84e7acac3774e0c7af98..bfecd71040c2f4373645380b4c31327d8b42d095 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
> 
>> @@ -1722,10 +1722,12 @@ static struct cxl_port *next_port(struct cxl_port *port)
>>  	return port->parent_dport->port;
>>  }
>>  
>> -static int match_switch_decoder_by_range(struct device *dev, void *data)
>> +static int match_switch_decoder_by_range(struct device *dev,
>> +					 const void *data)
>>  {
>>  	struct cxl_switch_decoder *cxlsd;
>> -	struct range *r1, *r2 = data;
>> +	const struct range *r1, *r2 = data;
> 
> As below. I'd not touch type of things you don't need to touch.
> 
explained below.

>> +
>>  
>>  	if (!is_switch_decoder(dev))
>>  		return 0;
>> @@ -3176,9 +3178,10 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
>>  	return rc;
>>  }
>>  
>> -static int match_root_decoder_by_range(struct device *dev, void *data)
>> +static int match_root_decoder_by_range(struct device *dev,
>> +				       const void *data)
>>  {
>> -	struct range *r1, *r2 = data;
>> +	const struct range *r1, *r2 = data;
> 
> From the point of view of keeping the patch to what it 'needs'
> to touch, should leave type of r1 alone.
> I've not looked at whether this causes any problems, just whether
> it is relevant to this change.
> 

1) i have checked that will not cause problem, may have code improvement

2) this change (2 lines) is simpler than below(3 lines)
   -	struct range *r1, *r2 = data;
   +    struct range *r1;
   +    const struct range *r2 = data;

3) r1 and r2 are used for range_contains() whose prototype was changed
to takes 2 const pointers recently.

4) make r1 & r2 keep the same type as original.

>>  	struct cxl_root_decoder *cxlrd;
>>  
>>  	if (!is_root_decoder(dev))
>> @@ -3189,11 +3192,11 @@ static int match_root_decoder_by_range(struct device *dev, void *data)
>>  	return range_contains(r1, r2);
>>  }
>>  
>> -static int match_region_by_range(struct device *dev, void *data)
>> +static int match_region_by_range(struct device *dev, const void *data)
>>  {
>>  	struct cxl_region_params *p;
>>  	struct cxl_region *cxlr;
>> -	struct range *r = data;
>> +	const struct range *r = data;
>>  	int rc = 0;
>>  
>