ASoC: soc_sdw_utils: skip the endpoint that doesn't present #5188
Conversation
This structure is used to copy information from the 'sdw_slave' structures, it's better to create a flexible array of 'sdw_slave' pointers and directly access the information. This will also help access additional information stored in the 'sdw_slave' structure, such as an SDCA context. This patch does not add new functionality, it only modified how the information is retrieved. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add new module for SDCA (SoundWire Device Class for Audio) support. For now just add a parser to identify the SDCA revision and the function mask. Note that the SDCA definitions and related MIPI DisCo properties are defined only for ACPI platforms and extracted with _DSD helpers. There is currently no support for Device Tree in the specification, the 'depends on ACPI' reflects this design limitation. This might change in a future revision of the specification but for SDCA 1.0 ACPI is the only supported type of platform firmware. The SDCA library is defined with static inline fallbacks, which will allow for unconditional addition of SDCA support in common parts of the code. The design follows a four-step process: 1) Basic information related to Functions is extracted from MIPI DisCo tables and stored in the 'struct sdw_slave'. Devm_ based memory allocation is not allowed at this point prior to a driver probe, so we only store the function node, address and type. 2) When a codec driver probes, it will register subdevices for each Function identified in phase 1) 3) a driver will probe for each subdevice and addition parsing/memory allocation takes place at this level. devm_ based allocation is highly encouraged to make error handling manageable. 4) Before the peripheral device becomes physically attached, register access is not permitted and the regmaps are cache-only. When peripheral device is enumerated, the bus level uses the 'update_status' notification; after optional device-level initialization, the codec driver will notify each of the subdevices so that they can start interacting with the hardware. Note that the context extracted in 1) should be arguably be handled completely in the codec driver probe. That would however make it difficult to use the ACPI information for machine quirks, and e.g. select different machine driver and topologies as done for the RT712_VB handling later in the series. To make the implementation of quirks simpler, this patchset extracts a minimal amount of context (interface revision and number/type of Functions) before the codec driver probe, and stores this context in the scope of the 'struct sdw_slave'. The SDCA library can also be used in a vendor-specific driver without creating subdevices, e.g. to retrieve the 'initialization-table' values to write platform-specific values as needed. For more technical details, the SDCA specification is available for public downloads at https://www.mipi.org/mipi-sdca-v1-0-download Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use SDCA helpers to get the basic information and store it in the slave context. The information will be optionally be used in codec drivers to register sub-devices for each Function. When platforms are not based on ACPI the helpers do absolutely nothing. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We shouldn't do any devm_ based allocation in the io_init(), this need to happen in the probe(). Luckily, we now have an SDCA helper to look in ACPI tables if a SMART_MIC function is exposed. FIXME: the registers are not well handled today, the regmap lists registers which are not really supported in all platforms. The regmap needs to throw an error if those registers are accessed without existing. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing machine_quirk() returns a pointer to a soc_acpi_mach structure. For SoundWire/SDCA support, we need a slightly different functionality where a quirk function either validates or NACKs an initial selection, based on additional firmware/DMI information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In theory the dailinks are created based on the number of endpoints reported in ACPI match tables, so it should harmless to add a new dailink: RT713 VA would not use it since it has only 2 endpoints. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add a filter to skip the RT172 VB configuration if a SmartMic Function is not found in the SDCA descriptors. If the ACPI information is incorrect this can only be quirked further with DMI information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use the new machine_check() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
All existing SDCA codec drivers implement their own custom SDCA interrupt processing. The registers are standard, but actions resulting from an interrupt are specific, which really calls for a change in partitioning with common parts implemented once. In addition, SDCA functions may be supported by separate drivers, but the interrupt processing is handled at the SoundWire peripheral level. This means that SDCA function drivers need a new interface to register an interrupt source with the SDCA device interrupt handler, with the ability to provide a context to be used by a callback invoked in a standard hw-agnostic interrupt handler. Note: these helpers need to be in a dedicated module to avoid circular dependencies. The SoundWire bus code now relies on snd-soc-sdca, so we cannot call SoundWire bus functions from snd-soc-sdca. This is really annoying and maybe we have to split the SoundWire bus code from the slave probe code. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Rather than open-code, use the generic SDCA interrupt handler after registering the interrupt sources and callbacks. FIXME: Need to figure out what actions need to be taken for INT0 and INT8. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Rather than open-code, use the generic SDCA interrupt handler after registering the interrupt sources and callbacks. FIXME: Need to figure out what actions need to be taken for INT0 and INT8. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
sound/soc/sdw_utils/soc_sdw_utils.c
Outdated
| if (!codec_dai) { | ||
| dev_err(dev, "codec dai %s not registered yet\n", | ||
| dlc.dai_name); | ||
| return -EAGAIN; |
There was a problem hiding this comment.
can this happen?
I am wondering if the deferred-probe mechanism doesn't already guarantee that if this function is reached than all resources were registered.
Probably a question for @charleskeepax and @shumingfan as well.
There was a problem hiding this comment.
I think these can indeed be missing here. Until snd_soc_register_card (which we are before since we are building the DAI links) is called there is no connection between the drivers, but once that is called ASoC will check if all the components are probed.
There was a problem hiding this comment.
We don't need the connection. What we need is devm_snd_soc_register_component() being called by codec drivers. The thing is machine driver could probe before codec driver. And machine driver will probe again after codec driver is probed. Not sure what should I do here in this scenario. Maybe just create the dai link as usual and let ASoC do machine probe again?
There was a problem hiding this comment.
Hmm... yes one needs to be careful here, because the codec probe could be totally asynchronous to us here. So it would be possible that this code here runs, then the codec probes, then we call devm_snd_soc_register_component. So I don't think we could rely on register component to probe defer.
I suspect we need to handle probe defers here, should we not be returning -EPROBE_DEFER rather than -EAGAIN?
There was a problem hiding this comment.
Hmm... yes one needs to be careful here, because the codec probe could be totally asynchronous to us here. So it would be possible that this code here runs, then the codec probes, then we call devm_snd_soc_register_component. So I don't think we could rely on register component to probe defer.
I suspect we need to handle probe defers here, should we not be returning -EPROBE_DEFER rather than -EAGAIN?
Yes, it is for this purpose. Thanks.
| slave->sdca_data.sdca_func[k].name); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be better to use the type here? It gets patched up I think in the case of the older versions of the standard, so it seems a bit nicer than string matching.
There was a problem hiding this comment.
Yes, my first version is to use type. However, we have different SDCA function types map to the same DAI type.
enum sdca_function_type {
SDCA_FUNCTION_TYPE_SMART_AMP = 0x01, /* Amplifier with protection features */
SDCA_FUNCTION_TYPE_SIMPLE_AMP = 0x02, /* subset of SmartAmp */
SDCA_FUNCTION_TYPE_SMART_MIC = 0x03, /* Smart microphone with acoustic triggers */
SDCA_FUNCTION_TYPE_SIMPLE_MIC = 0x04, /* subset of SmartMic */
SDCA_FUNCTION_TYPE_SPEAKER_MIC = 0x05, /* Combination of SmartMic and SmartAmp */
SDCA_FUNCTION_TYPE_UAJ = 0x06, /* 3.5mm Universal Audio jack */
SDCA_FUNCTION_TYPE_RJ = 0x07, /* Retaskable jack */
SDCA_FUNCTION_TYPE_SIMPLE_JACK = 0x08, /* Subset of UAJ */
SDCA_FUNCTION_TYPE_HID = 0x0A, /* Human Interface Device, for e.g. buttons */
SDCA_FUNCTION_TYPE_IMP_DEF = 0x1F, /* Implementation-defined function */
};For example, SOC_SDW_DAI_TYPE_AMP could be SDCA_FUNCTION_TYPE_SMART_AMP or SDCA_FUNCTION_TYPE_SIMPLE_AMP. And so do SOC_SDW_DAI_TYPE_JACK and SOC_SDW_DAI_TYPE_MIC
There was a problem hiding this comment.
You could do something like add a function:
int asoc_sdw_get_dai_type(u32 type) {
switch (type) {
case SDCA_FUNCTION_TYPE_SMART_AMP:
case SDCA_FUNCTION_TYPE_SIMPLE_AMP:
return SOC_SDW_DAI_TYPE_JACK;
case SDCA_FUNCTION_TYPE_SMART_MIC:
case SDCA_FUNCTION_TYPE_SIMPLE_MIC:
case SDCA_FUNCTION_TYPE_SPEAKER_MIC:
return SOC_SDW_DAI_TYPE_MIC;
case SDCA_FUNCTION_TYPE_UAJ:
case SDCA_FUNCTION_TYPE_RJ:
case SDCA_FUNCTION_TYPE_SIMPLE_JACK:
return SOC_SDW_DAI_TYPE_JACK;
default:
return -EINVAL;
}
}
Then update your loop to something like:
for (k = 0; k < slave->sdca_data.num_functions; k++) {
int dai_type = asoc_sdw_get_dai_type(slave->sdca_data.sdca_func[k].type);
if (dai_type == dai_info->dai_type) {
dev_dbg(&slave->dev, "Function %s found\n",
slave->sdca_data.sdca_func[k].name);
break;
}
}
There was a problem hiding this comment.
Yes, it looks better. Thanks @charleskeepax
There was a problem hiding this comment.
I tested it by blacklist the codec driver first and then modprobe it after the machine driver probed. And it works.
bardliao
force-pushed
the
skip-endpoint
branch
from
c079c50
to
9e25018
Compare
A codec endpoint may not be used. We could check the present SDCA functions to know if the endpoint is used or not. Skip the endpoint which is not used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
bardliao
force-pushed
the
skip-endpoint
branch
from
9e25018
to
8cdf02c
Compare
bardliao
changed the title
|
|
||
| slave = dev_to_sdw_dev(codec_dai->dev); | ||
| if (!slave) | ||
| return -EINVAL; |
There was a problem hiding this comment.
Turns out there is one slight complication I had overlooked here as well, the codec device might not necessarily be the soundwire device. For example cs42l43 is an MFD so it is the parent of the codec device that is the soundwire slave, this situation would likely exist for the SDCA auxiliary device things as well.
There was a problem hiding this comment.
Thanks @charleskeepax for pointing this out. Let's figure out how to solve it. Maybe check up to 2 level parents?
There was a problem hiding this comment.
I have been playing around with using bus_find_device to search for the soundwire device instead I think that might be workable at the expense of perhaps being a little expensive.
|
In a wider context we would also need to think about how we handle this on the topology side. For example, I can setup my device without the mic link in ACPI, but then I will get a "can't connect DAI alh-copier.Capture-SmartMic.0 stream Capture-SmartMic", since that link is still in the topology. I guess would be fine for cases like the DMICs being on the host, since we will load a different topology in that case anyway (with the -Xch on the end). For the case where we have some customers removing SDCA functions based on BIOS settings that wouldn't work as the loaded topology would be the same in both cases. |
Yes, and I need more time to think about it. |
|
Close it for now. Let's focus on #5199 first. |
A codec endpoint may not be used. We could check the present SDCA
functions to know if the endpoint is used or not. Skip the endpoint
which is not used.
The first 12 commits are from #5128. Please review the last commit. That is a proposal to skip the unused endpoints.