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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux