[kconfig] [ARMHF] [marvell armada cpu support] #172
Conversation
Deleted Arm related unused patches
kconfig_inclusions:
Enable Optoe
Gpio based I2c Mux
flash mtd parts support for flash partitions
kconfig_exclusions:
Removed uncommon soc support to reduce kernel size
Removed sound card
Reoved debug slab
Signed-off-by: Antony Rheneus <arheneus@marvell.com>
|
Please splite this up into one commit per list item in the merge/pull request description, and describe in the commit message, why that’s needed/useful. |
@paulmenzel, Do you want each Kernel config should be raised as a separate PRs? |
No, not for me. But one commit per change with a commit message would be nice. |
commit Signed-off-by: Antony Rheneus <arheneus@marvell.com>
Added commits as per your request, |
|
Thank you for splitting this up. Please use Additionally, are you sure you do not break existing devices with these config changes? Keep in mind, the same Linux kernel image is used for all devices. Lastly, for review and understanding your changes, commit messages are really necessary to explain the motivation and change. I believe the link I added in my first comment explains this well. For example, the two commits: It reads like they try to achive the same goal. But, what is the current default, and what is the problem when loading the Linux kernel from U-Boot (what version)? Shouldn’t XZ be used, because the Linux kernel is smaller, and loading a smaller image is better than the added decompression time? Why Gzip? Big distributions moved to LZ4 or Zstd because that give the fastest loading speed. Without documenting your goal and reasoning, nobody knows why these changes are done. |
…/sonic-linux-kernel into arm_update_master
@paulmenzel , I have rebased. Let me know if it is now easily reviewable? |
|
in my opinion, the comment added for each commit is not really helpful. The comment needs explain the reason why, not what. People can look at the commit and understand what you did, but it is hard for people to figure out why. that is basically @paulmenzel 's point. I do not think rebase address his concern. |
I have opened a new PR 176, please review it. As I could not rebase muliple times over this PR, as it doubled the commits from 24 to 48 and ended up in conflicts for each commit. |
|
If you agree on review the PR 176, I will close this PR 172. |
|
(You could have force pushed the rebased/new branch into this branch.) The commits do not double when rebasing. What command did you use for rebasing? Maybe we can help you. #176 is definitely an improvement. Thank you for the work. But it’s still lacking the git commit message summaries. Please read How to Write a Git Commit Message again. If there are open questions, please tell us. I’d really prefer to keep the discussion in this merge/pull request. But if you prefer #176, then we can move there too. |
I used "git rebase --interactive HEAD~24", as totally I had 24 commits for each config. Basically the config present in "arch/arm/configs/mvebu_v7_defconfig" should get into marvell-armhf configuration. Which is not so when debian build generates, hence I had to port configs from the mvebu_v7_defconfig, else kernel would not boot at all. Another example I can refer, OPTOE driver, which I want to enable or disable, I can only brief enabling OPTOE driver for SFF optics, rest woudl be just non-terse explanation from the OPTOE feature. Better quote comment for my each commit description for what explanation you want to know specific to Armada A385 soc, so that I can get more or better explanation from soc team to add it. |
|
For me problem is not the commits, but the conflict occurs even for "reword" commit description Command used for git reabse:- Git log of 46 commits:-
|
|
All the comments were addressed in PR #176 |
Deleted Arm related unused patches
kconfig_inclusions:
Enable Optoe
Gpio based I2c Mux
flash mtd parts support for flash partitions
kconfig_exclusions:
Removed uncommon soc support to reduce kernel size
Removed sound card
Reoved debug slab
Signed-off-by: Antony Rheneus arheneus@marvell.com