Re: [PATCH 1/8] cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 15 Mar 2022 21:13:47 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Rather then duplicating the setting of valid, length, and offset for

than

> each type, just convey a pointer to the register map to common code.
> 
> Yes, the equivalent change in cxl_probe_component_regs() does not save

Why "equivalent"?  I'd just drop that word as not clear what it's equivalent to
and sentence makes sense without it.

> any lines of code, but it is preparation for adding another component
> register type to map (RAS Capability Strucutre).

Structure

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/regs.c |   46 ++++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 39a129c57d40..bd6ae14b679e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -59,36 +59,41 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
>  		void __iomem *register_block;
> -		u32 hdr;
> -		int decoder_cnt;
> +		struct cxl_reg_map *rmap;
>  		u16 cap_id, offset;
> -		u32 length;
> +		u32 length, hdr;

Unrelated change, but meh, it makes sense and doesn't effect readability of overall
patch much.

>  
>  		hdr = readl(base + cap * 0x4);
>  
>  		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
>  		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
>  		register_block = base + offset;
> +		hdr = readl(register_block);
>  
> +		rmap = NULL;
>  		switch (cap_id) {
> -		case CXL_CM_CAP_CAP_ID_HDM:
> +		case CXL_CM_CAP_CAP_ID_HDM: {
> +			int decoder_cnt;
> +
>  			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
>  				offset);
>  
> -			hdr = readl(register_block);
> -
>  			decoder_cnt = cxl_hdm_decoder_count(hdr);
>  			length = 0x20 * decoder_cnt + 0x10;
> -
> -			map->hdm_decoder.valid = true;
> -			map->hdm_decoder.offset = CXL_CM_OFFSET + offset;
> -			map->hdm_decoder.size = length;
> +			rmap = &map->hdm_decoder;
>  			break;
> +		}
>  		default:
>  			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
>  				offset);
>  			break;
>  		}
> +
> +		if (!rmap)
> +			continue;
> +		rmap->valid = true;
> +		rmap->offset = CXL_CM_OFFSET + offset;
> +		rmap->size = length;
>  	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, CXL);
> @@ -117,6 +122,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  	cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
> +		struct cxl_reg_map *rmap;
>  		u32 offset, length;
>  		u16 cap_id;
>  
> @@ -125,28 +131,22 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  		offset = readl(base + cap * 0x10 + 0x4);
>  		length = readl(base + cap * 0x10 + 0x8);
>  
> +		rmap = NULL;
>  		switch (cap_id) {
>  		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
>  			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
> -
> -			map->status.valid = true;
> -			map->status.offset = offset;
> -			map->status.size = length;
> +			rmap = &map->status;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
>  			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
> -			map->mbox.valid = true;
> -			map->mbox.offset = offset;
> -			map->mbox.size = length;
> +			rmap = &map->mbox;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
>  			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
>  			break;
>  		case CXLDEV_CAP_CAP_ID_MEMDEV:
>  			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
> -			map->memdev.valid = true;
> -			map->memdev.offset = offset;
> -			map->memdev.size = length;
> +			rmap = &map->memdev;
>  			break;
>  		default:
>  			if (cap_id >= 0x8000)
> @@ -155,6 +155,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  				dev_dbg(dev, "Unknown cap ID: %#x offset: %#x\n", cap_id, offset);
>  			break;
>  		}
> +
> +		if (!rmap)
> +			continue;
> +		rmap->valid = true;
> +		rmap->offset = offset;
> +		rmap->size = length;
>  	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_probe_device_regs, CXL);
>