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@xxxxxxxxxx/
> >
> > 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
