Re: [PATCHv2 2/2] ASoC: OMAP-McBSP: ASoC interface for McBSP sidetone

[Peter Ujfalusi <peter.ujfalusi@nokia.com>]

Hello,

Looks good, but I have one comment, you can consider if you like it...

...

> +static int omap_mcbsp2_st_set_mode(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	u8 value = ucontrol->value.integer.value[0];
> +
> +	if (value == omap_st_is_enabled(1))
> +		return 0;
> +
> +	if (value)
> +		omap_st_enable(1);
> +	else
> +		omap_st_disable(1);
> +
> +	return 1;
> +}
> +
> +static int omap_mcbsp2_st_get_mode(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	ucontrol->value.integer.value[0] = omap_st_is_enabled(1);
> +	return 0;
> +}
> +
> +static int omap_mcbsp3_st_set_mode(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	u8 value = ucontrol->value.integer.value[0];
> +
> +	if (value == omap_st_is_enabled(2))
> +		return 0;
> +
> +	if (value)
> +		omap_st_enable(2);
> +	else
> +		omap_st_disable(2);
> +
> +	return 1;
> +}
> +
> +static int omap_mcbsp3_st_get_mode(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +
> +	ucontrol->value.integer.value[0] = omap_st_is_enabled(2);
> +	return 0;
> +}

Instead of having these two set of function, I would have only one:

static int omap_mcbsp_st_put_mode(struct snd_kcontrol *kcontrol,
					struct snd_ctl_elem_value *ucontrol)
{
	struct soc_mixer_control *mc =
			(struct soc_mixer_control *)kcontrol->private_value;
	u8 value = ucontrol->value.integer.value[0];

	if (value == omap_st_is_enabled(mc->reg))
		return 0;

	if (value)
		omap_st_enable(mc->reg);
	else
		omap_st_disable(mc->reg);

	return 1;
}

static int omap_mcbsp_st_get_mode(struct snd_kcontrol *kcontrol,
					struct snd_ctl_elem_value *ucontrol)
{
	struct soc_mixer_control *mc =
			(struct soc_mixer_control *)kcontrol->private_value;

	ucontrol->value.integer.value[0] = omap_st_is_enabled(mc->reg);

	return 0;
}

Than

> +static const struct snd_kcontrol_new omap_mcbsp2_st_controls[] = {
> +	SOC_SINGLE_EXT("McBSP2 Sidetone Switch", 0, 0, 1, 0,
> +			omap_mcbsp2_st_get_mode, omap_mcbsp2_st_set_mode),

	SOC_SINGLE_EXT("McBSP2 Sidetone Switch", 1, 0, 1, 0,
		omap_mcbsp_st_get_mode, omap_mcbsp_st_put_mode),

> +	OMAP_MCBSP_SOC_SINGLE_S16_EXT("McBSP2 Sidetone Channel 0 Volume",
> +				      -32768, 32767,
> +				      omap_mcbsp2_get_st_ch0_volume,
> +				      omap_mcbsp2_set_st_ch0_volume),
> +	OMAP_MCBSP_SOC_SINGLE_S16_EXT("McBSP2 Sidetone Channel 1 Volume",
> +				      -32768, 32767,
> +				      omap_mcbsp2_get_st_ch1_volume,
> +				      omap_mcbsp2_set_st_ch1_volume),
> +};
> +
> +static const struct snd_kcontrol_new omap_mcbsp3_st_controls[] = {
> +	SOC_SINGLE_EXT("McBSP3 Sidetone Switch", 0, 0, 1, 0,
> +			omap_mcbsp3_st_get_mode, omap_mcbsp3_st_set_mode),

	SOC_SINGLE_EXT("McBSP3 Sidetone Switch", 2, 0, 1, 0,
		omap_mcbsp_st_get_mode, omap_mcbsp_st_put_mode),

> +	OMAP_MCBSP_SOC_SINGLE_S16_EXT("McBSP3 Sidetone Channel 0 Volume",
> +				      -32768, 32767,
> +				      omap_mcbsp3_get_st_ch0_volume,
> +				      omap_mcbsp3_set_st_ch0_volume),
> +	OMAP_MCBSP_SOC_SINGLE_S16_EXT("McBSP3 Sidetone Channel 1 Volume",
> +				      -32768, 32767,
> +				      omap_mcbsp3_get_st_ch1_volume,
> +				      omap_mcbsp3_set_st_ch1_volume),
> +};
> +
> +int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id)
> +{
> +	if (!cpu_is_omap34xx())
> +		return -ENODEV;
> +
> +	switch (mcbsp_id) {
> +	case 2: /* McBSP 2 */
> +		return snd_soc_add_controls(codec, omap_mcbsp2_st_controls,
> +					ARRAY_SIZE(omap_mcbsp2_st_controls));
> +	case 3: /* McBSP 3 */
> +		return snd_soc_add_controls(codec, omap_mcbsp3_st_controls,
> +					ARRAY_SIZE(omap_mcbsp3_st_controls));
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(omap_mcbsp_st_add_controls);
> +
>  static int __init snd_omap_mcbsp_init(void)
>  {
>  	return snd_soc_register_dais(omap_mcbsp_dai,
> diff --git a/sound/soc/omap/omap-mcbsp.h b/sound/soc/omap/omap-mcbsp.h
> index 1968d03..6c363e5 100644
> --- a/sound/soc/omap/omap-mcbsp.h
> +++ b/sound/soc/omap/omap-mcbsp.h
> @@ -57,4 +57,6 @@ enum omap_mcbsp_div {
> 
>  extern struct snd_soc_dai omap_mcbsp_dai[NUM_LINKS];
> 
> +int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id);
> +
>  #endif

-- 
Péter