Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 10 May 2023 10:03:21 +0300 | Subject | Re: [PATCH v9 2/2] spi: loongson: add bus driver for the loongson spi controller |
| |
On Tue, May 9, 2023 at 7:39 AM Mark Brown <broonie@kernel.org> wrote: > On Mon, May 08, 2023 at 06:04:06PM +0300, andy.shevchenko@gmail.com 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
| |