linux-kernel - Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84c77e62-93f7-4a3e-8a86-7f653076bdcd@riscstar.com>
Date: Sun, 21 Sep 2025 13:58:50 -0500
From: Alex Elder <elder@...cstar.com>
To: Vivian Wang <wangruikang@...as.ac.cn>, broonie@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: dlan@...too.org, ziyao@...root.org, linux-spi@...r.kernel.org,
 devicetree@...r.kernel.org, paul.walmsley@...ive.com, palmer@...belt.com,
 aou@...s.berkeley.edu, alex@...ti.fr, p.zabel@...gutronix.de,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI
 controller driver

On 9/21/25 1:51 PM, Vivian Wang wrote:
> 
> On 9/20/25 23:59, Alex Elder wrote:
>> On 9/19/25 10:52 PM, Vivian Wang wrote:
>>>
>>> [...]
>>>
>>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>>> +{
>>> +    struct k1_spi_io *rx = &drv_data->rx;
>>> +    u32 bytes = drv_data->bytes;
>>> +    u32 val;
>>> +
>>> +    val = readl(drv_data->base + SSP_DATAR);
>>> +    rx->resid -= bytes;
>>> +
>>> +    if (!rx->buf)
>>> +        return;    /* Null reader: discard the data */
>>> +
>>> +    if (bytes == 1)
>>> +        *(u8 *)rx->buf = val;
>>> +    else if (bytes == 1)
>>>
>>> Typo? else if (bytes == 2)
>>
>> Wow.  Yes that is an error that I'll correct.
>>
>>>> +        *(u16 *)rx->buf = val;
>>>> +    else
>>>> +        *(u32 *)rx->buf = val;
>>>
>>> Maybe
>>>
>>>      else if (bytes == 4)
>>>          *(u32 *)rx->buf = val;
>>>      else
>>>          WARN_ON_ONCE(1);
>>
>> The value of bytes will be 1, 2, or 4, which we can tell
>> by inspection.  At one time I had a switch statement with
>> a default, but I decided to leave out the default, which
>> won't happen.
>>
>>> Just to make the pattern consistent? Same for k1_spi_write_word.
>>
>> Consistent with what?
>>
> I was just thinking it would be clearer if the code states clearly:
> 
>      1 -> u8
>      2 -> u16
>      4 -> u32
>      anything else -> shouldn't happen
> 
> As is, it wasn't obvious to me that we're just handling 4 as u32. Maybe
> we're just capping it at u32, and 8 is also handled.
> 
> Well, maybe I'm just not familiar with SPI stuff, and word size above 4
> doesn't make sense anyway.

Understood.  I only know it because I checked.  And I do want my
code to me understandable, so I'll add a comment as you suggest
below.

Thank you.

					-Alex

> 
> It could also be a comment
> 
>      else /* 4 */
> 
> Just a suggestion, no strong preference from me.
> 
> Vivian "dramforever" Wang
>