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

["Vlastimil Šetka" <setka-3PjVBYxTQDg@public.gmane.org>]

5.11.2014 11:54, Mark Brown:
> On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>
>> 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
> Why is the behaviour of spidev relevant here, if there is a problem with
> spidev why is it being addressed in a specific driver?
You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case 
you can declare speed_hz per transfer, and everything is OK in spi-dw.

If you do not declare speed_hz (default is 0) when using 
SPI_IOC_MESSAGE, or when you do just read/write on /dev/spidevX.Y, the 
spi_transfer->speed_hz is filled by spi->max_speed_hz (which is the 
value previously set by SPI_IOC_WR_MAX_SPEED_HZ). This triggers a 
problem in spi-dw.

>> 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.
> In what way does the test "not work"?  What should the test be doing and
> what is it actually doing?

Test:
- set SPI clock to 100 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl
- write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero 
speed_hz)
- set SPI clock to 200 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl
- write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero 
speed_hz)

In last step, the clock should be 200 000 Hz, but actually it's 100 000 Hz.

The reason is buggy condition which encloses chip->clk_div recalculation:

      if (transfer->speed_hz) {
          speed = chip->speed_hz;
          if (transfer->speed_hz != speed) {

because transfer->speed_hz is filled by spi->max_speed_hz, which is 
equal to chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in 
dw_spi_setup):

     chip->speed_hz = spi->max_speed_hz = new_spi_speed

>
>> +	/* Always calculate the desired clock divider */
>> +	speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz;
> Please avoid using the ternery operator standalone, write a normal if to
> help people read the code.  Though in this case the core should always
> ensure that there is a speed set on each transfer so I'm not clear when
> this test would ever use anything other than transfer->speed_hz anyway.
Yes, as I can catch from code, the transfer->speed_hz should never be 0. 
But in current spi-dw.c version there is a `if (transfer->speed_hz)` 
condition, so I kept it in the patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html