<div dir="ltr">Hi Olof,<br><br><div class="gmail_quote"><div dir="ltr">On Sun, 18 Nov 2018 at 23:38, Olof Johansson <<a href="mailto:olof@lixom.net" target="_blank">olof@lixom.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon <<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>> wrote:<br>
><br>
> Fix uninitialized 'val' warning receive function, send function<br>
> has been modify to be aligned with the receive function.<br>
><br>
> Signed-off-by: Tomer Maimon <<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>><br>
> ---<br>
>  drivers/spi/spi-npcm-pspi.c | 12 ++++++------<br>
>  1 file changed, 6 insertions(+), 6 deletions(-)<br>
><br>
> diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c<br>
> index 6dae91091143..f75df49ab84e 100644<br>
> --- a/drivers/spi/spi-npcm-pspi.c<br>
> +++ b/drivers/spi/spi-npcm-pspi.c<br>
> @@ -199,11 +199,11 @@ static void npcm_pspi_send(struct npcm_pspi *priv)<br>
>         wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);<br>
>         priv->tx_bytes -= wsize;<br>
><br>
> -       if (priv->tx_buf) {<br>
> -               if (wsize == 1)<br>
> -                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);<br>
> +       if (priv->tx_buf && wsize) {<br>
<br>
In general, doing an early:<br>
        if (!condition)<br>
                return;<br>
<br>
is a pattern we prefer in the kernel. Setting up the assumptions at<br>
the beginning of the function makes it easier to follow the code flow,<br>
and saves a level of indentation.<br>
<br>
It's a matter of taste though, and this function has only one level.<br>
So, either way is OK. Just mentioning it.<br>
<br>
>                 if (wsize == 2)<br>
>                         iowrite16(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);<br>
> +               else<br>
> +                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);<br>
<br>
I think this is broken? If wsize is something else than 1 or 2, you'll<br>
do a one-byte write but advance the buffer pointer with a different<br>
amount.<br>
<br>
It'll be fairly tricky to debug if this ever happens (it shouldn't,<br>
but still). This is why I added a WARN_ON_ONCE() in my patch instead.<br></blockquote><div><br></div><div>We just tried to reduce the number of lines to minimum, so we have debug it quite a lot (with all the numbers that can </div><div>get from priv->tx_bytes) and the only numbers that minimum function return are 0, 1 or 2.</div><div><br></div><div>But in the end of the day,  we don't have an issue with your solution as long it will be done also in the transfer function.<br></div><div><br></div><div>So if you can send a new patch with transfer function modification as well it will be great (Please let me know if you like me to send the patch).</div><div><br></div><div>Thanks again</div><div><br></div><div>Tomer</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-Olof<br>
</blockquote></div></div>