linux-kernel - Re: [PATCH v5 3/9] spi: support controllers with multiple data lanes

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: <cd0d6cb6-c025-4fab-9f40-300507b73f43@baylibre.com>
Date: Fri, 16 Jan 2026 16:20:21 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Sean Anderson <sean.anderson@...ux.dev>,
 linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v5 3/9] spi: support controllers with multiple data lanes

On 1/14/26 3:05 AM, Jonathan Cameron wrote:
> On Mon, 12 Jan 2026 11:45:21 -0600
> David Lechner <dlechner@...libre.com> wrote:
> 
>> Add support for SPI controllers with multiple physical SPI data lanes.
>> (A data lane in this context means lines connected to a serializer, so a
>> controller with two data lanes would have two serializers in a single
>> controller).
>>
>> This is common in the type of controller that can be used with parallel
>> flash memories, but can be used for general purpose SPI as well.
>>
>> To indicate support, a controller just needs to set ctlr->num_data_lanes
>> to something greater than 1. Peripherals indicate which lane they are
>> connected to via device tree (ACPI support can be added if needed).
>>
>> The spi-{tx,rx}-bus-width DT properties can now be arrays. The length of
>> the array indicates the number of data lanes, and each element indicates
>> the bus width of that lane. For now, we restrict all lanes to have the
>> same bus width to keep things simple. Support for an optional controller
>> lane mapping property is also implemented.
>>
>> Signed-off-by: David Lechner <dlechner@...libre.com>
>> ---
>>
>> v5 changes:
>> - Use of_property_read_variable_u32_array() for lane maps.
> For this, I think you need to check for short maps.
>>
>> v4 changes:
>> - Update for changes in devicetree bindings.
>> - Don't put new fields in the middle of CS fields.
>> - Dropped acks since this was a significant rework.
>>
>> v3 changes:
>> * Renamed "buses" to "lanes" to reflect devicetree property name change.
>>
>> This patch has been seen in a different series [1] by Sean before:
>>
>> [1]: https://lore.kernel.org/linux-spi/20250616220054.3968946-4-sean.anderson@linux.dev/
>>
>> Changes:
>> * Use u8 array instead of bitfield so that the order of the mapping is
>>   preserved. (Now looks very much like chip select mapping.)
>> * Added doc strings for added fields.
>> * Tweaked the comments.
>> ---
>>  drivers/spi/spi.c       | 116 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/spi/spi.h |  22 +++++++++
>>  2 files changed, 136 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index e25df9990f82..5c3f9ba3f606 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -2370,7 +2370,53 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>  		spi->mode |= SPI_CS_HIGH;
>>  
>>  	/* Device DUAL/QUAD mode */
>> -	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
>> +
>> +	rc = of_property_read_variable_u32_array(nc, "spi-tx-lane-map",
>> +						 spi->tx_lane_map, 1,
>> +						 ARRAY_SIZE(spi->tx_lane_map));
> 
> This reads 'up to' the ARRAY_SIZE(spi->tx_lane_map)
> If it is short, what is the right thing to do?  I'd either expect a check
> for that or for rc to be stashed somewhere if positive for later use.
> If the intent is for short the default of 0 is fine, then if it's a lot
> short we'll end up with repeated mappings to 0 which makes little sense.

The right thing would be to make sure that spi-tx-lane-map and
spi-tx-bus-width are the same size. I suppose we could reorder it
and call of_property_count_u32_elems(nc, "spi-tx-bus-width") first
to avoid needing to do the variable read.

> 
> 
>> +	if (rc == -EINVAL) {
>> +		/* Default lane map */
>> +		for (idx = 0; idx < ARRAY_SIZE(spi->tx_lane_map); idx++)
>> +			spi->tx_lane_map[idx] = idx;
>> +	} else if (rc < 0) {
>> +		dev_err(&ctlr->dev,
>> +			"failed to read spi-tx-lane-map property: %d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = of_property_count_u32_elems(nc, "spi-tx-bus-width");
>> +	if (rc < 0 && rc != -EINVAL) {
>> +		dev_err(&ctlr->dev,
>> +			"failed to read spi-tx-bus-width property: %d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	if (rc == -EINVAL) {
>> +		/* Default when property is not present. */
>> +		spi->num_tx_lanes = 1;
>> +	} else {
>> +		u32 first_value;
>> +
>> +		spi->num_tx_lanes = rc;
>> +
>> +		for (idx = 0; idx < spi->num_tx_lanes; idx++) {
>> +			of_property_read_u32_index(nc, "spi-tx-bus-width", idx,
>> +						   &value);
> 
> Probably want a sanity check on return value of that even though we are fairly sure
> it won't fail.

I figured that if of_property_count_u32_elems() succeeded then this could not
possibly fail. But OK, why not be extra careful.

> 
>> +
>> +			/*
>> +			 * For now, we only support all lanes having the same
>> +			 * width so we can keep using the existing mode flags.
>> +			 */
>> +			if (!idx)
>> +				first_value = value;
>> +			else if (first_value != value) {
>> +				dev_err(&ctlr->dev,
>> +					"spi-tx-bus-width has inconsistent values: first %d vs later %d\n",
>> +					first_value, value);
>> +				return -EINVAL;
>> +			}
>> +		}
>