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

dts: msm8952: Add support for HMD Global Nokia 5 #450

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

andrewgigena
Copy link
Contributor

This pull request adds device tree support for the HMD Global Nokia 5 (nd1) on the msm8952 platform. Also, I added the necessary tools to the build lk2nd over NixOS. Same issue as Nokia 6 lk2nd port: “The only weird thing I've noticed is that the volume keys are swapped around.”

Screenshot working:
nokia-5-lk2nd

I have been able to boot mainstream successfully.
PXL_20241122_135218275

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we want NixOS stuffs. Other than that, just some nits.

- HMD Global Nokia 6 (ple)
- HMD Global Nokia 5 (nd1)

Choose a reason for hiding this comment

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

Suggested change
- HMD Global Nokia 6 (ple)
- HMD Global Nokia 5 (nd1)
- HMD Global Nokia 5 (nd1)
- HMD Global Nokia 6 (ple)

$(LOCAL_DIR)/msm8937-nokia-ple.dtb \
$(LOCAL_DIR)/msm8937-nokia-nd1.dtb \

Choose a reason for hiding this comment

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

Suggested change
$(LOCAL_DIR)/msm8937-nokia-ple.dtb \
$(LOCAL_DIR)/msm8937-nokia-nd1.dtb \
$(LOCAL_DIR)/msm8937-nokia-nd1.dtb \
$(LOCAL_DIR)/msm8937-nokia-ple.dtb \

Comment on lines 11 to 12
&lk2nd {
nd1 {

Choose a reason for hiding this comment

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

Suggested change
&lk2nd {
nd1 {
&lk2nd {
nokia-nd1 {

Copy link
Member

Choose a reason for hiding this comment

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

I'd say it's fine to use either, seems like ple just has codename which is probably fine for case like this, where the msm/board id is mostly device specific anyway

@wonderfulShrineMaidenOfParadise
Copy link
Contributor

For sure we would like to add more devices to lk2nd, so we can add NixOS stuffs later.

@andrewgigena
Copy link
Contributor Author

Maybe we could add the necessary dependencies for NixOS in Documentation/building.md later. For now, I’ve fixed the commit to remove those flake files. What do you think?

Copy link
Member

@TravMurav TravMurav left a comment

Choose a reason for hiding this comment

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

wrt nixos configs: I definitely welcome extra build documentation in the Documentation dir, but I'm not so sure about the nix-specific files in the root...

I don't exactly know much about nix/nixos but per my understanding those files would lock down the toolchain version to whatever is shipped today in nixos, which is something that I don't quite like: i.e. if our code has issues that would've been found by newer gcc, they will be missed by those who use nix since I feel like no one would be updating those lock files. (on top of that, it's a bit unfortunate this seems to only specify x86_64 as I and perhaps few other people working on lk2nd do it on aarch64 machines at times :)

Comment on lines 11 to 12
&lk2nd {
nd1 {
Copy link
Member

Choose a reason for hiding this comment

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

I'd say it's fine to use either, seems like ple just has codename which is probably fine for case like this, where the msm/board id is mostly device specific anyway

Comment on lines 19 to 39
gpio-keys {
compatible = "gpio-keys";
up {
lk2nd,code = <KEY_VOLUMEUP>;
gpios = <&tlmm 91 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
};
};
};
Copy link
Member

Choose a reason for hiding this comment

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

If you say the keys are inverted, perhaps you can configure both volume up and down here, and assign resin (or whatever's the default for 8952) to up, with gpio91 being down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I been trying buttons configurations for a few hours, but none worked. I resolved the issue adding some invert code on keys.c it worked flawless.

Copy link
Member

Choose a reason for hiding this comment

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

no. if you've established that vol- is gpio 92, then put gpio 92 as vol- and explicitly define vol+ as resin - gpios = <&pmic_pon GPIO_PMIC_RESIN 0>; I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that worked.

Copy link
Contributor Author

@andrewgigena andrewgigena Nov 23, 2024

Choose a reason for hiding this comment

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

I mistakenly used gpio 91 as UP when on was clearly as DOWN on downstream

Comment on lines 27 to 31
up {
lk2nd,code = <KEY_VOLUMEUP>;
gpios = <&pmic_pon GPIO_PMIC_RESIN 0>;
};

Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be broken now, let's keep it consistent (8-wide hard tabs)


power {
lk2nd,code = <KEY_POWER>;
gpios = <&pmic_pon GPIO_PMIC_PWRKEY 0>;
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this? should be the default...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I added it to be certain

lk2nd/device/dts/msm8952/msm8937-nokia-nd1.dts Outdated Show resolved Hide resolved
@TravMurav TravMurav merged commit eac9628 into msm8916-mainline:main Nov 23, 2024
14 checks passed
@TravMurav
Copy link
Member

Thanks!

@andrewgigena
Copy link
Contributor Author

Thanks a lot, @TravMurav and @wonderfulShrineMaidenOfParadise

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

Successfully merging this pull request may close these issues.

3 participants