Re: [PATCH v2 2/2] ASoC: wm8985: add support for WM8758

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

On Mon, May 23, 2016 at 09:34:26AM +0200, Petr Kulhavy wrote:
> On 20 May 2016 at 18:11, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Tue, May 17, 2016 at 02:48:54PM +0200, Petr Kulhavy wrote:
> >> The WM8758 chip is almost identical to WM8985 with the difference that it
> >> doesn't feature the AUX input. This patch adds the WM8758 support into the
> >> WM8985 driver.
> >>
> >> The chip selection is done by the I2C name. The SPI probe supports only
> >> the WM8985.
> >>
> >> Signed-off-by: Petr Kulhavy <petr@barix.com>
> >> ---
> >> +
> >> +static int wm8985_add_widgets(struct snd_soc_codec *codec)
> >> +{
> >> +     struct wm8985_priv *wm8985 = snd_soc_codec_get_drvdata(codec);
> >> +     struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec);
> >> +
> >> +     snd_soc_dapm_new_controls(dapm, wm8985_common_dapm_widgets,
> >> +             ARRAY_SIZE(wm8985_common_dapm_widgets));
> >> +     snd_soc_dapm_add_routes(dapm, wm8985_common_dapm_routes,
> >> +             ARRAY_SIZE(wm8985_common_dapm_routes));
> >> +
> >> +     switch (wm8985->dev_type) {
> >> +     case WM8758:
> >> +             snd_soc_add_codec_controls(codec, wm8985_alc_snd_controls,
> >> +                     ARRAY_SIZE(wm8985_alc_snd_controls));
> >> +             snd_soc_add_codec_controls(codec, wm8985_adc_snd_controls,
> >> +                     ARRAY_SIZE(wm8985_adc_snd_controls));
> >> +             snd_soc_add_codec_controls(codec, wm8985_dac_snd_controls,
> >> +                     ARRAY_SIZE(wm8985_dac_snd_controls));
> >> +             snd_soc_add_codec_controls(codec, wm8985_eq_snd_controls,
> >> +                     ARRAY_SIZE(wm8985_eq_snd_controls));
> >> +             snd_soc_add_codec_controls(codec, wm8985_3d_snd_controls,
> >> +                     ARRAY_SIZE(wm8985_3d_snd_controls));
> >
> > Why not just put all these in a common controls array? They seem
> > to all be used on 8985 as well.
> 
> In general I agree with you. But doesn't the order in which
> snd_soc_add_codec_controls() is called affect the order in which the
> controls appear in the mixer?
> I have split the controls in order to preserve the original order of
> the controls.

Personally I would preference keeping the code clean rather than
preserving the order of the controls. I don't think the absolute
numerical position of the controls is normally considered an
important ABI to preserve, aside from anything else it could be
affected by things like probe order between cards.

> 
> >>  err_reg_enable:
> >> @@ -1047,13 +1164,6 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8985 = {
> >>       .probe = wm8985_probe,
> >>       .set_bias_level = wm8985_set_bias_level,
> >>       .suspend_bias_off = true,
> >> -
> >> -     .controls = wm8985_snd_controls,
> >> -     .num_controls = ARRAY_SIZE(wm8985_snd_controls),
> >> -     .dapm_widgets = wm8985_dapm_widgets,
> >> -     .num_dapm_widgets = ARRAY_SIZE(wm8985_dapm_widgets),
> >> -     .dapm_routes = wm8985_dapm_routes,
> >> -     .num_dapm_routes = ARRAY_SIZE(wm8985_dapm_routes),
> >
> > I think you should still be able to use these for the common
> > functionality.
> 
> All these are set by wm8985_add_widgets() now.
> Or maybe I didn't quite get what you mean?

My point was just that you can still use these helpers for the
common stuff and then wm8985_add_widgets needs to only add the
codec specific stuff.

Thanks,
Charles