-
Notifications
You must be signed in to change notification settings - Fork 22
Add pinetab2 support #33
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
base: master
Are you sure you want to change the base?
Conversation
alistair23
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Overall looks good, just a few comments
conf/machine/pine-pinetab2.conf
Outdated
| @@ -0,0 +1,45 @@ | |||
| #@TYPE: Machine | |||
| #@NAME: Pine PineTab2 | |||
| #@DESCRIPTION: An Rockchip RK356x based linux tablet https://www.pine64.org/ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Rockchip RK356x
| @@ -0,0 +1,25 @@ | |||
| DESCRIPTION = "Mainline Linux kernel with built-in wifi and USB support for PineTab2" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not mainline, it's a fork
Do we need this fork or can we use mainline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, unfortunately usb and wifi won't work without the patches provided in the danctnix kernel fork. I thought about just providing patches on top of yocto kernel but then it might break at some point in the future. With this recipe at least, one can easily just check newest release and change revisions to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a really small number of patches required then patches on top of the Yocto kernel is a bit cleaner. It makes it obvious what is actually required.
Otherwise a fork is fine (if there is good justification) but we should call it a fork in the description as it isn't mainline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, another thing is that WiFi/BT driver is not yet stable and there is still some development going on so additional patches will be most certainly added at some point in the future.
I thought about adding non-WiFi/BT module patches on top of yocto kernel and providing WiFi/BT driver as out of tree module but from what i saw recent changes to the WiFi/BT driver in the fork are accompanied by some device tree changes so it does not seem to be viable option (+ the fact that development of the driver for the fork happens in-the-tree)
I think it's best to just leave the fork for now and eventually switch to yocto + patches in the future when things stabilize.
| do_install() { | ||
| install -d ${D}${nonarch_base_libdir}/firmware | ||
| cp -r ${S}/firmware/* ${D}${nonarch_base_libdir}/firmware/ | ||
| install -m 0644 ${S}/LICENSE ${D}${nonarch_base_libdir}/firmware/LICENSE.bes2600 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to include this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the license file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the license file. I don't think we need to install it
Main differences compared to other machines: