Re: [PATCH 1/6] spi: add flow controll support

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel wrote:

At a high level I think this needs splitting up quite a bit - this is
defining a new DT binding, a new core interface for controlling flow
control and a bitbanging flow control implementation.  At least those
three bits should probably be separate patches, it may make sense to
split up the bitbanging implementation some more too.

> Different HW implement different variants of SPI based flow control (FC).
> To flexible FC implementation a spited it to fallowing common parts:
> Flow control: Request Sequence
> Master CS   |-------2\_____________________|
> Slave  FC   |-----1\_______________________|
> DATA        |-----------3\_________________|

I don't know what these pictures mean since you don't say what "slave
FC" is - I'm assuming you mean a separate pin but you need to specify
that.  It really looks like this would be clearer with words rather than
ASCII art.

In the case above this looks like a normal interrupt from a device, I'm
not clear what is different here or why this is in the core?

> Flow control: ACK End of Data
> Master CS   |______________________/2------|
> Slave  FC   |________________________/3----|
> DATA        |__________________/1----------|

Why would anything care about this case and why is it part of the SPI
protocol rather than something that the device driver would do?

> +	if (atomic_read(&spi->active_rq)) {
> +		if (!spi_fc_equal_cs(spi))
> +			dev_warn(&spi->dev, "Got request, but device is not ready\n");
> +		atomic_set(&spi->active_rq, 0);
> +		return ret;
> +	}

Atomic operations are very hard to use safely, you need to really spell
out what you think this is doing from a synchronization point of view
and why it's safe.

> +	if (spi->mode | SPI_FC_REQUEST &&
> +			!spi->cs_enabled && fc_enabled) {
> +		atomic_set(&spi->active_rq, 1);
> +		if (spi->request_cb)
> +			spi->request_cb(spi);

This logic is all too complicated for me to follow.  Why are we oring
things with spi->mode?

> +	struct device_node *np = spi->dev.of_node;
> +	int ret;
> +
> +	if (!np)
> +		return 0;

We can't support things only for DT, we need a way for things to be used
on other systemms.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]