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

[Alex Elder]

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