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

[ilkka.koskinen at nokia.com]

>From: Ujfalusi Peter (Nokia-D/Tampere) 
>Sent: 18 February, 2010 10:15
>
>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

Makes sense - I'll change it.

Cheers, Ilkka


>> +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
>