-
Notifications
You must be signed in to change notification settings - Fork 140
ASoC: partial match the sdca codec name #5664
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: topic/sof-dev
Are you sure you want to change the base?
ASoC: partial match the sdca codec name #5664
Conversation
| if (dai_info->codec_name) { | ||
| struct snd_soc_component *component; | ||
|
|
||
| component = snd_soc_lookup_component_by_name(dai_info->codec_name); |
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.
Can we be sure the component exists by this point?
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.
Can we be sure the component exists by this point?
No, but in theory, the function will be called again when the component probed and be registered.
|
Ok well it mostly works for me, will investigate the probe concerns a little more. One thing that is missing though is the handling of aux devices on the link, we added very basic support for this for the HID device and it would also need renaming. |
Oh, yes, I missed |
3495316 to
e4b195e
Compare
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 I think this looks good to me and it tests out fine on my setup here even with the function disabled as it is on the customer system.
EDIT: Also thank you very much for having a look at this, been quite slammed at the moment and it's a big help.
ujfalusi
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.
@bardliao and how we are going to find the other codec?
snd_soc_sdca.UAJ.1, snd_soc_sdca.UAJ.2 and we are looking for snd_soc_sdca.UAJ if I got the idea right.
sound/soc/soc-core.c
Outdated
| struct snd_soc_component *component; | ||
|
|
||
| mutex_lock(&client_mutex); | ||
| component = snd_soc_lookup_component_by_name_nolocked(component_name); |
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 this helps much in readability, just move the for_each... here
and use guard(mutex)(&client_mutex); instead of the manual lock/unlock, it takes care of unlock at returns
For now, we will have only one device for each SDCA function. We will figure out how to handle the case when it really happens. |
Add a helper to help user to get the component by name. Signed-off-by: Bard Liao <[email protected]>
Currently, we can set codec name in the dai info which will be set as the codec component name in a DAI link. However, the codec name may not be fixed. For example, there is an index in a SDCA codec name and that is not fixed. Lookup the fixed codec name string from the component list to get the right component name to ensure the DAI link will bind to the right codec component. Signed-off-by: Bard Liao <[email protected]>
The index is not fixed and it will lead to the DAI link can't bind the codec component with the name when the index is different from the predefined one. Signed-off-by: Bard Liao <[email protected]>
e4b195e to
0f09026
Compare
Hrm, so is it like that there is only a single codec with a unique name with a random number tossed to it's name for whatever reason? |
Yes, but the number is not random. I think it is the order of the function being listed in the DisCo table. @charleskeepax Please correct me if I am wrong. |
|
Yeah its an IDA in the SDCA auxiliary, typically it will be the order things are registered although I don't think that is guaranteed if drivers are removed and added. |
Right, but can we have |
|
As Bard notes it won't with the current code, but we don't currently have any devices doing that. We likely will in the future but we are hoping to save that problem for another day. |
I'm perfectly aware that we won't with the current code, I'm asking if is this a valid concern or just a perfectionists problem. |
|
No, it's a real problem. UAJ stands for Universal Audio Jack, it is one of several functions that can exist on an SDCA audio part. Parts can have multiple functions and there are no rules against a part having multiple instances of the same function, say in the UAJ case this might be a part that supported dual jack sockets. There are currently no parts supported by the SDCA driver that have duplicate functions, but it is likely there will be in the future. |
| { | ||
| if (dai_info->codec_name) | ||
| return devm_kstrdup(dev, dai_info->codec_name, GFP_KERNEL); | ||
| if (dai_info->codec_name) { |
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.
what is the "partial match" you mention in your commit in the code?
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 removed the index from the codec_name, and use the partial codec name to match the component name.
|
@ujfalusi I understand that we will need to handle the duplicate function case sometime in the future. But this PR can buy us some time to solve the current issue. Unless we can find a way to handle the duplicate function case shortly, otherwise I would suggest that we go with the solution to unblock the OEM release. |
Currently, we set a predefined codec component name in a DAI link. But the codec name may contain an index which is not fixed. This series suggest using partial match the codec name to fix the issue.