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: <411ade91-3fb5-40a3-baca-e6b03c5783ae@iscas.ac.cn>
Date: Mon, 22 Sep 2025 02:51:35 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Alex Elder <elder@...cstar.com>, 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/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.

It could also be a comment

    else /* 4 */

Just a suggestion, no strong preference from me.

Vivian "dramforever" Wang