Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2

[Serge Semin]

On Fri, Apr 21, 2023 at 02:51:58PM +0530, Joy Chakraborty wrote:
> On Fri, Apr 21, 2023 at 2:23 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> >
> > On Thu, Apr 20, 2023 at 05:51:31AM +0000, Joy Chakraborty wrote:
> > > n_bytes variable in the driver represents the number of bytes per word
> > > that needs to be sent/copied to fifo. Bits/word can be between 8 and 32
> > > bits from the client but in memory they are a power of 2, same is mentioned
> > > in spi.h header:
> > > "
> > >  * @bits_per_word: Data transfers involve one or more words; word sizes
> > >  *    like eight or 12 bits are common.  In-memory wordsizes are
> > >  *    powers of two bytes (e.g. 20 bit samples use 32 bits).
> > >  *    This may be changed by the device's driver, or left at the
> > >  *    default (0) indicating protocol words are eight bit bytes.
> > >  *    The spi_transfer.bits_per_word can override this for each transfer.
> > > "
> > >
> > > Hence, round of n_bytes to a power of 2 to avoid values like 3 which
> > > would generate unalligned/odd accesses to memory/fifo.
> > >
> > > Fixes: a51acc2400d4 ("spi: dw: Add support for 32-bits max xfer size")
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > > Signed-off-by: Joy Chakraborty <joychakr@xxxxxxxxxx>
> > > ---
> > >  drivers/spi/spi-dw-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > > index c3bfb6c84cab..a6486db46c61 100644
> > > --- a/drivers/spi/spi-dw-core.c
> > > +++ b/drivers/spi/spi-dw-core.c
> > > @@ -426,7 +426,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
> > >       int ret;
> > >
> > >       dws->dma_mapped = 0;
> >
> > > -     dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > +     dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE));
> >
> > Almost 100 symbols looks too bulky. Moreover single-lined nested call
> > makes things a bit harder to read. What about formatting it up like
> > this?
> >
> >         dws->n_bytes =
> >                 roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> >                                                 BITS_PER_BYTE));
> >
> >         or like this:
> >
> >         dws->n_bytes = roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
> >                                                        BITS_PER_BYTE));
> >
> > Splitting the line into chunks will simplify the visual
> > differentiation between the outer and inner calls.
> >
> > * Note even though the 80-char columns limit isn't that strict rule
> > now, but it's still preferable unless exceeding the limit significantly
> > increases readability. The update you suggest doesn't seem like the case
> > which would improve the readability.
> >
> 
> Sure, I can make the following change in the formatting and send the
> patch series:
>          dws->n_bytes =
>                  roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
>                                                  BITS_PER_BYTE));

Ok. Thanks in advance.

-Serge(y)

> 
> Thanks
> Joy
> > -Serge(y)
> >
> > >       dws->tx = (void *)transfer->tx_buf;
> > >       dws->tx_len = transfer->len / dws->n_bytes;
> > >       dws->rx = transfer->rx_buf;
> > > --
> > > 2.40.0.634.g4ca3ef3211-goog
> > >