Re: [PATCH] spi: dw: Fix dynamic speed change

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Tue, 2014-11-04 at 16:36 -0600, tthayer@xxxxxxxxxxxxxxxxxxxxx wrote:
> From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>
> 
> Changing SPI transfer speed using a utility such as spi_config with
> spidev updates chip->speed_hz which is compared to the next transfer
> speed.
> 
> When speed_hz is not declared in a SPI transfer, the transfer speed is
> not updated for the next read/write on /dev/spidevX.Y. The element
> spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of
> if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz
> matches transfer->speed_hz and the clock divider is not updated.
> 
> This fix: On each transfer update the clock divider, compare to the
> previous clock divider and update if necessary. This fixes another
> bug where the clock divider calculation at the top of the
> pump_transfers() function could be an odd-number.
> 

My intention is to use SPI core API as much as possible. Thus,
pump_transfers() I think should be gone in future. Instead of doing an
additional work can you provide a helper function to set speed_hz and
call it from pump_transfers()?



> Reported-by: Vlastimil Setka <setka@xxxxxxx>
> Signed-off-by: Vlastimil Setka <setka@xxxxxxx>
> Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/spi/spi-dw.c |   33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 72e12ba..3456b34 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -376,9 +376,6 @@ static void pump_transfers(unsigned long data)
>  	chip = dws->cur_chip;
>  	spi = message->spi;
>  
> -	if (unlikely(!chip->clk_div))
> -		chip->clk_div = dws->max_freq / chip->speed_hz;
> -
>  	if (message->state == ERROR_STATE) {
>  		message->status = -EIO;
>  		goto early_exit;
> @@ -415,21 +412,27 @@ static void pump_transfers(unsigned long data)
>  
>  	cr0 = chip->cr0;
>  
> -	/* Handle per transfer options for bpw and speed */
> -	if (transfer->speed_hz) {
> -		speed = chip->speed_hz;
> +	/* Always calculate the desired clock divider */
> +	speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz;
> +
> +	if (speed > dws->max_freq) {
> +		dev_err(&spi->dev, "Unsupported SPI freq: %d Hz\n", speed);
> +		message->status = -EIO;
> +		goto early_exit;
> +	}
> +
> +	/* clk_div doesn't support odd number */
> +	clk_div = dws->max_freq / speed;
> +	clk_div = (clk_div + 1) & 0xfffe;
>  
> -		if (transfer->speed_hz != speed) {
> -			speed = transfer->speed_hz;
> +	/* Determine if the clock divider changed, if so update chip struct */

Maybe "…update speed_hz" ?

> +	if (clk_div != chip->clk_div)
> +		chip->clk_div = clk_div;
> +	else
> +		clk_div = 0; /* Prevent register programming below */
>  
> -			/* clk_div doesn't support odd number */
> -			clk_div = dws->max_freq / speed;
> -			clk_div = (clk_div + 1) & 0xfffe;
> +	chip->speed_hz = speed;
>  
> -			chip->speed_hz = speed;
> -			chip->clk_div = clk_div;
> -		}
> -	}
>  	if (transfer->bits_per_word) {
>  		bits = transfer->bits_per_word;
>  		dws->n_bytes = dws->dma_width = bits >> 3;


-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy

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