| Message ID | 20191021033913.220758-77-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Add initial support for apollolake | expand |
Hi Simon, On Mon, Oct 21, 2019 at 11:40 AM 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 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 7f73f096ecb..b28f3423584 100644 > --- a/drivers/spi/ich.c > +++ b/drivers/spi/ich.c > @@ -424,9 +424,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); > + } Isn't the changed one the same as the old codes? > } else if (slave->max_write_size) { > op->data.nbytes = min(ICH_BOUNDARY - page_offset, > slave->max_write_size); > -- Regards, Bin
Hi Bin, On Tue, 19 Nov 2019 at 07:36, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Mon, Oct 21, 2019 at 11:40 AM 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 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 7f73f096ecb..b28f3423584 100644 > > --- a/drivers/spi/ich.c > > +++ b/drivers/spi/ich.c > > @@ -424,9 +424,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); > > + } > > Isn't the changed one the same as the old codes? The old code fails if max_read_size is 0 but max_write_size is set. It forgets to take account of max_write_size in that case. > > > } else if (slave->max_write_size) { > > op->data.nbytes = min(ICH_BOUNDARY - page_offset, > > slave->max_write_size); > > -- Regards, Simon
Hi Simon, On Thu, Nov 21, 2019 at 9:50 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > On Tue, 19 Nov 2019 at 07:36, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Simon, > > > > On Mon, Oct 21, 2019 at 11:40 AM 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 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 7f73f096ecb..b28f3423584 100644 > > > --- a/drivers/spi/ich.c > > > +++ b/drivers/spi/ich.c > > > @@ -424,9 +424,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); > > > + } > > > > Isn't the changed one the same as the old codes? > > The old code fails if max_read_size is 0 but max_write_size is set. It > forgets to take account of max_write_size in that case. > 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); > > > -- > Regards, Bin
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 7f73f096ecb..b28f3423584 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -424,9 +424,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); + } } else if (slave->max_write_size) { op->data.nbytes = min(ICH_BOUNDARY - page_offset, slave->max_write_size);
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 v3: None Changes in v2: None drivers/spi/ich.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)