Hi Bin,
On Wed, 27 Nov 2019 at 00:16, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Nov 25, 2019 at 12:12 PM Simon Glass sjg@chromium.org wrote:
This incorrectly shortens read operations if there is a maximum write size but no maximum read size. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None
drivers/spi/ich.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 08c37ca4ab..17b7a0ba0b 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -425,9 +425,11 @@ static int ich_spi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op) page_offset = do_div(aux, ICH_BOUNDARY); }
if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {op->data.nbytes = min(ICH_BOUNDARY - page_offset,slave->max_read_size);
if (op->data.dir == SPI_MEM_DATA_IN) {if (slave->max_read_size) {op->data.nbytes = min(ICH_BOUNDARY - page_offset,slave->max_read_size);}I wrote the following comments in v3, but looks did not get clarified?
I still don't get this. Based on your description, it seems that your logic works if we remove the } before the else if.
} else if (slave->max_write_size) { op->data.nbytes = min(ICH_BOUNDARY - page_offset, slave->max_write_size);--
You can try out a dry run...
Say: max_read_size = 0 max_write_size = 64
Say we are doing a read...
if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {
This branch will not be taken. data.dir is ...IN but because the max_read_size is 0, we skip this bit.
op->data.nbytes = min(ICH_BOUNDARY - page_offset, slave->max_read_size); } else if (slave->max_write_size) {
This branch is taken. Even though we are not doing a write, we set nbytes to 64:
op->data.nbytes = min(ICH_BOUNDARY - page_offset, slave->max_write_size); }
So now we are doing a read but using the max_write_size. This is incorrect.
So we need to separate these two things, since otherwise reads get limited when they should not be.
Regards, Simon