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

[Mark Brown]

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.

> 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.

There are also pervasive coding style issues which are really
distracting.

>  drivers/spi/spi-daisy_chain.c       | 428 ++++++++++++++++++++++++++++

Please use - and _ consistently.

> @@ -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.

> +++ 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.

> +	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.

> +	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?

> +			//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?

> +			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.

> +				//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()?

> +					//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.

Attachment: signature.asc
Description: PGP signature