-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dir is potentially a bad name as spi is bi-directional? Is host/device better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm yeah role="host"
/ role="device"
would work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the signature of the function and every use of it in every board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.