Re: [PATCH v2 1/2] spi: Add the SPI daisy chain support.

[Adrian Fiergolski]

On 06.07.2020 18:18, Mark Brown wrote:
> On Mon, Jul 06, 2020 at 11:22:43AM +0200, Adrian Fiergolski wrote:
>
> Please don't send new patches in reply to old threads, it buries them
> and can be confusing.

Ok, thanks. It's my debut in sharing a complete driver patch ;-)

>
>> The implementation is transparent for the SPI devices and doesn't require
>> their modifications. It is based on a virtual SPI device (spi-daisy_chain)
>> and defines two required device tree properties ('spi-daisy-chain-len' and
>> 'spi-daisy-chain-noop') and one optional
> It would really help to have an example of how a client device will use
> this, right now it's a bit hard to follow.  Overall it feels like this
> should be better abstracted, right now there's lots of ifdefs throughout
> the code which make things unclear and also seem like they're going to
> be fragile long term since realistically very few systems will be using
> this.  Perhaps this needs to be a library for devices that can daisy
> chain?  It does feel like the instances should be aware of each other
> since half the point with building the hardware like this is that it
> enables operations on multiple devices to happen in sync.

Well, I assume that one can connect completely different SPI devices on
a single daisy chain. In order to address all devices in a single
access, a controller would need to delay a transaction, wait for a
certain moment (how long?), synchronise and then transfer the message. I
think it adds unnecessary complexity, as MHO many engineers' issue at
the moment, including myself, is hardware implementing SPI daisy chain
without any support in the kernel. Usually, those are simple devices
where performance is not critical, which need to be accessed only once
or sporadically.

>
> There are also pervasive coding style issues which are really
> distracting.
>
>>  drivers/spi/spi-daisy_chain.c       | 428 ++++++++++++++++++++++++++++
> Please use - and _ consistently.
Basing on other drivers, I understood that the convention is 'spi-name'.
My idea was that name is daisy_chain.
>
>> @@ -55,6 +55,14 @@ config SPI_MEM
>>  	  This extension is meant to simplify interaction with SPI memories
>>  	  by providing a high-level interface to send memory-like commands.
>>  
>> +config SPI_DAISY_CHAIN
>> +	bool "SPI daisy chain support"
>> +	depends on OF
> Please keep Makefile and Kconfig sorted.
To be done in v3.
>
>> +++ b/drivers/spi/spi-daisy_chain.c
>> @@ -0,0 +1,428 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/*
>> + * A driver handling the SPI daisy chaines.
> Please make the entire comment block a C++ to make things look more
> intentional.
To be done in v3.
>
>> +	int rc;
>> +
>> +	//the device is not part of a daisy-chain
>> +	if (spi->daisy_chain_devs == NULL)
> Coding style for the comment here and throughout the code.  If your code
> doesn't visually resemble the normal coding style for the code base
> you're working on then that is a bit of a red flag when reviewing.
Do you mean '//' vs '/*' ? In the spi.c I tried to follow the original
style. In spi-daisy_chain.c, which is a new driver added by myself, I
used '//' which is me personal preference. Is it wrong approach in your
opinion?
>
>> +	if (!list_is_singular(&message->transfers)) {
>> +		dev_err(&spi->dev,
>> +			"Mutliple transfer segments are not support when on daisy chain");
>> +		return -EINVAL;
>> +	}
> That seems excessively restrictive?
Well, with multiple transfers, drivers could request deactivation of SS
in between them. That would break the daisy chain, as data is latched in
all devices once SS is de-asserted. How such a transaction should be
then handled? Do you know any example of drivers/devices using multiple
transfers?
>
>> +			//check if frequency is not being changed
>> +			if (tr->speed_hz && tr->speed_hz != spi->max_speed_hz) {
>> +				dev_err(&spi->dev,
>> +					"Change of SPI frequency not supported when on daisy chain");
>> +				return -EINVAL;
>> +			}
> Again this seems unreasonably restrictive, especially given the above
> single transfer restriction which means the speed can't change during a
> message?
We could allow here also slower transfers.

A person defining the daisy chain in the DT knowns the maximum frequency
(aka propagation delays between different components on PCB + internal
timing of the devices) at which the whole chain operates properly. Thus,
the transfer speed of the message can't violate this upper limit.
>
>> +			if (tr->len == spi_chain_dev->no_operation.len) {
>> +				tr->bits_per_word = spi_chain_dev->no_operation
>> +							    .bits_per_word;
>> +				tr->cs_change = 0;
>> +
>> +				list_add_tail(&tr->transfer_list,
>> +					      &message->transfers);
>> +			}
>> +			//daisy chain operation has different than regular length
>> +			else {
> Coding style on both the if () and placement of the comment.
It was corrected like that by clang-format-buffer in emacs which uses
the format defined in the kernel source-tree.
>
>> +				//copy tx buffer
>> +				if (tr->tx_buf) {
>> +					ntr->tx_buf =
>> +						kmalloc(ntr->len, GFP_KERNEL);
>> +					if (!ntr->tx_buf) {
>> +						rc = -ENOMEM;
>> +						goto err_out;
>> +					}
> Why is this not a kmemdup()?
Not really. Due to SPI big-endiannes, one needs to apply padding
(right-justification) properly at the beginning of the new buffer. I
have been looking for some kernel function, which would copy the buffers
from their ends in the reverse order. I haven't found a good candidate.
Any suggestion?
>
>> +					//The daisy-chain padding is assumed to be right-justified,
>> +					//so unused tx bits are transferred first
>> +					memcpy((void *)((char *)ntr->tx_buf +
>> +							ntr->len - tr->len),
>> +					       tr->tx_buf, tr->len);
> These casts shouldn't be needed, especially the cast to void * - if you
> need to cast to void * something bad is most likely happening.  Simiar
> issues apply in other places where you're casting.
tx_buf is 'const void*' in spi_transfer. spi-daisy_chain could allocate
a new spi_transfer with the extended tx_buffer, copy rest of fields from
the original one, swap the transactions in the message and eventually
(after transmission) copy back the rx buffer to the original transaction
and swap the transfers again. I think though, it's a bit of unnecessary
overhead. What do you think?