Dnia Poniedziałek, 5 Października 2015 14:34 Florian Achleitner <achleitner.florian@xxxxxxxxxxx> napisał(a) > Hi Jakub! > > On Saturday 03 October 2015 19:52:57 Jakub Kiciński wrote: > > Hi Florian! > > > > Thanks a lot for the submission. Would you mind digging a little > > deeper into this problem? I think the root cause is that the SPI > > read fails for some reason and our driver does not handle that > > properly, i.e. we don't read 255, it's just a random/failure value. > > Unfortunatelly I don't have any boards with this chip any more to > > do any tests or development myself, so it's on you ;) > > No problem, the board is still on the desk :) Thanks! > > This driver initially supported only I2C and in I2C regmap code if > > the read fails we will always get a zero value. Therefore we felt > > free to ignore in sc16is7xx_port_read() the return value of > > regmap_read(). > > > > Could you please run your tests again and see if perhaps the read is > > failing? In that case we should zero the return value from > > sc16is7xx_port_read(). > > I added a dev_err in *_port_read,write,update and *_fifo_read,write. No regmap > function returned an error in my test, but the 255 read value is still there. > > > I think from this thread: > > https://lkml.org/lkml/2008/2/20/271 > > we can assume that zero-length writes are a valid use-case for SPI > > and if so could you please test the driver for your SPI controller? > > Perhaps the zero-length check should be placed in the SPI controller > > driver? > > I digged a little deeper and did some measurements to support my idea. > I think the reason for the 255 read is that the chip does not support the zero > length write. > > The chip's SPI interface defines two sorts of frames. One for normal register > access, which is essentially an address followed by one byte of data, either > read or written. > > The second type is for accessing the fifo. It has an address and two bytes of > data, by definition. > > If the master now issues a zero length write, it sends only the address byte, > but the chip will expect two following data bytes, which do not arrive. > Instead it will consume the following frame. When this frame is the tx fifo > level read, the chip will not drive its SO line (still expecting a fifo > write), and the master reads 255. Now two bytes were clocked, and they are > back in sync. However, the value is crap. > > If my theorie is true, we would also have to make sure, that fifo access is > always two bytes to keep it synced. I will check this, and craft another > patch, if neccessary. My knowledge about SPI is minimal but this sounds reasonable. I just wonder why we've never seen this issue before? If the address byte is treated as data then we should see it received on the other side, did you observe that? > > I am OK with adding the sanity checks but lets first get to the bottom > > of this! > > I agree. I split the patch in two parts, because I would vote for patch 1/2 > unconditionally, to prevent buffer overruns, whatever happens. > And then, use patch 2/2 only if it is the right solution. I would personally move the zero checks of to_send to sc16is7xx_fifo_write() and just return early from there if to_send is zero. Even if this issue is somehow specific to your board/SPI controller we can treat this early return as an optimization :) As for the error check I would break out of the loop when we see the bogus value instead of just masking of the top bit. I think such value should not be trusted at all. Thanks, Jakub -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
