Re: [PATCH v9 2/2] spi: loongson: add bus driver for the loongson spi controller

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, May 9, 2023 at 7:39 AM Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Mon, May 08, 2023 at 06:04:06PM +0300, andy.shevchenko@xxxxxxxxx wrote:
> > Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:

...

> > > +           loongson_spi_write_reg(loongson_spi,
> > > +                                  LOONGSON_SPI_SFCS_REG,
> > > +                                  (val ? (0x11 << spi->chip_select) :
> > > +                                  (0x1 << spi->chip_select)) | cs);
>
> > Too many parentheses.
>
> The code is absolutely fine, there is nothing wrong with adding explicit
> parentheses even where not strictly needed if it helps to make things
> clear (which is obviously always a problem wiht ternery operator use).

> Please, stop this sort of nitpicking.  It is at times actively unhelpful.

Okay, sorry for the noise.

...

> > > +           bit = fls(div) - 1;
> > > +           if ((1<<bit) == div)
> > > +                   bit--;
> > > +           div_tmp = rdiv[bit];
>
> > I believe this can be optimized.
>
> This isn't constructive feedback, if there is a concrete optimisation
> you want to suggest please just suggest it.

It goes together with the other questions in this function. But if you
think about some code proposal, here we are:

_original_

       const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

               div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
               if (div < 2)
                       div = 2;
               if (div > 4096)
                       div = 4096;

               bit = fls(div) - 1;
               if ((1<<bit) == div)
                       bit--;
               div_tmp = rdiv[bit];

               loongson_spi->spcr = div_tmp & 3;
               loongson_spi->sper = (div_tmp >> 2) & 3;

_proposed_

       const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

// as far as I understood the above table
00 00  [0]  <= 2
00 01  [1]  <= 4
01 00  [2]  <= 8
00 10  [3]  <= 16
00 11  [4]  <= 32
01 01  [5]  <= 64
01 10  [6]  <= 128
01 11  [7]  <= 256
10 00  [8]  <= 512
10 01  [9]  <= 1024
10 10  [10]  <= 2048
10 11  [11]  <= 4096

               div =
clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
               div_tmp = rdiv[fls(div - 1)];

               loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
               loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;

But TBH I would expect the author to think about it more.

Also the check can be modified to have less indented code:

       if (hz && loongson_spi->hz == hz)
          return 0;

       if (!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
          return 0;

...


> > > +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
>
> > Please, use _NS variant.
>
> It really does not matter, the chances of any collisions is pretty much
> zero.

The point is in preventing us from using those symbols outside of the scope.

--
With Best Regards,
Andy Shevchenko