[alsa-devel] [PATCH RFC 1/1] ASoC: TI WL1273 FM Radio Codec.

[Mark Brown]

On Tue, Jul 20, 2010 at 03:41:58PM +0300, Matti J. Aaltonen wrote:

> +#undef DEBUG

This shouldn't be here.

> +static int snd_wl1273_set_audio_route(struct snd_kcontrol *kcontrol,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec);
> +
> +	/* Do not allow changes while stream is running */
> +	if (codec->active)
> +		return -EPERM;
> +
> +	wl1273->mode = ucontrol->value.integer.value[0];

You're not doing any validation of the supplied value here.

> +static const struct soc_enum wl1273_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(wl1273_audio_route), wl1273_audio_route),
> +};

Don't put your enums in arrays, it just makes things harder to maintain
since you have to reference an index into an array rather than a named
value.

> +static int snd_wl1273_fm_audio_put(struct snd_kcontrol *kcontrol,

> +static const struct snd_kcontrol_new wl1273_controls[] = {
> +	SOC_ENUM_EXT("Codec Mode", wl1273_enum[0],
> +		     snd_wl1273_get_audio_route, snd_wl1273_set_audio_route),
> +	SOC_ENUM_EXT("Audio Switch", wl1273_audio_enum[0],
> +		     snd_wl1273_fm_audio_get,  snd_wl1273_fm_audio_put),
> +	SOC_SINGLE_EXT("Volume", 0, 0, WL1273_MAX_VOLUME, 0,
> +		       snd_wl1273_fm_volume_get, snd_wl1273_fm_volume_put),
> +};

My major comment about this driver is that it'd probably make sense to
redo it on top of Liam's multi-component branch, though it shouldn't be
a pressing concern for merge.

> +	switch (wl1273->mode) {
> +	case WL1273_MODE_BT:
> +		pcm->info_flags &= ~SNDRV_PCM_INFO_HALF_DUPLEX;
> +		break;

Wouldn't it be nicer to do this stuff with the ALSA constraint APIs
rather than futzing with the constant data?

> +static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai,
> +			      unsigned int fmt)
> +{
> +	return 0;
> +}

Remove unused functions.

> +enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec)
> +{
> +	struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec);
> +	return wl1273->mode;
> +}
> +EXPORT_SYMBOL_GPL(wl1273_get_codec_mode);

Why is this being exported?

> +static int wl1273_soc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	return 0;
> +}
> +
> +static int wl1273_soc_resume(struct platform_device *pdev)
> +{
> +	return 0;
> +}

Remove unused functions.