Re: [PATCH 2/8] cxl/pci: Cleanup cxl_map_device_regs()

[Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>]

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

> Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
> is in preparation for deleting cxl_map_regs().
> 
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Trivial style comments inline.  Otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

> ---
>  drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index bd6ae14b679e..bd766e461f7d 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
>  {
> +	resource_size_t phys_addr =
> +		pci_resource_start(pdev, map->barno) + map->block_offset;

I'm not totally convinced by this refactoring as it's ugly either
way...  Still your code, and I don't care that strongly ;)

>  	struct device *dev = &pdev->dev;
> -	resource_size_t phys_addr;
> -
> -	phys_addr = pci_resource_start(pdev, map->barno);
> -	phys_addr += map->block_offset;
> -
> -	if (map->device_map.status.valid) {
> -		resource_size_t addr;
> +	struct mapinfo {
> +		struct cxl_reg_map *rmap;
> +		void __iomem **addr;
> +	} mapinfo[] = {
> +		{ .rmap = &map->device_map.status, &regs->status, },

Combining c99 style .rmap for first parameter and then not doing it
for the second is a bit odd looking.  Was there a strong reason for
doing this?  I'd just drop the ".rmap =" as it's not as though
we need to look far to see what it's setting.

> +		{ .rmap = &map->device_map.mbox, &regs->mbox, },
> +		{ .rmap = &map->device_map.memdev, &regs->memdev, },
> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> +		struct mapinfo *mi = &mapinfo[i];
>  		resource_size_t length;
> -
> -		addr = phys_addr + map->device_map.status.offset;
> -		length = map->device_map.status.size;
> -		regs->status = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->status)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.mbox.valid) {
>  		resource_size_t addr;
> -		resource_size_t length;
>  
> -		addr = phys_addr + map->device_map.mbox.offset;
> -		length = map->device_map.mbox.size;
> -		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->mbox)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.memdev.valid) {
> -		resource_size_t addr;
> -		resource_size_t length;
> +		if (!mi->rmap->valid)
> +			continue;
>  
> -		addr = phys_addr + map->device_map.memdev.offset;
> -		length = map->device_map.memdev.size;
> -		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->memdev)
> +		addr = phys_addr + mi->rmap->offset;
> +		length = mi->rmap->size;
> +		*(mi->addr) = devm_cxl_iomap_block(dev, addr, length);
> +		if (!*(mi->addr))
>  			return -ENOMEM;
>  	}
>  
>