On Thu, Apr 19, 2018 at 4:51 PM, Julius Werner <jwer...@chromium.org> wrote: >> How does it mean two different things? The pairing of a controller and >> device where a full duplex operation is needed but the controller >> doesn't support things should indeed fail. I do not undertand how >> xfer() means two different things. Whenever pairing hardware together >> one needs to validate that things work when married. > > spi_xfer_two_vectors() takes a call like this: > > struct spi_op vectors[2] = { {.dout = out, .din = NULL, ...}, {.dout = > NULL, .din = in, ...}; > spi_xfer_vector(..., vectors, 2); > > and turns it into > > spi_xfer(..., dout, ..., din, ...); > > Those are two different things. The former means "first transfer dout on > MOSI and ignore MISO, then read in din on MISO and send zeroes or whatever > out on MOSI". The latter means "do a full-duplex transaction where you read > from MISO into din and write dout out on MOSI at the same time". If you did > the latter call on a controller supporting full-duplex it wouldn't actually > be a correct SPI flash transaction. But all the controllers that use > spi_xfer_two_vectors() have implemetations of xfer() that don't actually do > the thing I just described... instead they do the "first out, then in" > behavior even though that's normally not what spi_xfer(..., dout, ..., din, > ...) means. I think those implementations of xfer() are wrong and violating > the API. If they were corrected, spi_xfer_two_vectors() wouldn't make sense > the way it works right now.
I said it was helper to fuse the ops. Sure we can invert the code paths. > >> OK. It's just moving code around. > > Yeah, I just want to shuffle code around. Nothing is broken right now, it's > just working in a confusing and inconsistent way. We should have an API > where the callbacks share the same semantics across all implementations > (and also document those semantics more clearly). I can send some CLs out. -- coreboot mailing list: coreboot@coreboot.org https://mail.coreboot.org/mailman/listinfo/coreboot