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

[Arnaud Pouliquen <arnaud.pouliquen@st.com>]

On 04/18/2015 03:09 PM, Mark Brown wrote:
> On Tue, Apr 14, 2015 at 03:35:31PM +0200, Arnaud Pouliquen wrote:
>
>> +struct codec_regfield {
>> +	struct regmap_field  *field;
>> +	const struct reg_field regfield;
>> +};
> The fact that you're defining this structure should be settinng off
> alarm bells.  You shouldn't be storing the per device dynamically
> allocated regmap_feild in global static data, you should be storing it
> in the driver data for your device.
Not clear for me.
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)},
...
}

>
>> +/* specify ops depends on codec version */
>> +struct sti_codec_ops {
>> +	int (*dai_dac_probe)(struct snd_soc_dai *dai);
>> +	int (*mute)(struct snd_soc_dai *dai, int mute);
>> +	int (*startup)(struct snd_soc_dai *);
>> +	void (*shutdown)(struct snd_soc_dai *);
>> +};
> Don't define an abstraction layer within your driver wrapping the core
> abstraction layer, this is just making things harder to follow.  Just
> register different ops with the core if you need to.
>
>> +	/* init DAC configuration */
>> +	if (data->chipid == CHIPID_STIH407) {
>> +		/* init configuration */
>> +		ret = regmap_field_write(
>> +			dac->regfield[STIH407_DAC_STANDBY].field, 1);
>> +		ret |= regmap_field_write(
>> +			dac->regfield[STIH407_DAC_STANDBY_ANA].field, 1);
>> +		ret |= regmap_field_write(
>> +			dac->regfield[STIH407_DAC_SOFTMUTE].field, 1);
> Don't or multiple return values together, check and return errors
> properly.  That way the error value isn't corrupted if you get different
> errors.
>
>> +	} else if (data->chipid == CHIPID_STIH416) {
> Use switch statements to select among multiple values.
>
>> +	dac->rst = devm_reset_control_get(codec->dev, "dac_rst");
>> +	if (IS_ERR(dac->rst)) {
>> +		dev_err(dai->codec->dev,
>> +			"%s: ERROR: DAC reset not declared in DT (%d)!\n",
>> +			__func__, (int)dac->rst);
> That error message might be wrong, it could be some other error.
>
>> +		return -EFAULT;
> Don't ignore the error that was returned, use PTR_ERR() - this is broken
> for probe deferral.
>
>> +int stih407_sas_dac_startup(struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +	struct sti_sas_codec_data *drvdata = dev_get_drvdata(codec->dev);
>> +	struct sti_dac_audio *dac = &drvdata->dac;
>> +	int ret;
>> +
>> +	/* mute */
>> +	ret = regmap_field_write(
>> +		dac->regfield[STIH407_DAC_SOFTMUTE].field, 1);
>> +
>> +	/* Enable analog */
>> +	ret |= regmap_field_write(
>> +		dac->regfield[STIH407_DAC_STANDBY_ANA].field, 0);
>> +
>> +	/* Disable standby */
>> +	ret |= regmap_field_write(
>> +		dac->regfield[STIH407_DAC_STANDBY].field, 0);
> This looks like at least standby and analogue should be moved to DAPM,
> I'm not clear why the mute is being controlled here...
>
>> +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?
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?
>> +static int sti_sas_codec_suspend(struct snd_soc_codec *codec)
>> +{
>> +	/* Nothing done but need to be declared for PM management*/
>> +	return 0;
>> +}
> Remove empty functions, the comment is not accurate.
>
>> +	/* Allocate device structure */
>> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct sti_sas_codec_data),
>> +			       GFP_KERNEL);
>> +
>> +	if (!drvdata) {
>> +		dev_err(&pdev->dev, "Failed to allocate device structure");
> The memory allocators are already very noisy if they fail.