Hi Srinivas,
On Fri, Jan 15, 2021 at 04:14:05PM +0000, Srinivas Kandagatla wrote:
On 14/01/2021 09:46, Stephan Gerhold wrote:
[...] The problem was introduced in commit 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver"). The mistake made there is that lpass.h now contains
#include <dt-bindings/sound/sc7180-lpass.h>
This thing was obviously missed in the review and testing, and its really bad idea to have multiple header files based on different SOC for the same driver. We are planning to add some basic tests in ciloop to catch such issues!
IMO, Its better to sort it out now, before this gets complicated!
Am thinking of making a common header file ("lpass,h") and include that in the existing SoC specific header for compatibility reasons only.
In future we should just keep adding new DAI index in incremental fashion to common header file rather than creating SoC specific one!
[...]
Srinivas mentioned a potential different fix here: https://lore.kernel.org/alsa-devel/4b34bd4f-e7bc-84f9-5e8a-b2348c17b7aa@lina...
Instead of this patch, we could change the dt-bindings for LPASS, so that all SoCs use consistent DAI IDs.
TBH, Am inclined to do the right thing and make DAI ID's consistent! Like we do at the dsp afe interfaces.
This will also bring in the need to add .of_xlate_dai_name callback along with fixing sc7180_lpass_cpu_dai_driver array index.
Without this the code will end up very confusing!
I agree that this would be cleaner, as I mentioned here:
In general, I think this might be cleaner, especially in case more special DAIs are added in the future. However, when I made this patch (before Srinivas mentioned it) I tried to avoid changing the dt-bindings because:
- Changing dt-bindings after they are added is generally discouraged.
but more importantly:
- lpass-ipq806x.c does not seem to have PRIMARY, SECONDARY, ... but something completely different. I know nothing about that platform so I don't know how to handle it.
... but it's still not clear to me how to handle ipq806x. The DAIs it has don't really look similar to what MSM8916 and SC7180 have.
Right now it declares just a single DAI, but multiple "ports":
enum lpaif_i2s_ports { IPQ806X_LPAIF_I2S_PORT_CODEC_SPK, IPQ806X_LPAIF_I2S_PORT_CODEC_MIC, IPQ806X_LPAIF_I2S_PORT_SEC_SPK, IPQ806X_LPAIF_I2S_PORT_SEC_MIC, IPQ806X_LPAIF_I2S_PORT_MI2S, };
static struct snd_soc_dai_driver ipq806x_lpass_cpu_dai_driver = { .id = IPQ806X_LPAIF_I2S_PORT_MI2S, /* ... */ };
I suppose we could just declare this as MI2S_PRIMARY but not sure if that is accurate. Do you have more information about this platform?
Thanks, Stephan