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

Enable vchiq on ARCH_BCM2835 #973

Merged
merged 3 commits into from
May 19, 2015
Merged

Enable vchiq on ARCH_BCM2835 #973

merged 3 commits into from
May 19, 2015

Conversation

notro
Copy link
Contributor

@notro notro commented May 19, 2015

Tested on Pi1 and Pi2 regular kernel with and without DT + ARCH_BCM2835.

Tested from userspace:

$ vcgencmd measure_temp
temp=34.7'C

notro added 3 commits May 19, 2015 13:31
Prepare to turn the vchiq module into a driver.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Turn vchiq into a driver and stop hardcoding resources.
Use devm_* functions in probe path to simplify cleanup.
A global variable is used to hold the register address. This is done
to keep this patch as small as possible.
Also make available on ARCH_BCM2835.
Based on work by Lubomir Rintel.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Add vchiq to Device Tree. There are no kernel users yet,
but it's available to userspace (vcgencmd).

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
@popcornmix
Copy link
Collaborator

Fine by me. @pelwell - this is your code - feel free to merge if happy.

@pelwell
Copy link
Contributor

pelwell commented May 19, 2015

I'm fine with the substance of the change - thanks! - but I'm not keen on the division into multiple commits. I would argue that the first two patches should be combined into one, since the first patch is useless without the second, and causing the kernel to attempt to load what is effectively a non-existent module (because of the lack of a "compatible" string), though harmless, seems like bad form when it could so easily be avoided.

@notro
Copy link
Contributor Author

notro commented May 19, 2015

It is common practice in mainline to split driver commits from device commits. Actually the first commit would probably have been 3 commits in mainline.
If it's a new driver, there would be a separate commit for enabling the module as well.
There is no problem having a device with no matching driver. We already have one device added in the board file without an enabled driver: bcm2835_hwmon_device

Following your logic all three should be squashed.

One problem with having large commits is that it makes it more difficult to browse the git log for changes. This is quite a big project so many people are reading the git log.

But I'll squash the first two if you want it that way.

@pelwell
Copy link
Contributor

pelwell commented May 19, 2015

Any thoughts, @popcornmix?

@popcornmix
Copy link
Collaborator

I guess it depends on whether we can see these commits being upstreamed.
If they are expected to be upstreamed, then having commits in a form suitable for upstreaming is valid.

If they are not being upstreamed, then I generally prefer the minimal number of commits. It generally makes cherry-picking, rebasing and bisecting simpler if you don't have a long list of dependent commits.

@pelwell
Copy link
Contributor

pelwell commented May 19, 2015

I wouldn't have thought anything of it if the two device commits weren't on opposite sides of the driver commit, but if this is as you would like it to be then I'm not going to be awkward about it.

pelwell added a commit that referenced this pull request May 19, 2015
Enable vchiq on ARCH_BCM2835
@pelwell pelwell merged commit e72ad4b into raspberrypi:rpi-4.0.y May 19, 2015
@notro notro deleted the vchiq branch May 19, 2015 13:32
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