Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
PAZ00/tegraalc5632: move to Tegra/alc5632 tree
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
- Loading branch information
Showing
5 changed files
with
3 additions
and
27 deletions.
There are no files selected for viewing
File renamed without changes.
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../Tegra/alc5632/alc5632.conf |
This file was deleted.
Oops, something went wrong.
8ff2d50There 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.
@perexg Hello, I'm looking at upstreaming two UCMs for Tegra tablet devices and got very confused by the changes you made recently. Could you please explain why you turned a device-specific UCMs into platform-specific? This looks very wrong to me because each device has its own specifics in regards to hardware configuration, hence the UCM configs can't be re-used even if they targeting the same audio codec. I think those changes need to be reverted.
8ff2d50There 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 just added another level to the config files. If there's a common configuration, it can be moved to a common include path. I just work with the current configurations. The new level clearly identify the platform and PAZ00 was confusing for me without any explanation.
8ff2d50There 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.
Previous the UCM rules were matching to a specific device by the model name, while now these rules should match to any Tegra device, unless I'm missing something. For example, any Tegra device which has ALC5632 codec, will pick up the PAZ00 rule. PAZ00 isn't a platform, it's a netbook device, while Tegra is platform which is used by a lot of different devices.
8ff2d50There 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.
Right, but many parts can be shared. You can create conditions to match against some key (components string, card long name etc.) for your device. See https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/chtrt5645/HiFi.conf for an example. I mean that you can move PAZ00 config to another place or reuse some parts from it for your new device.
8ff2d50There 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.
But you haven't added the conditions to the moved rules, so now these rules should match to unrelated devices, potentially breaking audio. The conditions approach also should become very messy once a single rule file is re-used by a dozen devices. Is it possible to have a
Tegra/device/...structure instead ofTegra/codec/for the UCM rules?8ff2d50There 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.
The support for new devices should refine the current configuration. If you show me a configuration for your device, we can talk about details.
8ff2d50There 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.
My point is that yours change may upset users of out-of-tree devices because those moved UCMs now will be picked up for them. Of course it's fine if it's yours conscious decision, but it is not very user-friendly ;)
The configurations are here https://github.com/grate-driver/alsa-ucm-conf/commits/master, last two commits. Support for these devices just has been merged into v5.9 upstream Linux kernel. I'll probably need to rebase and move the configs to
Tegra/now, but not sure what to do about theTegra/codec/. Is it possible to move these configs toTegra/device/?8ff2d50There 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.
The behaviour can be changed in ucm2/ucm conf file. Just set
Define.V2Module** ""(empty string) to disable the module name based filter to prefer the card name based behaviour. It's clear.It appears that you're writing configs for different codecs, thus the top level asoc module name should be different, doesn't? Just follow the ucm2/module/snd_soc_tegra_alc5632.conf example.
Also, you should check your changes in the ucm validator. I see that your device names are just wrong (outside the recent naming standard) for example.
8ff2d50There 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.
IIUC, the card name filtering should always take precedence over the module filtering, isn't it the default case for the UCMs filtering?
I'm not sure why do I need to disable the module filtering, my goal is to have audio working out-of-the box on all Linux disros without a need from user to change anything once alsa-ucm-conf package is installed. So it's not clear to me why do I need to change anything in the
ucm2/ucm.conffile.Yes, my configs are for the different codecs. I'm just replying here to the alc5632 patch because it caught my eye first. To make clear my point: the "Tegra alc5632" != "PAZ00 alc5632", same for the max98090. Hence it should be incorrect to use the UCM rules that are intended for a specific devices on all Tegra devices that have alc5632 or max98090 CODECs, it's still not clear to me why you made that change. In general the "common" UCM can't exist for different devices because the hardware is wired very differently on every device, thus all audio routing and configurations are very different, and hence each device (model) may need to have its own/individual rules.
I checked the changes in the validator and it has no complains. The names should be correct because that's how the cards are named in the kernel/device-trees.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts?#n981
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/tegra30-asus-nexus7-grouper-common.dtsi?#n1166
It is not clear to me what do you mean by the "naming standard", could you please clarify what do you mean by the
standard? Do you mean this new alsa-ucm-conf naming convention where you moved PAZ00 and Nyan UCMs to theTegra/codecdirectories? Or are you talking about something else? I'm not following closely ALSA changes, so will be great if you could give a bit more detailed explanations in order to get me on the same page.Should I move the Acer A500 and Nexus 7 UCMs to
Tegra/wm8903andTerga/rt5640respectively and then use the regex conditions for the card-name matching?8ff2d50There 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.
Yes, it's expected to refine the configs when new hardware is added. Usually, there are only small changes required to add new platform which is using the similar hardware.
It's a bit nonsense to have three fields describing the card with the identical contents.
Yes with the conf.d/<driver_name>/<long_card_name>.conf redirect (symlink).