[U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check

[Stefan Mavrodiev <stefan@olimex.com>]

On 12/17/18 8:37 AM, Stefan Mavrodiev wrote:
>
> On 12/14/18 5:55 PM, Maxime Ripard wrote:
>> On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote:
>>> On 12/14/18 11:25 AM, Maxime Ripard wrote:
>>>> On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote:
>>>>> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote:
>>>>>> On 12/5/18 5:46 PM, Maxime Ripard wrote:
>>>>>>> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote:
>>>>>>>> Current driver doesn't check if the destination pointer is NULL.
>>>>>>>> This cause the data from the FIFO to be stored inside the internal
>>>>>>>> SDRAM ( address 0 ).
>>>>>>>>
>>>>>>>> The patch add simple check if the destination pointer is NULL.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>>>>>> ---
>>>>>>>>     drivers/spi/sun4i_spi.c | 3 ++-
>>>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
>>>>>>>> index b86b5a00ad..38cc743c61 100644
>>>>>>>> --- a/drivers/spi/sun4i_spi.c
>>>>>>>> +++ b/drivers/spi/sun4i_spi.c
>>>>>>>> @@ -129,7 +129,8 @@ static inline void
>>>>>>>> sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
>>>>>>>>           while (len--) {
>>>>>>>>             byte = readb(&priv->regs->rxdata);
>>>>>>>> -        *priv->rx_buf++ = byte;
>>>>>>>> +        if (priv->rx_buf)
>>>>>>>> +            *priv->rx_buf++ = byte;
>>>>>>> It seems pretty inefficient to test the pointer at each access, it
>>>>>>> would be better to check it once before starting the transfer.
>>>>>>>
>>>>>>> I'm not sure if that can even happen?
>>>>>> I've tried to check that before draining the receive fifo, but
>>>>>> then the controller doesn't work. I'm thinking that the fifo must
>>>>>> be drained in any case.
>>>>> Any further comments?
>>>> I was expecting you to comment on whether the FIFO needed to be
>>>> drained or not :)
>>> Sorry. I didn't understand that.
>>>
>>> Anyway. After some code checking, I found that the FIFO needs to be 
>>> drained
>>> because TP_EN (Transmit Pause Enable) bit is set during bus claim.
>>>
>>> "....
>>>
>>> In master mode, it is used to control transmit state machine to
>>> stop smart burst sending when RX FIFO is full.
>>>
>>> ..."
>>>
>>> Perhaps this bit should be enabled only when we want to read back data?
>> It's been a while since I last looked at the spi driver. What is linux
>> doing?
>
> In the kernel version there is the same check, like the one in the patch:
>
> drivers/spi/spi-sun4i.c:
> static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
> ......
>
> while (len--) {
>     byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
>     if (sspi->rx_buf)
>         *sspi->rx_buf++ = byte;
> }
> .....
>
> Guess I've missed this check when I was adopting the driver for u-boot.
>
>
>>
>> Maxime
>>

Any comments?
I think with all said, the patch can be merged as it is.