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

  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@intel.com>
Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/spi/spi-dw-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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@intel.com>
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  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.

-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
>
  

On Fri, Apr 21, 2023 at 2:23 PM Serge Semin <fancer.lancer@gmail.com> 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@intel.com>
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  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));

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
> >
  

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@gmail.com> 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@intel.com>
> > > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > > ---
> > >  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
> > >
  

From: Joy Chakraborty
> Sent: 21 April 2023 10:22
...
> 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));

Won't checkpatch bleat about that?

Is it ever actually valid for the caller to provide a
value that isn't 8, 16 or 32 ?

I'm sure it looked as though some other lengths/counts
where likely to go badly wrong.

I know there are times when it is useful to bit-bang 'odd'
numbers of bits - like command+address+delay for fast reads
but that is a sub-32bit transfer so (at least somewhere)
is 1 word but not all the bits.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> From: Joy Chakraborty
> > Sent: 21 April 2023 10:22
> ...
> > 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));
>
 
> Won't checkpatch bleat about that?

Why would it?

> 
> Is it ever actually valid for the caller to provide a
> value that isn't 8, 16 or 32 ?

Judging by this
https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
it is. SPI-controller also supports word lengths within the
pre-synthesized range. So it's up to the SPI-peripherals and their
protocols what word length to select.

-Serge(y)

> 
> I'm sure it looked as though some other lengths/counts
> where likely to go badly wrong.
> 
> I know there are times when it is useful to bit-bang 'odd'
> numbers of bits - like command+address+delay for fast reads
> but that is a sub-32bit transfer so (at least somewhere)
> is 1 word but not all the bits.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
  

On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > From: Joy Chakraborty
> > > Sent: 21 April 2023 10:22
> > ...
> > > 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));
> >
>
> > Won't checkpatch bleat about that?
>
> Why would it?

I ran checkpatch on this and it seems to be fine with minor spacing changes.

>
> >
> > Is it ever actually valid for the caller to provide a
> > value that isn't 8, 16 or 32 ?
>
> Judging by this
> https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> it is. SPI-controller also supports word lengths within the
> pre-synthesized range. So it's up to the SPI-peripherals and their
> protocols what word length to select.
>
> -Serge(y)
>
> >
> > I'm sure it looked as though some other lengths/counts
> > where likely to go badly wrong.
> >
> > I know there are times when it is useful to bit-bang 'odd'
> > numbers of bits - like command+address+delay for fast reads
> > but that is a sub-32bit transfer so (at least somewhere)
> > is 1 word but not all the bits.
> >
> >       David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
  

On Fri, Apr 21, 2023 at 10:40:44PM +0530, Joy Chakraborty wrote:
> On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > > From: Joy Chakraborty
> > > > Sent: 21 April 2023 10:22
> > > ...
> > > > 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));
> > >
> >
> > > Won't checkpatch bleat about that?
> >
> > Why would it?
> 
> I ran checkpatch on this and it seems to be fine with minor spacing changes.

What spacing do you mean? No problem with the change as is:
[fancer@mobilestation] kernel $ git show HEAD | grep -A1 -B2 roundup_pow_of_two
-	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));
[fancer@mobilestation] kernel $ ./scripts/checkpatch.pl --git HEAD
total: 0 errors, 0 warnings, 10 lines checked

Commit e18b699257db ("spi: dw: Round of n_bytes to power of 2") has no obvious style problems and is ready for submission.

-Serge(y)

> 
> >
> > >
> > > Is it ever actually valid for the caller to provide a
> > > value that isn't 8, 16 or 32 ?
> >
> > Judging by this
> > https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> > it is. SPI-controller also supports word lengths within the
> > pre-synthesized range. So it's up to the SPI-peripherals and their
> > protocols what word length to select.
> >
> > -Serge(y)
> >
> > >
> > > I'm sure it looked as though some other lengths/counts
> > > where likely to go badly wrong.
> > >
> > > I know there are times when it is useful to bit-bang 'odd'
> > > numbers of bits - like command+address+delay for fast reads
> > > but that is a sub-32bit transfer so (at least somewhere)
> > > is 1 word but not all the bits.
> > >
> > >       David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
  

On Fri, Apr 21, 2023 at 10:45 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 10:40:44PM +0530, Joy Chakraborty wrote:
> > On Fri, Apr 21, 2023 at 10:18 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > >
> > > On Fri, Apr 21, 2023 at 04:39:30PM +0000, David Laight wrote:
> > > > From: Joy Chakraborty
> > > > > Sent: 21 April 2023 10:22
> > > > ...
> > > > > 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));
> > > >
> > >
> > > > Won't checkpatch bleat about that?
> > >
> > > Why would it?
> >
> > I ran checkpatch on this and it seems to be fine with minor spacing changes.
>
> What spacing do you mean? No problem with the change as is:
> [fancer@mobilestation] kernel $ git show HEAD | grep -A1 -B2 roundup_pow_of_two
> -       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));
> [fancer@mobilestation] kernel $ ./scripts/checkpatch.pl --git HEAD
> total: 0 errors, 0 warnings, 10 lines checked
>
> Commit e18b699257db ("spi: dw: Round of n_bytes to power of 2") has no obvious style problems and is ready for submission.
>
> -Serge(y)
>

Sorry for my error, it looks like my email client does not show it correctly.
What I was going to upload in V9 is the same as you mentioned.

Thanks
Joy
> >
> > >
> > > >
> > > > Is it ever actually valid for the caller to provide a
> > > > value that isn't 8, 16 or 32 ?
> > >
> > > Judging by this
> > > https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/spi/spi.c#L3630
> > > it is. SPI-controller also supports word lengths within the
> > > pre-synthesized range. So it's up to the SPI-peripherals and their
> > > protocols what word length to select.
> > >
> > > -Serge(y)
> > >
> > > >
> > > > I'm sure it looked as though some other lengths/counts
> > > > where likely to go badly wrong.
> > > >
> > > > I know there are times when it is useful to bit-bang 'odd'
> > > > numbers of bits - like command+address+delay for fast reads
> > > > but that is a sub-32bit transfer so (at least somewhere)
> > > > is 1 word but not all the bits.
> > > >
> > > >       David
> > > >
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)