Re: [PATCH 2/7] ASoC: soc-core: add snd_soc_runtime_get_dai_fmt()

From: Mark Brown <broonie@kernel.org>
To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>
Subject: Re: [PATCH 2/7] ASoC: soc-core: add snd_soc_runtime_get_dai_fmt()
Date: Fri, 23 Apr 2021 19:35:03 +0100	[thread overview]
Message-ID: <20210423183503.GK5507@sirena.org.uk> (raw)
In-Reply-To: <87y2dbgk47.wl-kuninori.morimoto.gx@renesas.com>

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On Thu, Apr 22, 2021 at 10:53:44AM +0900, Kuninori Morimoto wrote:

> +/* Describes the possible PCM format */
> +#define SND_SOC_POSSIBLE_DAIFMT_FORMAT_SHIFT	0
> +#define SND_SOC_POSSIBLE_DAIFMT_FORMAT_MASK	(0xFFFF << SND_SOC_POSSIBLE_DAIFMT_FORMAT_SHIFT)
> +#define SND_SOC_POSSIBLE_DAIFMT_I2S		(1 << SND_SOC_DAI_FORMAT_I2S)
> +#define SND_SOC_POSSIBLE_DAIFMT_RIGHT_J		(1 << SND_SOC_DAI_FORMAT_RIGHT_J)
> +#define SND_SOC_POSSIBLE_DAIFMT_LEFT_J		(1 << SND_SOC_DAI_FORMAT_LEFT_J)
> +#define SND_SOC_POSSIBLE_DAIFMT_DSP_A		(1 << SND_SOC_DAI_FORMAT_DSP_A)
> +#define SND_SOC_POSSIBLE_DAIFMT_DSP_B		(1 << SND_SOC_DAI_FORMAT_DSP_B)
> +#define SND_SOC_POSSIBLE_DAIFMT_AC97		(1 << SND_SOC_DAI_FORMAT_AC97)
> +#define SND_SOC_POSSIBLE_DAIFMT_PDM		(1 << SND_SOC_DAI_FORMAT_PDM)

I'm not 100% sure I get why we have the separate _POSSIBLE_ macros here?
One thing we'll have to take account of is that there's some DAIs that
have some restrictions on what options they can combine - for example
only doing I2S with one format of clock but allowing clock inversion in
DSP mode.  It might be safer (if tedious for driver authors without some
help...) to just have arrays of fully specified daifmt values.

>  /* Digital Audio interface formatting */
> +u64 snd_soc_dai_get_fmt(struct snd_soc_dai *dai);

Like I said on the cover letter I think we need to be able to specify at
least two levels of preference here.  How about

	int snd_soc_dai_get_fmt(struct snd_soc_dai *dai,
				u64 *preferred, u64 *supported);

or something?  Just thinking off the top of my head, that's a bit ugly
and doesn't scale to multiple levels so I don't know if I'm *super*
happy with that interface.  But we might be better off using just arrays
of daifmt values like I say, if we do that perhaps just one array but
sorting it might do?

> +	/* use original dai_fmt if sound card specify */
> +	if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK))
> +		mask |= SND_SOC_DAIFMT_FORMAT_MASK;
> +	if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_CLOCK_MASK))
> +		mask |= SND_SOC_DAIFMT_CLOCK_MASK;
> +	if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_INV_MASK))
> +		mask |= SND_SOC_DAIFMT_INV_MASK;
> +	if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_MASTER_MASK))
> +		mask |= SND_SOC_DAIFMT_MASTER_MASK;
> +
> +	dai_link->dai_fmt =	dai_link->dai_fmt | (dai_fmt & mask);

If the sound card specifies something why not just use it verbatim
instead of trying to merge?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]