Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

Joel Holdsworth
2016-11-08T17:13:58Z

Hi Marek,

>>> Should be this way for the sake of readability, fix globally:
>>>
>>> struct spi_transfer assert_cs_then_reset_delay = {
>>> .cs_change = 1,
>>> .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY
>>> };
>>
>> Sure ok. Personally, I prefer it to be concise, but I'm happy to accept
>> the norms.
>
> I prefer it to be readable :)
>

I'll conform to what you're saying.

But I just want to point out that this...

spi_message_add_tail(&(struct spi_transfer){.cs_change = 1,
.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}, &message);

...is clearly more readable than this...

struct spi_transfer assert_cs_then_reset_delay = {
.cs_change = 1,
.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY
};

....
<30 lines of unrelated code>
....

spi_message_add_tail(&assert_cs_then_reset_delay, &message);

...in my opinion anyway ;)

>> Previously I had a copy of spi_set_cs copy-pasted into my driver, but in
>> the end I decided to replace that with the zero-length transfers because
>> there's a danger that if the original spi_set_cs() gets rewritten some
>> time, my copy-paste code would leave around some nasty legacy.
>>
>> On the whole, I don't think the zero-length transfers are too
>> egregiously bad, and all the alternatives seem worse to me.
>
> So why not turn the CS line into GPIO and just toggle the GPIO?
>

Two reasons.

1. On some devices the CS line is built into the SPI master, rather than
being a normal GPIO.

2. The SPI driver stack addresses SPI devices in terms of which CS line
they are attached to. You can't have an spi_device in the kernel where
the SPI driver machinery doesn't have a CS line to control. Moreover it
needs to be possible for another SPI device to interrupt a running
transfer to the FPGA. Supporting this involves the SPI framework
managing the CS line.

Thanks
Joel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in