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:
signature.asc
Description: PGP signature
