Re: [PATCH v4 04/14] cxl: Introduce parent_port_of() helper

["Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>]

On Thursday, March 6, 2025 5:44:38 PM Central European Summer Time Robert Richter wrote:
> Often a parent port must be determined. Introduce the parent_port_of()
> helper function for this.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>
I think that "often" doesn't provide any relevant information.
I would rephrase the commit message with something like that:

"Introduce the parent_port_of() helper function to avoid open
coding of determination of a parent port.".

Other than that:

Reviewed-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
>
>  drivers/cxl/core/port.c   | 15 +++++++++------
>  drivers/cxl/core/region.c | 11 ++---------
>  drivers/cxl/cxl.h         |  1 +
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 6a44b6dad3c7..25eecb591496 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -602,17 +602,20 @@ struct cxl_port *to_cxl_port(const struct device *dev)
>  }
>  EXPORT_SYMBOL_NS_GPL(to_cxl_port, "CXL");
>  
> +struct cxl_port *parent_port_of(struct cxl_port *port)
> +{
> +	if (!port || !port->parent_dport)
> +		return NULL;
> +	return port->parent_dport->port;
> +}
> +EXPORT_SYMBOL_NS_GPL(parent_port_of, "CXL");
> +
>  static void unregister_port(void *_port)
>  {
>  	struct cxl_port *port = _port;
> -	struct cxl_port *parent;
> +	struct cxl_port *parent = parent_port_of(port);
>  	struct device *lock_dev;
>  
> -	if (is_cxl_root(port))
> -		parent = NULL;
> -	else
> -		parent = to_cxl_port(port->dev.parent);
> -
>  	/*
>  	 * CXL root port's and the first level of ports are unregistered
>  	 * under the platform firmware device lock, all other ports are
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cbe762abf6b3..4f79cc17c9c8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1748,13 +1748,6 @@ static int cmp_interleave_pos(const void *a, const void *b)
>  	return cxled_a->pos - cxled_b->pos;
>  }
>  
> -static struct cxl_port *next_port(struct cxl_port *port)
> -{
> -	if (!port->parent_dport)
> -		return NULL;
> -	return port->parent_dport->port;
> -}
> -
>  static int match_switch_decoder_by_range(struct device *dev,
>  					 const void *data)
>  {
> @@ -1781,7 +1774,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	struct device *dev;
>  	int rc = -ENXIO;
>  
> -	parent = next_port(port);
> +	parent = parent_port_of(port);
>  	if (!parent)
>  		return rc;
>  
> @@ -1861,7 +1854,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
>  	 */
>  
>  	/* Iterate from endpoint to root_port refining the position */
> -	for (iter = port; iter; iter = next_port(iter)) {
> +	for (iter = port; iter; iter = parent_port_of(iter)) {
>  		if (is_cxl_root(iter))
>  			break;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index be8a7dc77719..24cec16d02a6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -724,6 +724,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
>  int cxl_num_decoders_committed(struct cxl_port *port);
>  bool is_cxl_port(const struct device *dev);
>  struct cxl_port *to_cxl_port(const struct device *dev);
> +struct cxl_port *parent_port_of(struct cxl_port *port);
>  void cxl_port_commit_reap(struct cxl_decoder *cxld);
>  struct pci_bus;
>  int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
>