[alsa-ucm-conf,v3,1/2] sof-soundwire: Add basic support for cs42l43

cs42l43 is a codec device, add basic support for it. Including a dual
channel DMIC input, stereo headphones, and a mono headset microphone.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

No changes since v2.

Thanks,
Charles

 ucm2/sof-soundwire/cs42l43-dmic.conf | 28 +++++++++++++++++
 ucm2/sof-soundwire/cs42l43.conf      | 47 ++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 ucm2/sof-soundwire/cs42l43-dmic.conf
 create mode 100644 ucm2/sof-soundwire/cs42l43.conf

On 06. 12. 23 17:46, Charles Keepax wrote:
> cs35l56 is a boosted speaker amp, add UCM support for configurations
> with up to 8 amps.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v2:
>   - Rebased on top of conversion of the Realtek amps.
>   - Add a macro for each amp to simplify things a bit.

Thanks. This patch was inspiration for me. Could you check modifications in 
https://github.com/alsa-project/alsa-ucm-conf/pull/370 ? We can use regex to 
create condition against SpeakerAmps variable, so the configuration may look like:

...
	Condition {
		Type RegexMatch
		Regex "${var:__ForAmps}"
		String "${var:SpeakerAmps}"
	}
...
	Macro.num1.cs42l43spk { ForAmps "[12468]" Amp 1 }
	Macro.num2.cs42l43spk { ForAmps "[2468]" Amp 2 }
	Macro.num3.cs42l43spk { ForAmps "[468]" Amp 3 }
	Macro.num4.cs42l43spk { ForAmps "[468]" Amp 4 }
	Macro.num5.cs42l43spk { ForAmps "[68]" Amp 5 }
	Macro.num6.cs42l43spk { ForAmps "[68]" Amp 6 }
	Macro.num7.cs42l43spk { ForAmps "8" Amp 7 }
	Macro.num8.cs42l43spk { ForAmps "8" Amp 8 }
...

I assume that only even count for amplifiers is valid (with mono exception).

						Jaroslav

On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
> On 06. 12. 23 17:46, Charles Keepax wrote:
> >cs42l43 is a codec device, add basic support for it. Including a dual
> >channel DMIC input, stereo headphones, and a mono headset microphone.
> >
> >Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> >---
> >+	Value {
> >+		CapturePriority 100
> >+		CapturePCM "hw:${CardId},4"
> >+	}
> >+}
> 
> Just curious: Why dmic input does not have Decimator switch/volume
> controls like Headset output?
> 
> We can combine mono controls to one stereo in latest UCM.

Oh, I was not aware we could do that. I would yes much rather
handle the switches and volumes in this way. I will see if I can
figure it out, but if you had any good examples that already
exist that would really be handy?

Thanks,
Charles

On Wed, Dec 06, 2023 at 06:46:18PM +0100, Jaroslav Kysela wrote:
> On 06. 12. 23 17:46, Charles Keepax wrote:
> >cs35l56 is a boosted speaker amp, add UCM support for configurations
> >with up to 8 amps.
> >
> >Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> >---
> >
> >Changes since v2:
> >  - Rebased on top of conversion of the Realtek amps.
> >  - Add a macro for each amp to simplify things a bit.
> 
> Thanks. This patch was inspiration for me. Could you check
> modifications in
> https://github.com/alsa-project/alsa-ucm-conf/pull/370 ? We can use

Some comments on the Github, would you rather I submitted my
patches through a github pull request as well? I am happy using
either that or email to submit the patches, so if you have a
preference I will do that.

Thanks,
Charles

On 07. 12. 23 10:55, Charles Keepax wrote:
> On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
>> On 06. 12. 23 17:46, Charles Keepax wrote:
>>> cs42l43 is a codec device, add basic support for it. Including a dual
>>> channel DMIC input, stereo headphones, and a mono headset microphone.
>>>
>>> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>>> ---
>>> +	Value {
>>> +		CapturePriority 100
>>> +		CapturePCM "hw:${CardId},4"
>>> +	}
>>> +}
>>
>> Just curious: Why dmic input does not have Decimator switch/volume
>> controls like Headset output?
>>
>> We can combine mono controls to one stereo in latest UCM.
> 
> Oh, I was not aware we could do that. I would yes much rather
> handle the switches and volumes in this way. I will see if I can
> figure it out, but if you had any good examples that already
> exist that would really be handy?

You may look for "LibraryConfig.remap.Config" and "Include.ctl-remap.File" 
strings in .conf files.

					Jaroslav

