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

Move some led drivers to common folder #13749

Merged
merged 4 commits into from
Jul 31, 2021

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Jul 28, 2021

Description

Not 100% sold on the current "parent folder that only contains 2 files" paradigm. What I would like to see would be something like...

drivers/led/apa102.h
drivers/led/aw20216.h
drivers/led/is31fl3731.h
  ...
drivers/led/is31fl3737.c

However it does at least have some value for issi right now due to the amount of tiles inc. led/rgb variant drivers.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Jul 28, 2021
@zvecr zvecr added the breaking_change Changes that need to wait for a version increment label Jul 28, 2021
@fauxpark
Copy link
Member

Not 100% sold on the current "parent folder that only contains 2 files" paradigm

I have thought (not very hard) about the logistics and worth of moving to a more traditional include/ and src/-style hierarchy. But that's probably a lot of work that can probably only be done once things like tmk_core/ are assimilated into quantum/.

@drashna
Copy link
Member

drashna commented Jul 29, 2021

Ideally, I'd like to see the ISSI drivers in there too, but this is great.

@zvecr
Copy link
Member Author

zvecr commented Jul 29, 2021

Ideally, I'd like to see the ISSI drivers in there too, but this is great.

I was planning to, as part of this PR but still not sure on the issi parent folder.

Done! Apologies in advance for the extra files that have to be reviewed...

@zvecr zvecr marked this pull request as ready for review July 30, 2021 14:52
@zvecr zvecr requested review from a team, drashna and fauxpark July 30, 2021 14:52
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth putting the ISSI drivers into drivers/led/issi/.

@zvecr
Copy link
Member Author

zvecr commented Jul 30, 2021

May be worth putting the ISSI drivers into drivers/led/issi/.

That hass been on the cards for the lifetime of this PR, and no one spoke up about it so i went with my personal preference.

What i want to avoid is the "vendor" folder situation that happened with the other 2 drivers that I've moved. A folder with just 2 files inside it, which had been copied as some form of standard due to the precedence set with the issi folder.

@fauxpark
Copy link
Member

The ISSI drivers warrant their own directory due to the sheer number of them, whereas anything directly under drivers/led/ can be seen as "miscellaneous", until we collect enough like ones to group them together.

@zvecr
Copy link
Member Author

zvecr commented Jul 30, 2021

I personally dont see the value, but if it helps get this across the line then i will make the change, Just a little disappointed that it was up for grabs and its only after ive made the change that i now have to do more work.

@fauxpark
Copy link
Member

I don't really see where you mentioned this.

@zvecr zvecr force-pushed the feature/led_driver_folder branch from f257a33 to 7564eff Compare July 30, 2021 15:47
@zvecr
Copy link
Member Author

zvecr commented Jul 30, 2021

In the PR description. Doesnt matter anyway, ive moved the files keeping the issi folder.

@fauxpark
Copy link
Member

Wasn't really clear to me what you were saying there, the issi folder isn't a "parent folder that only contains 2 files" but I understand now.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm fine with throwing everything in /drivers/led, since the naming helps sort them out. But either way is absolutely fine, IMO.

@zvecr zvecr merged commit 206a995 into qmk:develop Jul 31, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
* Move some led drivers to common folder
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Move some led drivers to common folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change Changes that need to wait for a version increment core documentation keyboard translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants