- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.2k
drivers: eth_mcux power down #21370
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
drivers: eth_mcux power down #21370
Conversation
        
          
                drivers/ethernet/eth_mcux.c
              
                Outdated
          
        
      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.
aling parameters to last '(' + 1 space
        
          
                west.yml
              
                Outdated
          
        
      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.
wait that's a different change: you are bumping nxp's HAL to a newer verison. make a dedicated PR for it please.
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.
@tbursztyka without this version eth_mcux won't work, the version mentioned here has changes related to this patch.
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.
So make a version bump PR first, and then the eth_mcux PR dependent over it.
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.
@tbursztyka wouldn't a separate commit work rather than a whole new PR, there is no major SDK update
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.
Other option is to add a separate commit into this PR that updates the HAL version.
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.
the pb with the hal version bump is that it might affect other drivers that are not related to ethernet. Thus why it's imo preferable to separate completely
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.
But if we find out that the HAL version does not work everywhere we would be forced to revert both commits anyway so the end result is the same (as if we submit two separate PRs).
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.
@jukkar should be a good thing, one will not work without the other.
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.
https://docs.zephyrproject.org/latest/guides/modules.html#changes-to-existing-modules
You can use revision: pull/24/head to run CI testing. Once it passes, the zephyrproject-rtos/hal_nxp#24 can be merged and the revision here can be updated to the commit hash in the master (i.e. the commit that has been rebased onto master of hal_nxp).
0fd6c9c    to
    88a530a      
    Compare
  
    | @jukkar @tbursztyka i've split west and driver in separate commits and aligned code. | 
zephyrproject-rtos/hal_nxp#24 Add a mask for KSZ8081 PHY's control register to set power down. Signed-off-by: Andrei Gansari <[email protected]>
| All checks passed. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. | 
Power down PHY in eth_mcux driver and set carrier off. Signed-off-by: Andrei Gansari <[email protected]>
| Is this PR abandoned? | 
Power down PHY in eth_mcux driver and set carrier off.
Signed-off-by: Andrei Gansari [email protected]
Depends on zephyrproject-rtos/hal_nxp#24