On 2024/12/24 04:33, Jonathan Cameron wrote:

On Wed, 11 Dec 2024 08:08:06 +0800
Zijun Hu [off-list ref] wrote:

quoted

From: Zijun Hu <redacted>

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.

quoted

Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Zijun Hu <redacted>

quoted

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

quoted

@@ -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.

quoted

+
 
 	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.

quoted

 	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;