dev.spi: Update SPIResources to accept a name and direction. #20
Conversation
nmigen_boards/dev/spi.py
Outdated
| @@ -4,16 +4,20 @@ | |||
| __all__ = ["SPIResource"] | |||
|
|
|||
|
|
|||
| def SPIResource(number, *, cs, clk, mosi, miso, int=None, reset=None, attrs=None): | |||
| def SPIResource(name, number, *, cs, clk, mosi, miso, dir="o", int=None, reset=None, attrs=None): | |||
There was a problem hiding this comment.
I think dir is potentially a bad name as spi is bi-directional? Is host/device better?
There was a problem hiding this comment.
Mm yeah role="host" / role="device" would work well.
There was a problem hiding this comment.
Works for me, will change it when I get the chance.
| @@ -4,16 +4,20 @@ | |||
| __all__ = ["SPIResource"] | |||
|
|
|||
|
|
|||
| def SPIResource(number, *, cs, clk, mosi, miso, int=None, reset=None, attrs=None): | |||
There was a problem hiding this comment.
This breaks the signature of the function and every use of it in every board.
There was a problem hiding this comment.
Ahhh okay I see now. Although SPIResource is currently unused:
William@William-THINK MINGW64 ~/src/nmigen-boards
$ git grep -F "SPIResource" master
master:nmigen_boards/dev/__init__.py:from .spi import SPIResource
master:nmigen_boards/dev/spi.py:__all__ = ["SPIResource"]
master:nmigen_boards/dev/spi.py:def SPIResource(number, *, cs, clk, mosi, miso, int=None, reset=None, attrs=None):
Every *Resource function you have in nmigen-boards starts with a mandatory number argument. I still feel like name should be mandatory in case a user forgets to put it?
nmigen_boards/dev/spi.py
Outdated
| io.append(Subsignal("cs", PinsN(cs, dir=get_dir()))) | ||
| io.append(Subsignal("clk", Pins(clk, dir=get_dir()))) | ||
| io.append(Subsignal("mosi", Pins(mosi, dir=get_dir()))) | ||
| io.append(Subsignal("miso", Pins(miso, dir=get_dir(False)))) |
There was a problem hiding this comment.
Never, ever, use non-keyword boolean arguments unless the name of the function serves as the name of the argument as well. Don't do it in languages without keyword arguments either... in C++ something like /*host_out=*/false works.
Anyway, it's also pointlessly complex to make this a function. Just inline the logic.
nmigen_boards/dev/spi.py
Outdated
| @@ -4,16 +4,30 @@ | |||
| __all__ = ["SPIResource"] | |||
|
|
|||
|
|
|||
| def SPIResource(number, *, cs, clk, mosi, miso, int=None, reset=None, attrs=None): | |||
| def SPIResource(name_or_number, number=None, *, cs, clk, mosi, miso, int=None, reset=None, attrs=None, role="host"): | |||
There was a problem hiding this comment.
This can use def SPIResource(*args, cs, ...) for simplicity, with Resource.family(*args, ...) below.
This makes it easy to create peripherals like SPI ADCs, among others.