On Thu, Dec 07, 2023 at 02:56:17PM +0100, Jaroslav Kysela wrote:
> On 07. 12. 23 10:55, Charles Keepax wrote:
> >On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
> >>On 06. 12. 23 17:46, Charles Keepax wrote:
> >Oh, I was not aware we could do that. I would yes much rather
> >handle the switches and volumes in this way. I will see if I can
> >figure it out, but if you had any good examples that already
> >exist that would really be handy?
> 
> You may look for "LibraryConfig.remap.Config" and
> "Include.ctl-remap.File" strings in .conf files.

Apologies still struggling to get this working. I think there must
some important boiler plate or limitation I am missing. Would really
appreciate if you could have a look at this and let me know if it
looks sane. I am starting out with just the simplest thing I can
think of, just trying to rename a control:

LibraryConfig.remap.Config {
	ctl.default.remap {
		"name='cs42l43 PDM2 Switch'" "name='cs42l43 Decimator 3 Switch'"
	}
}

SectionDevice."Mic"
{
	Comment "Microphones"

	EnableSequence
	[
		cset "name='cs42l43 PDM2 Switch' 1"
	]

	DisableSequence
	[
		cset "name='cs42l43 PDM2 Switch' 0"
	]

	Value
	{
		CapturePriority 100
		CapturePCM "hw:${CardId},4"
	}
}

Everything works as expected if I use "cs42l43 Decimator 3 Switch"
directly in the use-case, however if I use "cs42l43 PDM2 Switch"
I get the error:

ALSA lib main.c:826:(execute_sequence) unable to execute cset 'name='cs42l43 PDM2 Switch' 0'
ALSA lib main.c:2573:(set_verb_user) error: failed to initialize new use case: HiFi

The LibraryConfig bit doesn't seem to cause any errors in its own
right, but the error messae suggests to me it didn't add the alias
for the control. I have tried a lot of variations on the code,
but I can't seem to locate what I am doing wrong.

Also if there are any docs I should read happy to go there first?

Thanks,
Charles

On Fri, Dec 08, 2023 at 12:00:26PM +0000, Charles Keepax wrote:
> On Thu, Dec 07, 2023 at 02:56:17PM +0100, Jaroslav Kysela wrote:
> > On 07. 12. 23 10:55, Charles Keepax wrote:
> > >On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
> > >>On 06. 12. 23 17:46, Charles Keepax wrote:
> > >Oh, I was not aware we could do that. I would yes much rather
> > >handle the switches and volumes in this way. I will see if I can
> > >figure it out, but if you had any good examples that already
> > >exist that would really be handy?
> > 
> > You may look for "LibraryConfig.remap.Config" and
> > "Include.ctl-remap.File" strings in .conf files.
> 
> Apologies still struggling to get this working. I think there must
> some important boiler plate or limitation I am missing. Would really
> appreciate if you could have a look at this and let me know if it
> looks sane. I am starting out with just the simplest thing I can
> think of, just trying to rename a control:
> 

Ok it seems starting with the simplest thing was not the best
idea :-)

If you only have a remap, no map then it looks like one needs to
do something like this:

+++ b/src/control/control_remap.c
@@ -1192,7 +1192,7 @@ int snd_ctl_remap_open(snd_ctl_t **handlep, const char *name, snd_config_t *rema
                goto _err;
        }
 
-   priv->numid_remap_active = priv->map_items > 0;
+ priv->numid_remap_active = priv->map_items > 0 || priv->remap_items > 0;

Otherwise the check at the start of remap_find_numid_app will
keep failing and you get stuck in an infinite loop in
remap_numid_child_new. I will likely send a patch soon (might
slip into the new year, getting close to the end of the year and I
am rather unfamiliar with the user side of the alsa code), but I
want double check things a little more first.

Any input on this change would be greatly appreciated?

> LibraryConfig.remap.Config {
> 	ctl.default.remap {
> 		"name='cs42l43 PDM2 Switch'" "name='cs42l43 Decimator 3 Switch'"
> 	}
> }

To answer part of my own question, and in the hope that if anyone
else is having similar difficulties they will find this thread,
the remapping works the other way around, it should go:

"name='cs42l43 Decimator 3 Switch'" "name='cs42l43 PDM2 Switch'"

With the newly mapped control second, kinda confusing as the .map
sections do it the other way around, but fair enough.

> 
> SectionDevice."Mic"
> {
> 	Comment "Microphones"
> 
> 	EnableSequence
> 	[
> 		cset "name='cs42l43 PDM2 Switch' 1"
> 	]
> 
> 	DisableSequence
> 	[
> 		cset "name='cs42l43 PDM2 Switch' 0"
> 	]
> 
> 	Value
> 	{
> 		CapturePriority 100
> 		CapturePCM "hw:${CardId},4"
> 	}
> }

