Skip to content
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

Removing unused variables #223

Open
wants to merge 3 commits into
base: 5.8.7.1_35809.20191129_COEX20191120-7777
Choose a base branch
from

Conversation

sevan
Copy link

@sevan sevan commented Oct 4, 2022

Locally I have managed to reach building the driver without -Wno-unused-variable and just having -Werror turned on to catch all the unused variables which gcc flagged up.
There is a combination of stuff that's just not referenced and can be deleted, stuff that's reliant on CONFIG_RTW_DEBUG for macros to work which reference the variables, and things that need guarding off for specific configuration definition.
This pull request starts off covering these cases. locally there are changes that are bodges just to skip over things but as I said, I do have the driver building and working without -Wno-unused-variableand just having -Werror, the build is silent.

sevan added 3 commits October 2, 2022 23:32
Sweep across default build to purge variables marked as unused.
This is incomplete and there are more items which need guarding off,
for example there are variables which are only used by debug print
macros that do nothing by default.
There are variables which are only used by debug print macros when
debugging is enabled. Guard them off with CONFIG_RTW_DEBUG to keep
the compiler happy about unused variables.
while here zap some unused variable declarations.
@MaxG87
Copy link
Collaborator

MaxG87 commented Oct 14, 2022

Thank you for your contribution, I appreciate it very much.

I too think that the source code is in a shape that requires some clean-ups. I have tried to make only the most necessary adjustments to avoid merge conflicts. However, with RTW88-USB raising, I do not think that a new revision of RTL88x2BU will appear, so fixing the issues that annoy us might be a good thing.

If we go this route, some other adaptions come to my mind too. For instance, I would get rid of the support for other operating systems (e.g. PLATFORM_WINDOWS preprocessor directives) and drop support for old Kernel versions too. Especially these accumulated guards for old Kernel versions are a bit annoying to me when I implement support for a recent version.

Thus, before I give your version a try and merge it if it runs for me, I would like to have the agreement of another collaborator here. @cilynx , @CGarces , what would you say?

@CGarces
Copy link
Contributor

CGarces commented Oct 16, 2022

Cleanup a RTL driver is a lot of work.

As reference you can see the evolution of rtl8723bs to land in the linux kernel as staging driver.
805 commits of really boring work.
Plus hundreds of commits to replace the custom RTL code to the equivalent functions of Linux kernel (check r8188eu)

Tools like unifdef or coccinelle maybe can help you to made the work faster.

I tryed to clean up rtl8189fs to maybe reach the staging area of Linux kernel but after 60+ commits I lost interest.

If RTW88-USB adds support to rtl88x2bu chipset, maybe is a better idea to contribute directly to the Linux kernel.
Contribute to linux kernel sound complicated but sometimes is just a single line of code like #180

@CGarces
Copy link
Contributor

CGarces commented Oct 16, 2022

BTW the PR looks fine for me, but should include Makefile change to remove -Wno-unused-variable

@sevan
Copy link
Author

sevan commented Oct 17, 2022

This change is incomplete for removing -Wno-unused-variable from the Makefile.
This removes all the obvious unused variables. I have local changes which I've not committed which allow me to complete the driver compile and that's how I identified these variables in this PR.

I'll take a look at the upstream process / cleanup in the repos you pointed to @CGarces.

@sevan
Copy link
Author

sevan commented May 2, 2023

@MaxG87 do you want to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants