Jonathan Cameron wrote:
> On Thu, 23 Jun 2022 19:45:50 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> > Interleave granularity and ways have CXL specification defined encodings.
> > Promote the conversion helpers to a common header, and use them to
> > replace other open-coded instances.
> >
> > Force caller to consider the error case of the conversion as well.
>
> What was the reasoning behind not just returning the value (rather
> than the extra *val parameter)? Negative values would be errors
> still. Plenty of room to do that in an int.
>
> I don't really mind, just feels a tiny bit uglier than it could be.
The rationale was to make it symmetric with reverse translation to
encoded values where those encode helpers are used directly for sysfs
input validation like the kstrto*() helpers.
Added a note to that effect.
>
> Also, there is one little unrelated type change in here.
>
> >
> > Co-developed-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > Signed-off-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> > ---
> > drivers/cxl/acpi.c | 34 +++++++++++++++++++---------------
> > drivers/cxl/core/hdm.c | 35 +++++++++--------------------------
> > drivers/cxl/cxl.h | 26 ++++++++++++++++++++++++++
> > 3 files changed, 54 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 951695cdb455..544cb10ce33e 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -9,10 +9,6 @@
> > #include "cxlpci.h"
> > #include "cxl.h"
> >
> > -/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> > -#define CFMWS_INTERLEAVE_WAYS(x) (1 << (x)->interleave_ways)
> > -#define CFMWS_INTERLEAVE_GRANULARITY(x) ((x)->granularity + 8)
> > -
> > static unsigned long cfmws_to_decoder_flags(int restrictions)
> > {
> > unsigned long flags = CXL_DECODER_F_ENABLE;
> > @@ -34,7 +30,8 @@ static unsigned long cfmws_to_decoder_flags(int restrictions)
> > static int cxl_acpi_cfmws_verify(struct device *dev,
> > struct acpi_cedt_cfmws *cfmws)
> > {
> > - int expected_len;
> > + unsigned int expected_len, ways;
>
> Type change for expected_len seems fine but isn't mentioned in the patch description.
Yeah, that seems a thoughtless change to me since only @ways needs to be
an unsigned int. I fixed it up.
