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

[Joel Holdsworth]

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