On 11/05/2014 07:24 AM, Mark Brown wrote:
On Wed, Nov 05, 2014 at 01:07:36PM +0100, Vlastimil Šetka wrote:
5.11.2014 11:54, Mark Brown:
On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer@xxxxxxxxxxxxxxxxxxxxx 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.
You can declare a speed per transfer for *any* client, this is totally
generic behaviour. What is the specific relevance of spidev here?
spidev was just the vehicle that the issue was observed on and is useful
for debugging.
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.
Again, this is something that any client could do...
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):
To repeat again: please talk about driver problems in terms of the
driver not in terms of a particular driver. Glancing at the code here
it looks like spidev is buggy here, it's just blindly allowing userspace
to overwrite the maximum speed configured for the device which seems
like a bad idea, drivers should have no reason to expect that something
called max_speed is actually variable. It looks like spidev is abusing
this as a default speed.
spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in
spidev_ioctl() to overwrite the maximum speed. From the code and the
name, it seems like overwriting the maximum speed was the intended use
and not a side effect.
chip->speed_hz = spi->max_speed_hz = new_spi_speed
So the driver is overriding its idea of the current speed without
actually updatng the hardware. Why is the fix here not to just delete
the assignment here, it seems fairly obviously buggy?
Yes, removing line 591 of the spi-dw.c (chip->speed_hz =
spi->max_speed_hz;) will solve the problem and seems like a clean fix.
In this case the chip->speed_hz will persist the last transfer speed
setting.
The chip->speed_hz parameter won't be updated as part of the spi-dw
driver initialization. This may not matter since it will get updated on
the first transfer in the (transfer->speed != speed) test shown above.
--
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
