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
