Re: [PATCH 7/7] ASoc: Codec: add sti platform codec

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2121 bytes --]

On Mon, Apr 20, 2015 at 11:13:27AM +0200, Arnaud Pouliquen wrote:
> On 04/18/2015 03:09 PM, Mark Brown wrote:

> Register definitions are already part of the driver data structure
> (stih407_data  or stih416_data) .
> Your point is that i should delete this structure?
> And then use  separate regmap_field  tables (dynamically allocated) for for
> register map
> and declare registers struct like this
> static struct reg_field sti_sas_dac_stih416[STIH416_DAC_MAX_RF] = {
>     /*STIH416_DAC_MODE*/
>     {REG_FIELD(STIH416_AUDIO_DAC_CTRL, 1, 2)},
> ...
> }

Yes, basically.

> >>+static int sti_sas_hw_params(struct snd_pcm_substream *substream,
> >>+			     struct snd_pcm_hw_params *params,
> >>+			     struct snd_soc_dai *dai)
> >>+{
> >>+	struct snd_soc_codec *codec = dai->codec;
> >>+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >>+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >>+	int ret, div;
> >>+
> >>+	div = sti_sas_dai_clk_div[dai->id];
> >>+	ret = snd_soc_dai_set_clkdiv(cpu_dai, SND_SOC_CLOCK_OUT, div);
> >>+	if (ret)

> >set_clkdiv() is an external API, you shouldn't be calling it within the
> >driver - probably you should just not implement it and inline the
> >functionality here.

> how to do it properly?

Like I say probably you should just inline the functionality here.  It's
not like anything else can usefully call set_clkdiv() if the driver is
just going to do it itself anyawy.

> My point is that cpu codecs have a constraint on MCLK_FS division.
> So i need to provide it to the CPU_DAI that generates MCLK.
> I checked simple driver
> priv->mclk_fs exists but is only used to set codec dai not CPU dai...
> should i add call in simple card?

Possibly.  In general it is better to add missing functionality to
generic code rather than bodge around the generic code.  However in this
case I expect that any user of the driver (not just simple card) may
need the same configuration.  If that is the case then the driver should
just always do the configuration without needing the machine driver to
do anything so this just doesn't need to be visible outside the driver.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]