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

Upstreaming the driver #35

Open
RyuzakiKK opened this issue Dec 8, 2020 · 14 comments
Open

Upstreaming the driver #35

RyuzakiKK opened this issue Dec 8, 2020 · 14 comments
Labels
help wanted Extra attention is needed

Comments

@RyuzakiKK
Copy link

Is there any plan for upstreaming this module driver to expand/replace the current in-tree hid-logitech?

@berarma
Copy link
Owner

berarma commented Dec 8, 2020

That's what I'd like to do. I've been planning it in my head but I think there's some work to do and I'm not sure how well will it be received. I just need some motivation and a bit of time.

I think I should break the changes into several patches for each feature, there would be a big one for the effects implementation and other smaller patches for the secondary features. And for every patch I work on, send it upstream and try to get it merged.

Any advice and guiding from someone knowledgeable would help at this point.

@Elizafox
Copy link

Elizafox commented Mar 29, 2021

That's what I'd like to do. I've been planning it in my head but I think there's some work to do and I'm not sure how well will it be received. I just need some motivation and a bit of time.

I think I should break the changes into several patches for each feature, there would be a big one for the effects implementation and other smaller patches for the secondary features. And for every patch I work on, send it upstream and try to get it merged.

Any advice and guiding from someone knowledgeable would help at this point.

Hi :)

I have contributed to Linux before, although only a minor patch (adding a device ID to a serial device, many moons ago). But! It isn't too difficult and don't get too intimidated.

It looks like the HID device maintainer list is at linux-input@vger.kernel.org, as found in the MAINTAINERS file:

HID CORE LAYER
M:	Jiri Kosina <jikos@kernel.org>
M:	Benjamin Tissoires <benjamin.tissoires@redhat.com>
L:	linux-input@vger.kernel.org
S:	Maintained
T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
F:	drivers/hid/
F:	include/linux/hid*
F:	include/uapi/linux/hid*

It's best to touch base with them first and get feedback on your changes and how they want to go about this.

Make sure all your code follows the kernel coding style. I haven't looked too deeply at the code, so I'm not sure if it all conforms, but I saw no obvious issues.

@berarma
Copy link
Owner

berarma commented Mar 29, 2021

Thanks for sharing!

I was just starting to take it more seriously. I wanted to simplify the changes before starting commiting patches but I'm hesitating about talking to them first.

When you say touching base do you mean explaining them the changes without sending any code?

@berarma
Copy link
Owner

berarma commented Aug 19, 2021

I think #49 should be fixed before trying to upstream the driver.

@berarma berarma added the help wanted Extra attention is needed label Sep 1, 2022
@motolav
Copy link
Contributor

motolav commented Jan 31, 2023

#49 is possibly fixed but needs someone with a G29 and Euro Truck Sim 1.41 to verify, You have to use the master tree as it was after version 0.4.0 where the issue might have been fixed as it fixed my issue with auto centering on the DFGT.

@berarma
Copy link
Owner

berarma commented Jan 31, 2023

#49 is possibly fixed but needs someone with a G29 and Euro Truck Sim 1.41 to verify, You have to use the master tree as it was after version 0.4.0 where the issue might have been fixed as it fixed my issue with auto centering on the DFGT.

Nothing has been fixed in the driver, it's that ETS2 has changed the way it uses FFB.

I haven't tried to upstream the driver because, due to my limited time, it'll most probably be a lot of work for me to create the patches they'll want. And if they're picky, I have already received hints that they will, they could block the important parts because they're not a perfect implementation. The truth is that the driver implements the features games use but in corner cases it wouldn't do what it's supposed to do.

Given the high probability that I could do the work just to be rejected, I'll wait until someone close to the kernel people gives me an indication that they're really interested and tell me which parts should be improved before approaching upstream.

@motolav
Copy link
Contributor

motolav commented Jan 31, 2023

Nothing has been fixed in the driver, it's that ETS2 has changed the way it uses FFB.

I was testing with the OLD FFB version of ETS2 that's why i specified 1.41 as its the old FFB model, versions 1.42 and after use the new FFB model. There absolutely was an unintentional fix in the driver with the changes after 0.4.0.

Given the high probability that I could do the work just to be rejected, I'll wait until someone close to the kernel people gives me an indication that they're really interested and tell me which parts should be improved before approaching upstream.

Submitting your code or someone else on your behalf to the mailing list is the only real way to actually get help on what needs to be changed

@isopix
Copy link

isopix commented Jan 31, 2023 via email

@berarma
Copy link
Owner

berarma commented Jan 31, 2023

I'll try to get in touch with the kernel maintainers and ask them what would I need to do to upstream it. Maybe it's not that hard.

@berarma
Copy link
Owner

berarma commented Jan 31, 2023

Nothing has been fixed in the driver, it's that ETS2 has changed the way it uses FFB.

I was testing with the OLD FFB version of ETS2 that's why i specified 1.41 as its the old FFB model, versions 1.42 and after use the new FFB model. There absolutely was an unintentional fix in the driver with the changes after 0.4.0.

That's very weird. I'll have to try it.

@motolav
Copy link
Contributor

motolav commented Jan 31, 2023

I'll try to get in touch with the kernel maintainers and ask them what would I need to do to upstream it. Maybe it's not that hard.

Setting up an email client is the hardest part if you want people to be able to compile and test your code from an email. You will likely have to make a new repo/branch to create either a single large commit of all your changes or a few smaller commits of all your major changes.

@motolav
Copy link
Contributor

motolav commented Feb 3, 2023

I pasted your driver into the kernel code see the diff and it seems a bit too much to submit as one commit, 1544 insertions(+), 531 deletions(-). (I manually updated hid-ids.h and hid-lg.c as there was a few changes after your fork)

@howlett
Copy link

howlett commented Jun 14, 2023

Don't worry about your code being bug-free before submitting, as long as it isn't going to break anything that works today. You are adding functionality that is lacking. There are totally bugs, when they are known, they can be fixed with patches.
Break your patches up as you said above, into logical units of work. Make sure each one compiles cleanly.
Run scripts/checkpatch.pl against each one to ensure you have correct formatting.
Set up git to send email and use "git send-email" to submit them. Cc the necessary lists (always lkml), and send it to the maintainer of the subsystem and anyone else that has contributed a lot to the code you are changing (use ./scripts/get_maintainer.pl to find out who)
Base your code on either Linus' branch or the subsystems git reop and tell the maintainer in the cover letter (git format-patch - --cover-letter, edit 000-cover-letter.patch and add details on the changes)

@hzulla
Copy link

hzulla commented Nov 5, 2023

Maybe it's not that hard.

Indeet, it's not. As a total beginner in kernel development, I contributed some minor HID patches and an insignificant gamepad device driver. The HID maintainers were friendly folks and very patient, contributing was a pleasant experience. Your driver is very helpful and would be a very good addition to the mainline kernel. Please be encouraged to go on upstreaming it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants