[alsa-devel] [PATCH v2 7/9] ASoC: Codec: Add sti platform codec

[Mark Brown]

On Mon, May 18, 2015 at 02:12:54PM +0200, Arnaud Pouliquen wrote:

> +	if (data->dev_data->chipid == CHIPID_STIH407)
> +		max_rf = STIH407_DAC_MAX_RF;
> +	else
> +		max_rf = STIH416_DAC_MAX_RF;

switch statements for selecting chip versions here and elsewhere please,
that way new variants are easier to handle.

> +	dac->field = devm_kzalloc(data->dev,
> +				  sizeof(struct regmap_field *) * max_rf,
> +				  GFP_KERNEL);

devm_kcalloc().

> +static int  stih407_sas_dac_supply(struct snd_soc_dapm_widget *w,
> +				   struct snd_kcontrol *kcontrol, int event) {
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct sti_sas_data *drvdata = dev_get_drvdata(codec->dev);
> +	struct sti_dac_audio *dac = &drvdata->dac;
> +	int ret = 0;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		/* Enable analog */
> +		ret = regmap_field_write(dac->field[STIH407_DAC_STANDBY_ANA],
> +					 0);
> +		/* Disable standby */
> +		if (!ret)
> +			ret = regmap_field_write(
> +				dac->field[STIH407_DAC_STANDBY], 0);
> +		break;

These event functions all look very similar and I can't help but think
that they look awfully like the sort of register write sequences that
DAPM normally generates anyway.  Is there any great reason for not doing
these by registering multiple widgets with routes between them rather
than with custom code?

> +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 div;
> +
> +	div = sti_sas_dai_clk_div[dai->id];
> +	if (cpu_dai->driver->ops->set_clkdiv)
> +		return cpu_dai->driver->ops->set_clkdiv(cpu_dai,
> +							SND_SOC_CLOCK_OUT, div);
> +	dev_warn(codec->dev, "WARN: CPU DAI not support sysclk div");

This is worrying, we shouldn't be peering inside the CPU DAI like this.
I'd expect this to either be done autonomously by the CPU DAI or handled
in a machine driver.

> +config SND_SOC_STI_SAS
> +	tristate "codec Audio support for STI SAS codec"
> +	depends on SND_SOC_STI
> +	help
> +		Say Y if you want to include STI SAS audio codec support

This appears to duplicate the CODEC Kconfig in the CODECs directory -
I'd expect it to be there rather than in the STI directory given that
that is where the code is.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150525/942ce46f/attachment.sig>