Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 21 Apr 2023 11:53:54 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Joy Chakraborty <joychakr@google.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
        manugautam@google.com, rohitner@google.com
Subject: Re: [PATCH v8 5/5] spi: dw: Round of n_bytes to power of 2
Message-ID: <20230421085354.34dwrgr3enlxqhtc@mobilestation>
References: <20230420055131.2048959-1-joychakr@google.com>
 <20230420055131.2048959-6-joychakr@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230420055131.2048959-6-joychakr@google.com>
Precedence: bulk

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
>