On 2024/12/24 04:33, Jonathan Cameron wrote:
> On Wed, 11 Dec 2024 08:08:06 +0800
> Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>
>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>
>> 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 <alison.schofield@xxxxxxxxx>
>> Reviewed-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>
>
>> 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;
>>
>
