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

[Vivian Wang <wangruikang@iscas.ac.cn>]

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