It would seem the primary issue is here, one needs to add:

CaptureCTL "default:${CardId}"
PlaybackCTL "default:${CardId}"

This is because the control device will normally be the hw: one,
which doesn't have the renaming applied to it, you need to switch
to the device with the remapping, which was added as the one
named default in lib/ctl-remap.conf.

Seems weird you have to do the PlaybackCTL in a capture only use
case, but otherwise it complains about them being mismatched. Also
weirdly you need to add this into some other devices in the same
use-case, otherwise it appears it just ignores these lines and
uses the hw device anyway. Still looking at what is going on with
that but I would appreciate any thoughts on that as well?

> Also if there are any docs I should read happy to go there first?

Updating this to any suggestions on where to add some docs would
also be appreciated?

Hopefully I can find sometime to document some of this a little
and save someone else spending the quite large amount of time I
have sunk into working this lot out so far.

> 

Finally, does anyone have any idea what is going on with the
current users of the remap. It looks like rt5660, rt5677, rt5651,
rt5645, rt5640, rt5682 all currently have remap sections in their
config. However almost the remapped controls are never used, which
might not be surprising given the likely bug at the start of this
email. But curious if anyone has any ideas that the remapping is
actually being used for something non-obvious on those devices?
Kinda wonder if we should just remove some of the unused
remapping as I found it quite confusing whilst trying to figure
this out.

Anyway any extra thoughts/corrections on all this would be really
really appreciated. I am still very new to the ALSA userspace
side of things, so sure I am still making lots of mistakes.

Thanks,
Charles

On 19. 12. 23 17:45, Charles Keepax wrote:
> On Fri, Dec 08, 2023 at 12:00:26PM +0000, Charles Keepax wrote:
>> On Thu, Dec 07, 2023 at 02:56:17PM +0100, Jaroslav Kysela wrote:
>>> On 07. 12. 23 10:55, Charles Keepax wrote:
>>>> On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
>>>>> On 06. 12. 23 17:46, Charles Keepax wrote:
>>>> Oh, I was not aware we could do that. I would yes much rather
>>>> handle the switches and volumes in this way. I will see if I can
>>>> figure it out, but if you had any good examples that already
>>>> exist that would really be handy?
>>>
>>> You may look for "LibraryConfig.remap.Config" and
>>> "Include.ctl-remap.File" strings in .conf files.
>>
>> Apologies still struggling to get this working. I think there must
>> some important boiler plate or limitation I am missing. Would really
>> appreciate if you could have a look at this and let me know if it
>> looks sane. I am starting out with just the simplest thing I can
>> think of, just trying to rename a control:
>>
> 
> Ok it seems starting with the simplest thing was not the best
> idea :-)
> 
> If you only have a remap, no map then it looks like one needs to
> do something like this:
> 
> +++ b/src/control/control_remap.c
> @@ -1192,7 +1192,7 @@ int snd_ctl_remap_open(snd_ctl_t **handlep, const char *name, snd_config_t *rema
>                  goto _err;
>          }
>   
> -   priv->numid_remap_active = priv->map_items > 0;
> + priv->numid_remap_active = priv->map_items > 0 || priv->remap_items > 0;

It's not a correct fix. I applied this quick fix with your Reported-by
tag:

