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?
> 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.
> 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?
Attachment:
signature.asc
Description: Digital signature
