Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LED Support for DellEMC Z9100 #2787
LED Support for DellEMC Z9100 #2787
Changes from 1 commit
2bc21cc
e08daa0
ec0d2b3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not a fan of copying the file from the HwSKU directory into to the platform directory. If the LED functionality needs to differ based on SKU, then we may want to consider moving all led_proc_init.soc files into the respective HwSKU directories. For SKUs which share a common led_proc_init.soc file, we can symlink them.
I'm not sure that this should hold up this PR, but it's something I wanted to at least note here for posterity.
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.
Joe, thanks for the view. I changed copy to symlink in new commit. Right now as per the design ledinit "platform/broadcom/docker-syncd-brcm/supervisord.conf" picks led_proc_init.soc in platform specific location than HWSKU as a preference.
command=/usr/bin/bcmcmd -t 60 "rcload /usr/share/sonic/platform/led_proc_init.soc"
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.
Yes, I understand that the current implementation looks for the led_proc_init.soc file in the platform directory. What I was noting for the future is that maybe we should consider moving the files into HwSKU directories and symlinking them if they share the same file.
I wasn't asking you to replace your copy operation with symlinking, but I'm OK with the change. My comment was about refactoring the mechanism completely in the future.
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.
Thanks Joe. Got it! But, when I introduce a symlink in syncd docker it points to a old hard link which is not valid in docker evnvironment.
[/usr/sonic/platfrorm] - do have only two directories hwsku and platform.
So, copy is the right way to address this. I changed this in new commit. Additionaly added checks to remove old led files at the time of copying.