Skip to content

Conversation

@jfng
Copy link
Contributor

@jfng jfng commented Jul 26, 2024

Instead, SiliconPlatform.request() returns an instance of SiliconPlatformPort.

Fixes #19.

@jfng jfng requested a review from whitequark July 26, 2024 00:26
@jfng jfng force-pushed the fix-silicon-platform-deprecation branch from c95460d to 2190063 Compare July 26, 2024 00:29
@whitequark
Copy link
Contributor

whitequark commented Jul 26, 2024

I'm confused about this PR. The SiliconPlatformPort looks kind of like it's intended to be a lib.io.PortLike, but it's not, and doesn't enable using it with lib.io.Buffer. Any new cores are going to use lib.io.Buffer so this PR isn't sufficient.

If the goal is to just remove the deprecation, lib.io.Buffer.Signature(...).create() is sufficient to replace Pin(...).

@jfng jfng force-pushed the fix-silicon-platform-deprecation branch from 2190063 to 61d67ff Compare July 26, 2024 00:55
@jfng
Copy link
Contributor Author

jfng commented Jul 26, 2024

I'm confused about this PR. The SiliconPlatformPort looks kind of like it's intended to be a lib.io.PortLike, but it's not, and doesn't enable using it with lib.io.Buffer. Any new cores are going to use lib.io.Buffer so this PR isn't sufficient.

It wasn't a PortLike because it had no support for __invert__(), and even if it was, it wouldn't be usable with lib.io buffers without a significant rewrite of the platform.

If the goal is to just remove the deprecation, lib.io.Buffer.Signature(...).create() is sufficient to replace Pin(...).

I updated the PR to return an interface created from lib.io.Buffer.Signature.

@whitequark
Copy link
Contributor

It wasn't a PortLike because it had no support for __invert__(), and even if it was, it wouldn't be usable with lib.io buffers without a significant rewrite of the platform.

Right. I recall that the rewrite of the platform was planned on one of our meetings, which is why I was confused. This PR works fine as a backstop until then.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@whitequark whitequark merged commit 497149e into ChipFlow:main Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SiliconPlatform needs to be migrated to Amaranth 0.5

2 participants