@@ -148,7 +148,7 @@ static snd_ctl_numid_t 
*remap_numid_child_new(snd_ctl_remap_t *priv, unsigned in

         if (numid_child == 0)
                 return NULL;
-       if (remap_find_numid_app(priv, numid_child)) {
+       if (priv->numid_remap_active && remap_find_numid_app(priv, numid_child)) {

It seems that I did tests only with 'amixer set/get' commands and not 'amixer 
cget/cset' commands. The PA/pipewire uses the simple mixer API (set/get) so I 
have not noticed this issue. Thanks for reporting.

>> LibraryConfig.remap.Config {
>> 	ctl.default.remap {
>> 		"name='cs42l43 PDM2 Switch'" "name='cs42l43 Decimator 3 Switch'"
>> 	}
>> }
> 
> To answer part of my own question, and in the hope that if anyone
> else is having similar difficulties they will find this thread,
> the remapping works the other way around, it should go:
> 
> "name='cs42l43 Decimator 3 Switch'" "name='cs42l43 PDM2 Switch'"
> 
> With the newly mapped control second, kinda confusing as the .map
> sections do it the other way around, but fair enough.

The description for the alsa-lib's remap plugin is here:

https://www.alsa-project.org/alsa-doc/alsa-lib/control_plugins.html

> It would seem the primary issue is here, one needs to add:
> 
> CaptureCTL "default:${CardId}"
> PlaybackCTL "default:${CardId}"

Look for 'PlaybackMixer "default:' strings in configs for sound servers and 
PlaybackMixerElem corresponding values. Sound servers does not use the control 
API directly but the simple mixer API.

The LibraryConfig blocks are added to the standard configuration and there are 
2 ways to use them.

1) private configuration - _ucm####. prefix (only in memory for UCM apps)
2) blocks can be saved using cfg-save sequence command (used in 
ucm2/lib/ctl-remap.conf)

The second case - ctl-remap.conf - should save new configurations to 
/var/lib/alsa/card#.conf.d and the global configuration 
(/usr/share/alsa/alsa.conf) will include them. So the default devices should 
be modified. You may also prepare/test configs in ~/.asoundrc and then copy 
them to ucm configuration files.

Also note that the remapping is for the application side (API), UCM sequences 
are using the direct hw: controls.

Some notes are also here: 
https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/DEBUG.md

>> Also if there are any docs I should read happy to go there first?
> 
> Updating this to any suggestions on where to add some docs would
> also be appreciated?
> 
> Hopefully I can find sometime to document some of this a little
> and save someone else spending the quite large amount of time I
> have sunk into working this lot out so far.

Yes, documentation needs more improvements. The contents should go probably to 
https://www.alsa-project.org/alsa-doc/alsa-lib/group__ucm__conf.html 
(ucm_confdoc.h in alsa-lib's source tree).

> Finally, does anyone have any idea what is going on with the
> current users of the remap. It looks like rt5660, rt5677, rt5651,
> rt5645, rt5640, rt5682 all currently have remap sections in their
> config. However almost the remapped controls are never used, which
> might not be surprising given the likely bug at the start of this
> email. But curious if anyone has any ideas that the remapping is
> actually being used for something non-obvious on those devices?
> Kinda wonder if we should just remove some of the unused
> remapping as I found it quite confusing whilst trying to figure
> this out.

The configs are for standard applications (case 2 - ctl-remap.conf).

				Jaroslav

On Tue, Dec 19, 2023 at 07:29:21PM +0100, Jaroslav Kysela wrote:
> On 19. 12. 23 17:45, Charles Keepax wrote:
> >On Fri, Dec 08, 2023 at 12:00:26PM +0000, Charles Keepax wrote:
> >>On Thu, Dec 07, 2023 at 02:56:17PM +0100, Jaroslav Kysela wrote:
> >>>On 07. 12. 23 10:55, Charles Keepax wrote:
> >>>>On Wed, Dec 06, 2023 at 06:26:17PM +0100, Jaroslav Kysela wrote:
> >>>>>On 06. 12. 23 17:46, Charles Keepax wrote:
> @@ -148,7 +148,7 @@ static snd_ctl_numid_t
> *remap_numid_child_new(snd_ctl_remap_t *priv, unsigned in
> 
>         if (numid_child == 0)
>                 return NULL;
> -       if (remap_find_numid_app(priv, numid_child)) {
> +       if (priv->numid_remap_active && remap_find_numid_app(priv, numid_child)) {
> 

This fix seems to work for me, thanks.

> >It would seem the primary issue is here, one needs to add:
> >
> >CaptureCTL "default:${CardId}"
> >PlaybackCTL "default:${CardId}"
> 
> Look for 'PlaybackMixer "default:' strings in configs for sound
> servers and PlaybackMixerElem corresponding values. Sound servers
> does not use the control API directly but the simple mixer API.
> 
> The LibraryConfig blocks are added to the standard configuration and
> there are 2 ways to use them.
> 
> 1) private configuration - _ucm####. prefix (only in memory for UCM apps)
> 2) blocks can be saved using cfg-save sequence command (used in
> ucm2/lib/ctl-remap.conf)
> 
> The second case - ctl-remap.conf - should save new configurations to
> /var/lib/alsa/card#.conf.d and the global configuration
> (/usr/share/alsa/alsa.conf) will include them. So the default
> devices should be modified. You may also prepare/test configs in
> ~/.asoundrc and then copy them to ucm configuration files.
> 
> Also note that the remapping is for the application side (API), UCM
> sequences are using the direct hw: controls.

Can I just check I follow here, you are saying it would be
unexpected to use the remapped controls in the ucm configuration
itself, only other applications would be expected to use the
remapped controls?

Thank you very much for the detailed reply, there is a lot for me
to think through there so I will try to go through that and
likely be back with a new spin of the patch in the new year.

Thanks,
Charles