Re: [PATCH 08/12] ASoC: wcd9335: add basic controls

[Srinivas Kandagatla]

Thanks Vinod for review comments,

On 25/07/18 13:04, Vinod wrote:
> On 23-07-18, 16:54, Srinivas Kandagatla wrote:
>
> > +static int wcd9335_set_compander(struct snd_kcontrol *kc,
> > +				 struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_soc_kcontrol_component(kc);
> > +	struct wcd9335_codec *wcd = dev_get_drvdata(component->dev);
> > +	int comp = ((struct soc_mixer_control *) kc->private_value)->shift;
> > +	int value = ucontrol->value.integer.value[0];
> > +	int sel;
> > +
> > +	wcd->comp_enabled[comp] = value;
> > +	sel = value ? WCD9335_HPH_GAIN_SRC_SEL_COMPANDER :
> > +		WCD9335_HPH_GAIN_SRC_SEL_REGISTER;
> > +
> > +	/* Any specific register configuration for compander */
> > +	switch (comp) {
> > +	case COMPANDER_1:
> > +		/* Set Gain Source Select based on compander enable/disable */
> > +		snd_soc_component_update_bits(component, WCD9335_HPH_L_EN,
> > +				      WCD9335_HPH_GAIN_SRC_SEL_MASK, sel);
> > +		break;
> > +	case COMPANDER_2:
> > +		snd_soc_component_update_bits(component, WCD9335_HPH_R_EN,
> > +				      WCD9335_HPH_GAIN_SRC_SEL_MASK, sel);
> > +		break;
> > +	case COMPANDER_3:
> > +		break;
> > +	case COMPANDER_4:
> > +		break;
> > +	case COMPANDER_5:
> > +		snd_soc_component_update_bits(component, WCD9335_SE_LO_LO3_GAIN,
> > +				      WCD9335_HPH_GAIN_SRC_SEL_MASK, sel);
> > +		break;
> > +	case COMPANDER_6:
> > +		snd_soc_component_update_bits(component, WCD9335_SE_LO_LO4_GAIN,
> > +				      WCD9335_HPH_GAIN_SRC_SEL_MASK, sel);
> > +		break;
> > +	case COMPANDER_7:
> > +		break;
> > +	case COMPANDER_8:
> > +		break;
>
> why not do:
>
>          case COMPANDER_3:
>          case COMPANDER_4:
>          case COMPANDER_7:
>          case COMPANDER_8:
>                  break;
>
> thereby clubbing all the ones not handled together..
I removed the un-handled ones in v2.

> > +static int wcd9335_rx_hph_mode_get(struct snd_kcontrol *kc,
> > +				 struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_soc_kcontrol_component(kc);
> > +	struct wcd9335_codec *wcd = dev_get_drvdata(component->dev);
> > +
> > +	ucontrol->value.integer.value[0] = wcd->hph_mode;
>
> i think this is an enum so shouldn't value.enumerated.item[0] be used?
Yep, there are one more instances like this, i fixed this in v2.
> > +
> > +	return 0;
> > +}
> > +
> > +static int wcd9335_rx_hph_mode_put(struct snd_kcontrol *kc,
> > +				 struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_soc_kcontrol_component(kc);
> > +	struct wcd9335_codec *wcd = dev_get_drvdata(component->dev);
> > +	u32 mode_val;
> > +
> > +	mode_val = ucontrol->value.enumerated.item[0];
>
> yaaay

--srini
>