On Thu, Mar 17, 2022 at 3:08 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> 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 ;)
Fair enough, but isn't there intrinsic beauty in a diff that deletes
more code than it adds?
The cleaner aspect to me is that the RAS Capability Structure support
can be added with a one line change rather than a new if block in
cxl_map_component_regs().
>
> > 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, ®s->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.
Good catch, yeah, not sure why I typed it that way, will fix.
