Re: [PATCH 06/26] cxl/port: Add Dynamic Capacity mode support to endpoint decoders

Ira Weiny
2024-04-05T20:34:37Z

Dave Jiang wrote:
>
>
> On 3/24/24 4:18 PM, ira.weiny@intel.com wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> >

[snip]

> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 66b8419fd0c3..e22b6f4f7145 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -255,6 +255,14 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> > __cxl_dpa_release(cxled);
> > }
> >
> > +static int dc_mode_to_region_index(enum cxl_decoder_mode mode)
> > +{
> > + if (mode < CXL_DECODER_DC0 || CXL_DECODER_DC7 < mode)
>
> I second what Fan said about readability here if you do (mode > CXL_DECODER_DC7) for upper bound check instead.
>

Sure...

[snip]

> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index e59d9d37aa65..80c0651794eb 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -208,6 +208,22 @@ static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> > mode = CXL_DECODER_PMEM;
> > else if (sysfs_streq(buf, "ram"))
> > mode = CXL_DECODER_RAM;
> > + else if (sysfs_streq(buf, "dc0"))
> > + mode = CXL_DECODER_DC0;
> > + else if (sysfs_streq(buf, "dc1"))
> > + mode = CXL_DECODER_DC1;
> > + else if (sysfs_streq(buf, "dc2"))
> > + mode = CXL_DECODER_DC2;
> > + else if (sysfs_streq(buf, "dc3"))
> > + mode = CXL_DECODER_DC3;
> > + else if (sysfs_streq(buf, "dc4"))
> > + mode = CXL_DECODER_DC4;
> > + else if (sysfs_streq(buf, "dc5"))
> > + mode = CXL_DECODER_DC5;
> > + else if (sysfs_streq(buf, "dc6"))
> > + mode = CXL_DECODER_DC6;
> > + else if (sysfs_streq(buf, "dc7"))
> > + mode = CXL_DECODER_DC7;
>
> I think maybe create a static string table that correlates cxl_decoder_mode
> to string. Then you can simplify cxl_decoder_mode_name() and as well as here.
> And here I think you can just do a for loop and go through the entire static
> table.

Ok... Sure.

Ira