openwrt
Rtl931x kernel 6.12 support
Thanks for adjusting your commits to the new testing kernel. The PR title is a bit misleading though. You are improving support for RTL931x on Kernel 6.12. (not adding it, like the title might imply)
Just at a quick glance: Some of the questions from the previous PR remain
-
About commit https://github.com/openwrt/openwrt/pull/19173/commits/2a4297aa5265bbc1d349972cdec6a5a7f21b17c6: According to @plappermaul the old implementation was dropped intentionally and replaced with a new one. Please evaluate whether the replacement solves all your needs.
-
About commit https://github.com/openwrt/openwrt/pull/19173/commits/798bcfabd01b23d61fa228c1e0c4ed00cec80160: Please help @jonasjelonek in verifying his PR https://github.com/openwrt/openwrt/pull/19171, approve it and then feel free to drop your version :)
-
Parts of your commit https://github.com/openwrt/openwrt/pull/19173/commits/ab44e7eb3899acf93e238576c70baae1f3ee76cc might be merged first via https://github.com/openwrt/openwrt/pull/19170 Not sure who of you was first and honestly I shouldn't care. I would assume you both noticed the issue and found a solution independently.
Small nitpicks about your commit messages: OpenWrt prefers/wants commit messages and subjects to be written in imperative form (just like you did with "Improve") adding > add copying > copy See https://openwrt.org/submitting-patches#submission_guidelines Also feel free to add a message to each commit that outlines why a change is useful/necessary.
For adding device support, maintainers expect a commit message to contain extra info like in the following commit: https://github.com/openwrt/openwrt/commit/6af9476b8a3c75ce65f17cfa89da7d35360a4756
- specs
- installation instructions
- recovery instructions (optional)
It doesn't have to be as extensive as in the commit message above but every bit helps :) How this looked for another plasma cloud device (though an AP not a switch): https://github.com/openwrt/openwrt/commit/4871fd2616acb03fefe69b068955dba36eb00770 Why is this helpful? Because such information will eventually vanish from OEM websites once a product goes EoL. But it will remain intact in OpenWrt commits and the OpenWrt wiki.
TL;DR
Everything you can split from this PR into smaller subsets improves the review process for everyone and keeps things simple.
- As an example I suggest creating an independent PR for the Plasma Cloud - psx28/esx28. It has nothing to do with improving support for the platform overall (it might just depend on this PR to be merged first). Adding a new device is far simpler and can be reviewed by committers that are not too familiar with the Realtek target.
I know this process is more tedious and requires more work on your part but it helps everyone involved with testing to help get your changes approved faster.
I'm sorry, if my long message overwhelms you. But it's the same for us with such a long PR: Discussion goes on about different commits and people get confused what others are talking about. Splitting PRs helps to keep things straight and discussions short :)
If English or writing commit messages are not your strong point we could split that work between us. Lot's of people working on the target at the moment also speak German. So maybe you can message them somehow outside of this PR to collaborate on the target. This would also prevent you and @jonasjelonek from sending in patches for similar/the same changes. You could even send them in together.
Not sure who of you was first and honestly I shouldn't care. I would assume you both noticed the issue and found a solution independently.
I'm pretty sure we found it independently. Had mine in my local tree for some time and then saw it here. But actually overlooked the function reference fix here ^^ Don't want to claim anything, let's join forces to push Realtek target forward :)
https://github.com/openwrt/openwrt/blob/9b777547beefa491319498208310f0758b49c45d/target/linux/realtek/base-files/etc/board.d/02_network#L111
Apparently your device comes with 3 fans, is it possible to set them up in the dts (and later configure them from userspace) or are they unconfigurable/connected to power directly?
Yes devices uses adt7476 and already recognized by lm-sensors,
these fans are regulated using DC-PWM control, controlled on pwm2 lane of the adt7476.
We use configuration inspired by target/linux/kirkwood/base-files/etc/init.d/hwmon_fancontrol, seagate,blackarmor-nas220
🚧 Build in progress for realtek/rtl931x/plasmacloud_psx28...
You can follow progress here
Triggered by: @aparcar
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks a lot, for the comments and feedback. We are closing this PR for now and smaller PR's shall be created.
@sharadanand: Can you publish here all new PRs?
I've created new pull requests with cleaned up changes from this PR + additional fixes:
- #19568
- #19569
- #19570
- #19571
- #19572
- #19573
- #19574
- #19575
- #19576
- #19577
- #19578
- #19579
- #19580
- #19581
- #19582
Just want to leave a big "Thank you" for splitting this PR into multiple smaller ones. This makes reviews simpler and the Realtek world much better.
quite cool to see 10/15 PRs already merged in 17 days since then. While one has just been superseeded and one was put on hold to work on redoing how mdio works. Only 3 or 4 PRs of the original submission are left ☺️
quite cool to see 10/15 PRs already merged in 17 days since then. While one has just been superseeded and one was put on hold to work on redoing how mdio works. Only 3 or 4 PRs of the original submission are left ☺️
This is not 100% correct. While it is still nice to see such a progress, not all the parts from this PR were converted into separate PRs. The PSX28+ESX28 support is currently still missing. Some of the userspace helpers can be found in the PSX10 support PR #19362. I would therefore consider the PSX10 support as dependency for the PSX28/ESX28 support.
The ticket #19362 receives from time to time rebases and I add notes about potential interesting main-branch changes which should be integrated during these rebases. But not much else happens regarding this one -> and I would guess, due to this situation, also not for the PSX28/ESX28
Looking forward to the next PR for the PSX28/ESX28 💪 Thanks for all your (plural) work on the realtek target
The PSX28+ESX28 support is currently still missing. Some of the userspace helpers can be found in the PSX10 support PR #19362. I would therefore consider the PSX10 support as dependency for the PSX28/ESX28 support.
@ecsv do you plan to submit a PR for the Plasma Cloud PSX28 now that #19362 is merged?
@hailfinger Actually, @sharadanand would have been the one who should have done this. But if I find time, I can also go through this and prepare a PR. Just don't expect that I will do this ~today~week.
The PSX28+ESX28 support is currently still missing. Some of the userspace helpers can be found in the PSX10 support PR #19362. I would therefore consider the PSX10 support as dependency for the PSX28/ESX28 support.
@ecsv do you plan to submit a PR for the Plasma Cloud PSX28 now that #19362 is merged?
I finally found some time for this. The basic support can be found in #20172