Skip to content

drivers: net: fix isr_arg for encx24j600 and ethos#4864

Merged
miri64 merged 5 commits intoRIOT-OS:masterfrom
kaspar030:fix_encx24j600_isr_arg
Feb 21, 2016
Merged

drivers: net: fix isr_arg for encx24j600 and ethos#4864
miri64 merged 5 commits intoRIOT-OS:masterfrom
kaspar030:fix_encx24j600_isr_arg

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Feb 20, 2016

See #4859.

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 20, 2016
@kaspar030 kaspar030 force-pushed the fix_encx24j600_isr_arg branch from b508f1d to 6593ec9 Compare February 20, 2016 23:37
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 20, 2016
@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

enc28j60 needs fixing, too.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

Just thinking about it: why do we need this parameter at all? The isr_arg is stored in dev regardless (which is always given). If we rename isr_arg to event_ctx or something like that we also make it more clear, that this isn't an argument anymore.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 21, 2016

Yeah, I came to the same conclusion (in #4859).

What do you think, does it make sense to keep the extra argument to event_callback at all?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

IMHO, no. But since this is an API change we should really sleep about this ;-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

Let's merge this one first (please adapt enc28j60 for that) and discuss this later.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

(because an API change WILL cause some discussion before getting merged, while this is a no-brainer)

@kaspar030 kaspar030 force-pushed the fix_encx24j600_isr_arg branch from 6593ec9 to 5b4b40c Compare February 21, 2016 21:04
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 21, 2016

  • rebased
  • unified isr_arg usage (pass it on EVENT_ISR, pass NULL otherwise)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 21, 2016

ACK and go.

miri64 added a commit that referenced this pull request Feb 21, 2016
@miri64
Merge pull request #4864 from kaspar030/fix_encx24j600_isr_arg
65caa15 
drivers: net: fix isr_arg for encx24j600 and ethos
@miri64 miri64 merged commit 65caa15 into RIOT-OS:master Feb 21, 2016
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Feb 21, 2016

Do we already have a follow-up issue/PR?

@kaspar030 kaspar030 deleted the fix_encx24j600_isr_arg branch February 21, 2016 23:17
@kaspar030 kaspar030 mentioned this pull request Feb 21, 2016
Merged
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 21, 2016

Do we already have a follow-up issue/PR?

Yep, #4871.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@miri64 miri64

Labels

Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kaspar030 @miri64 @OlegHahm