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

ucm2: add profile for the Librem 5 #181

Closed
wants to merge 1 commit into from

Conversation

craftyguy
Copy link
Contributor

cc @agx

@agx
Copy link
Contributor

agx commented Jun 30, 2022

@craftyguy thanks for taking a look at this! Would be great to have it upstream. There's rework pending here https://source.puri.sm/Librem5/librem5-base/-/merge_requests/311 which might make sense to have upstream right away. What do you think?

@craftyguy
Copy link
Contributor Author

There's rework pending here https://source.puri.sm/Librem5/librem5-base/-/merge_requests/311 which might make sense to have upstream right away. What do you think?

@agx I saw that, and based on the comments it still seemed WIP with the last update a couple of months ago... So I thought it would be better to upstream a known working config, then it could be tweaked later to perfection.

But obviously if it's in much better shape than what I think from reading the discussion, I'd have no problem updating this patch. My main motivation is to just get something upstream to hopefully help with audio on this platform breaking every time alsa is updated.

@agx
Copy link
Contributor

agx commented Jun 30, 2022

@craftyguy yeah I understand that, it's a drag with that being only downstream. That said I'd go for the reworked one as otherwise we might hit odd mixer settings when switching to the newer version.

@craftyguy craftyguy marked this pull request as draft July 1, 2022 07:09
@agx
Copy link
Contributor

agx commented Jul 29, 2022

Given that reworking the profile seems to have stalled I think this would be good to have as a starting point! (Tested on my Librem 5)

@agx
Copy link
Contributor

agx commented Aug 2, 2022

and just in case:

Tested-by: Guido Günther <agx@sigxcpu.org>

@craftyguy
Copy link
Contributor Author

@agx thanks!

@perexg anything else you need for this to be accepted?

Copy link
Member

@perexg perexg left a comment

Choose a reason for hiding this comment

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

Please, resolve my objections for all devices (MixerElem, 2 channels, shorter PCM devices).

ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
@craftyguy
Copy link
Contributor Author

@perexg thank you for the detailed feedback, I appreciate it! I tried to implement the changes you called for, by looking at other conf for examples, but I'm really not a ucm expert so please let me know if I misinterpreted something you said / more changes are needed for this to be acceptable 😄

Copy link
Member

@perexg perexg left a comment

Choose a reason for hiding this comment

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

The verb should also call DisableSequence for all devices when enabled like:

https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/HDA/HiFi.conf#L3-L9

ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Show resolved Hide resolved
ucm2/NXP/iMX8/Librem_5/HiFi.conf Outdated Show resolved Hide resolved
Tested-by: Guido Günther <agx@sigxcpu.org>
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.

None yet

3 participants