Re: [PATCH 1/2] ASoC: qcom: lpass: Fix hardcoded SC7810 DAI IDs

[Stephan Gerhold <stephan@gerhold.net>]

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@linaro.org/
> > 
> > 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