Re: [PATCH v2 1/2] spi: Handle spi-{tx,rx}-bus-width 0 as SPI_NO_TX/SPI_NO_RX

Hi Cole,

On 8/20/26 5:26 PM, Cole Munz wrote:
> The spi-peripheral-props binding shipped in dts/upstream allows a bus
> width of 0, meaning no RX or TX is possible on this device. The
> switches in spi_slave_of_to_plat() only handle 1/2/4/8, so a width of
> 0 falls through to the default case and warns "spi-rx-bus-width 0 not
> supported" on every boot, even though the devicetree is valid per the
> binding. The fact that the wire is missing is then dropped from
> plat->mode.
> 
> Map 0 to new SPI_NO_TX/SPI_NO_RX mode bits, as Linux has done since
> v5.12 ("spi: Add SPI_NO_TX/RX support", mainline d962608ce218).
> Bits 16 and 17 are the first free mode bits.
> 
> Mapping the bits is not enough on its own, as Quentin pointed out:
> nothing would stop a caller from asking for a transfer in a direction
> that has no wire, and every controller driver would need its own
> guard. Validate centrally in dm_spi_xfer() instead, matching Linux's
> __spi_validate(): a din on a SPI_NO_RX device or a dout on a SPI_NO_TX
> device fails with -EINVAL before it reaches the driver. A new sandbox
> test covers both rejections and the accepted case.
> 
> This comes up on devices with no MISO line at all, such as a
> write-only SPI display described with spi-rx-bus-width = <0>.
> 
> Signed-off-by: Cole Munz <[email protected]>
> ---
> Changes in v2: added the dm_spi_xfer() validation and the sandbox
> test. Rerun on this branch:
> 
>    $ ./u-boot -T -c "ut dm dm_test_spi_xfer_no_rx_tx"   failures: 0
>    (spi_xfer, spi_flash, spi_find, spi_claim_bus, spi_set_wordlen
>    also all failures: 0)
> 
> With the dm_spi_xfer() hunk reverted the new test fails both -EINVAL
> assertions, so it does test the right thing.
> 
> checkpatch flags the #if CONFIG_IS_ENABLED(DM_SPI_FLASH) guard and the
> DM_TEST-after-brace placement in the test; both copy the idiom the
> existing tests in test/dm/spi.c use, so I kept them consistent.

Agreed.

> 
>   drivers/spi/spi-uclass.c | 16 ++++++++++++++++
>   include/spi.h            |  2 ++
>   test/dm/spi.c            | 41 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index 120565df1497..dd1843ffac19 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -105,6 +105,7 @@ int dm_spi_set_wordlen(struct udevice *dev, unsigned int wordlen)
>   int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   		const void *dout, void *din, unsigned long flags)
>   {
> +	struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
>   	struct udevice *bus = dev->parent;
>   	struct dm_spi_ops *ops = spi_get_ops(bus);
>   
> @@ -113,6 +114,15 @@ int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   	if (!ops->xfer)
>   		return -ENOSYS;
>   
> +	/*
> +	 * A device with no wire in one direction cannot transfer in it,
> +	 * so reject the request here rather than in every driver.
> +	 */
> +	if (din && (slave_plat->mode & SPI_NO_RX))
> +		return -EINVAL;
> +	if (dout && (slave_plat->mode & SPI_NO_TX))
> +		return -EINVAL;
> +
>   	return ops->xfer(dev, bitlen, dout, din, flags);
>   }
>   
> @@ -229,6 +239,9 @@ static int spi_child_post_bind(struct udevice *dev)
>   	/* Device DUAL/QUAD mode */
>   	value = dev_read_u32_default(dev, "spi-tx-bus-width", 1);
>   	switch (value) {
> +	case 0:
> +		mode |= SPI_NO_TX;
> +		break;
>   	case 1:
>   		break;
>   	case 2:
> @@ -247,6 +260,9 @@ static int spi_child_post_bind(struct udevice *dev)
>   
>   	value = dev_read_u32_default(dev, "spi-rx-bus-width", 1);
>   	switch (value) {
> +	case 0:
> +		mode |= SPI_NO_RX;
> +		break;
>   	case 1:
>   		break;
>   	case 2:
> diff --git a/include/spi.h b/include/spi.h
> index 97096a775262..f477763bdc60 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -34,6 +34,8 @@ struct spinand_info;
>   #define SPI_RX_QUAD	BIT(13)			/* receive with 4 wires */
>   #define SPI_TX_OCTAL	BIT(14)			/* transmit with 8 wires */
>   #define SPI_RX_OCTAL	BIT(15)			/* receive with 8 wires */
> +#define SPI_NO_TX	BIT(16)			/* no transmit wire */
> +#define SPI_NO_RX	BIT(17)			/* no receive wire */
>   
>   /* Header byte that marks the start of the message */
>   #define SPI_PREAMBLE_END_BYTE	0xec
> diff --git a/test/dm/spi.c b/test/dm/spi.c
> index a89ba06274fc..cb8f88713a9e 100644
> --- a/test/dm/spi.c
> +++ b/test/dm/spi.c
> @@ -216,3 +216,44 @@ static int dm_test_spi_xfer(struct unit_test_state *uts)
>   	return 0;
>   }
>   DM_TEST(dm_test_spi_xfer, UTF_SCAN_PDATA | UTF_SCAN_FDT);
> +
> +/* Test that a transfer is rejected when the device has no wire for it */
> +static int dm_test_spi_xfer_no_rx_tx(struct unit_test_state *uts)
> +{
> +	struct dm_spi_slave_plat *plat;
> +	struct spi_slave *slave;
> +	struct udevice *bus;
> +	const int busnum = 0, cs = 0;
> +	const char dout[5] = {0x9f};
> +	unsigned char din[5];
> +	uint saved_mode;
> +
> +	ut_assertok(spi_get_bus_and_cs(busnum, cs, &bus, &slave));
> +	ut_assertok(spi_claim_bus(slave));
> +	plat = dev_get_parent_plat(slave->dev);
> +	saved_mode = plat->mode;
> +
> +	plat->mode |= SPI_NO_RX;
> +	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
> +				      SPI_XFER_BEGIN | SPI_XFER_END));
> +
> +	plat->mode = saved_mode | SPI_NO_TX;
> +	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
> +				      SPI_XFER_BEGIN | SPI_XFER_END));
> +
> +	plat->mode = saved_mode;
> +	ut_assertok(spi_xfer(slave, 40, dout, din,
> +			     SPI_XFER_BEGIN | SPI_XFER_END));

Please remove the happy path, it's already tested in dm_test_spi_xfer().

I'm wondering if we cannot simply merge the unhappy paths into 
dm_test_spi_xfer() and have only one test. No strong opinion here.

This looks ok to me, with either of the above done, so:

Acked-by: Quentin Schulz <[email protected]>

Thanks!
Quentin