qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in


From: Peter Maydell
Subject: Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement
Date: Thu, 21 May 2020 17:41:55 +0100

On Wed, 20 May 2020 at 15:33, Eden Mikitas <address@hidden> wrote:
> When inserting the value retrieved (rx) from the spi slave, rx is pushed to
> rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
> register the driver uses is also 32 bit. This zeroes the 24 most
> significant bits of rx. This proved problematic with devices that expect to
> use the whole 32 bits of the rx register.
> I tested this change by running `make check` and by booting linux on
> sabrelite (which uses an spi flash device).
>
> Signed-off-by: Eden Mikitas <address@hidden>
> ---
>  hw/ssi/imx_spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 2dd9a631e1..43b2f14dd2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>
>          rx = 0;
>
> -        while (tx_burst) {
> +        while (tx_burst > 0) {
>              uint8_t byte = tx & 0xff;

When does this make a difference? Looking at the code
I don't think tx_burst can ever be negative at this
point, in which case the two conditions are equivalent.
What was the motivation for this change?

>              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> @@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>          if (fifo32_is_full(&s->rx_fifo)) {
>              s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
>          } else {
> -            fifo32_push(&s->rx_fifo, (uint8_t)rx);
> +            fifo32_push(&s->rx_fifo, rx);
>          }
>
>          if (s->burst_length <= 0) {

This seems like a separate change to the first one;
in general unrelated changes should be each in their
own patch, rather than combined into a single patch.

The fifo32_push() part of this change looks correct to me.

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]