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

Hi Marek,

quoted

quoted

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 ;)

quoted

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
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html