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

[Alex Elder <elder@ieee.org>]

On 4/27/26 7:17 PM, Mark Brown wrote:
> On Mon, Apr 27, 2026 at 10:01:28PM -0400, Guodong Xu wrote:
> 
>> +static int k1_spi_transfer_one(struct spi_controller *host,
>> +			       struct spi_device *spi,
>> +			       struct spi_transfer *transfer)
>> +{
> 
>> +	/* Record how many words the len bytes represent */
>> +	count = transfer->len / drv_data->bytes;
>> +	drv_data->rx_resid = count;
>> +	drv_data->tx_resid = count;
> 
> This is setting up _resid with a number of words.

Guodong, see below, but I think the above should be:

	drv_data->rx_resid = transfer->len;
	drv_data->tx_resid = transfer->len;

>> +static void k1_spi_write_word(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct spi_transfer *transfer = drv_data->transfer;
>> +	u32 bytes = drv_data->bytes;
>> +	u32 val;
>> +
>> +	if (transfer->tx_buf) {
>> +		const void *buf;
>> +
>> +		buf = transfer->tx_buf + (transfer->len - drv_data->tx_resid);
> 
> This is using _resid as a byte count.  It'll be fine for 8 bits per word
> (which is by far the most common thing).

You're right, this is a really great observation.

The best thing is probably to just have the *_resid symbols
represent bytes.  (Their definitions in the structure say
that as well.)

k1_spi_write_word() decrements tx_resid by the number of
bytes transferred, so that's OK.

But the FIFO handles words, and k1_spi_write() limits the number
of *words* transferred, so the "count" calculation there needs to
take the word size into account.  Something like:

/**/	unsigned int resid_words;
	unsigned int count;

	/* Get the number of open slots in the FIFO; zero means all */
	count = FIELD_GET(SSP_STATUS_TFL, val) ? : K1_SPI_FIFO_SIZE;

	/*	
	 * Limit how much we try to send at a time, to reduce the
	 * chance the other side can overrun our RX FIFO.
	 */
/**/	resid_words = drv_data->tx_resid / drv_data->bytes;
/**/	count = min3(count, K1_SPI_THRESH, resid_words);
	do
		k1_spi_write_word(drv_data);
	while (--count);

	return !drv_data->tx_resid;

And we have the same problem in k1_spi_read().  There you can
probably change this:
     count = min(count, drv_data->rx_resid);
to this
     count = min(count, drv_data->rx_resid / drv_data->bytes);

You'll want to review and test yourself, but scanning through the
code this is what I see.

					-Alex