Re: [PATCH 4/4] spi: dw: Add Synopsys Component version reading and parsing

[Serge Semin]

On Fri, Nov 12, 2021 at 11:37:36PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 12, 2021 at 11:27 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Nov 12, 2021 at 10:52 PM Serge Semin
> > <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > > +       /*
> > > +        * Retrieve the Synopsys component version if it hasn't been specified
> > > +        * by the platform. Note the CoreKit version ID is encoded as a 4bytes
> > > +        * ASCII string enclosed with '*' symbol.
> > > +        */
> > > +       if (!dws->ver) {
> > > +               u32 comp;
> > > +
> > > +               comp = dw_readl(dws, DW_SPI_VERSION);
> > > +               dws->ver = (DW_SPI_GET_BYTE(comp, 3) - '0') * 100;
> > > +               dws->ver += (DW_SPI_GET_BYTE(comp, 2) - '0') * 10;
> > > +               dws->ver += (DW_SPI_GET_BYTE(comp, 1) - '0');
> > > +
> > > +               dev_dbg(dev, "Synopsys DWC%sSSI v%u.%02u\n",
> > > +                       (dws->caps & DW_SPI_CAP_DWC_HSSI) ? " " : " APB ",
> > > +                       dws->ver / 100, dws->ver % 100);
> >

> > Oh là là, first you multiply then you divide in the same piece of code!
> > What's wrong with fourcc (and thus keep it in ver filed as is) ? (Also
> > we have %p4cc)

Please note that's just a dev_DBG() print. So division has been used
in there to check whether the conversion was correct. The whole idea
behind the code above it was to retrieve the Component version as a
single number so then it could be used by the driver code in a simple
statement with a normal integer operation. For instance in case if we
need to check whether DW SSI IP-core version is greater than 1.01 we'd
have something like this: if (dws->ver > 101). Here 101 looks at least
close to the original 1.01. How would the statement look with four
chars? Of course we could add an another macro which would look like
this:
#define DW_SSI_VER(_maj, _mid, _min) \
	((_maj) << 24 | (_mid) << 16 | (_min) << 8 | '*')
and use it with raw version ID, like this
(dws->ver > DW_SSI_VER('1', '0', '1')). But IMO it doesn't look
better if not worse.
Alternatively we could split the version ID into two parts with
major and minor numbers. But normally one doesn't make much sense
without another so each statement would need to check both of them
at once anyway. So I decided to stick with a simplest solution and
combined them into a single storage. Have you got a better idea of
how to implement this functionality?

> >
> > > +       }
> 

> Have you seen this, btw?
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_dwlib.c#L93

It doesn't utilized version ID for something functional, but just
prints it to the console. So it isn't that good reference in this
case.

-Sergey

> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko