Re: [PATCH 1/8] spi: sh-msiof: Add sleep before master transfer for test

[Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>]

Hi Dirk,

On 09/06/2017 10:05 AM, Dirk Behme wrote:
> From: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@xxxxxxxxxxx>
> 
> This patch is for debug of transfer between master and slave.
> Since the slave needs to complete a preparation in data transfer
> before the master working, the sleep wait is put before
> the data transfer of the master.
> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@xxxxxxxxxxx>
> Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
> ---
>  drivers/spi/Kconfig        | 20 ++++++++++++++++++++
>  drivers/spi/spi-sh-msiof.c | 15 +++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index a75f2a2cf780..0139ecf8f42e 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -600,6 +600,26 @@ config SPI_SH_MSIOF
>  	help
>  	  SPI driver for SuperH and SH Mobile MSIOF blocks.
>  
> +config SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> +	bool "Transfer Synchronization Debug support for MSIOF"
> +	depends on SPI_SH_MSIOF
> +	default n

Drop 'default n', it is the default per se.

> +	help
> +	  In data transfer, the slave needs to have completed
> +	  a transfer preparation before the master.
> +	  As a test environment, it was to be able to put a sleep wait
> +	  before the data transfer of the master.
> +
> +config SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP
> +	int "Master of sleep latency (msec time)"

Master of sleep latency? Probably reformulation is wanted.

> +	default 1

In addition please define and add a valid 'range' option.

> +	depends on SPI_SH_MSIOF && SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG

Dependence on SPI_SH_MSIOF is inherited.

> +	help
> +	  Select Sleep latency of the previous data transfer
> +	  at the time of master mode.
> +	  Examples:
> +	    N => N msec
> +
>  config SPI_SH
>  	tristate "SuperH SPI controller"
>  	depends on SUPERH || COMPILE_TEST
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 0eb1e9583485..2b4d3a520176 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -41,6 +41,10 @@ struct sh_msiof_chipdata {
>  	u16 min_div;
>  };
>  
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> +#define TRANSFAR_SYNC_DELAY (CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP)

typo, s/TRANSFAR/TRANSFER/

Parenthesis around CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG_MSLEEP
are not needed.

> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */
> +
>  struct sh_msiof_spi_priv {
>  	struct spi_master *master;
>  	void __iomem *mapbase;
> @@ -910,6 +914,11 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>  		if (tx_buf)
>  			copy32(p->tx_dma_page, tx_buf, l / 4);
>  
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> +		if (p->mode == SPI_MSIOF_MASTER)
> +			msleep(TRANSFAR_SYNC_DELAY);
> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */

If SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG is unset, you may set TRANSFAR_SYNC_DELAY
to 0 and get rid of #ifdefs in the code.

	if (p->mode == SPI_MSIOF_MASTER && TRANSFAR_SYNC_DELAY)
		msleep(TRANSFAR_SYNC_DELAY);

> +
>  		ret = sh_msiof_dma_once(p, tx_buf, rx_buf, l);
>  		if (ret == -EAGAIN) {
>  			pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> @@ -983,6 +992,12 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>  	words = len / bytes_per_word;
>  
>  	while (words > 0) {
> +
> +#ifdef CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG
> +		if (p->mode == SPI_MSIOF_MASTER)
> +			msleep(TRANSFAR_SYNC_DELAY);
> +#endif /* CONFIG_SPI_SH_MSIOF_TRANSFER_SYNC_DEBUG */
> +
>  		n = sh_msiof_spi_txrx_once(p, tx_fifo, rx_fifo, tx_buf, rx_buf,
>  					   words, bits);
>  		if (n < 0)
> 

In general I don't think it makes any sense to incluide this change.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html