[PATCH v2 2/3] ASoC: codecs: rk3308: restore audio and headphone support

[Mark Brown]

> Restore the vendor analog enable and disable order and settling delays
> through shared DAPM supply events for codec revisions A, B and C. Use
> virtual intermediate widgets to prevent coalesced or repeated analog
> writes without adding startup, shutdown or hw_free DAI operations.

This change is far too big and complex to be a single patch, doing a
whole bunch of different things with limited overlap.  It's all really
difficult to follow, please split it up into independent changes.  The
jack detection should be split out at a minimum, and probably there
should be multiple refactorings.

> +static int rk3308_codec_output_routes(struct rk3308_codec_priv *rk3308)
> +{
> +	static const struct snd_soc_dapm_route routes[][2] = {
> +		{
> +			{ "L_LINEOUT_EN", NULL, "DAC_L_HPMIX_SEL" },
> +			{ "R_LINEOUT_EN", NULL, "DAC_R_HPMIX_SEL" },
> +		}, {
> +			{ "L_HPOUT_EN", NULL, "DAC_L_HPMIX_SEL" },
> +			{ "R_HPOUT_EN", NULL, "DAC_R_HPMIX_SEL" },
> +		},
> +	};
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(rk3308->component);
> +	u32 mask = rk3308->dac_output == 11 ? 3 :
> +		rk3308->dac_output == 0 ? 1 : 2;

Magic numbers...  I have no idea what this is intended to mean.

> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(routes); i++) {
> +		if (!((mask ^ rk3308->output_routes_mask) & BIT(i)))
> +			continue;
> +		if (mask & BIT(i))
> +			ret = snd_soc_dapm_add_routes(dapm, routes[i], ARRAY_SIZE(routes[i]));
> +		else
> +			ret = snd_soc_dapm_del_routes(dapm, routes[i], ARRAY_SIZE(routes[i]));
> +		if (ret)
> +			return ret;
> +		rk3308->output_routes_mask ^= BIT(i);
> +	}

This is dynamically hotpluggin DAPM routes at runtime which is a really
weird way of doing things.  Why are you doing things this way?  Normally
routing changes would done through DAPM routing changes in mux and mixer
widgets.  Why are you doing things this way?  It's a lot of code, and
the userspace impacts are likely to cause confusion.

> +	if (!ret && rk3308->hpdet_jack)
> +		snd_soc_jack_report(rk3308->hpdet_jack,
> +				    output ? SND_JACK_HEADPHONE : 0, SND_JACK_HEADPHONE);

This is going to report that the user has inserted or removed something
physically based on a control write, this is obviously not something
that should happen.  The user can physically see if something has been
plugged and this is going to report something different...

> +static int rk3308_codec_hpdet_init(struct platform_device *pdev,
> +				   struct rk3308_codec_priv *rk3308)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	if (rk3308->codec_ver == ACODEC_VERSION_A ||
> +	    of_property_read_bool(np, "rockchip,no-hp-det") ||

Why do we need this property, the machine driver sets up the jack?

> +	    !of_find_property(np, "rockchip,detect-grf", NULL))

of_property_present()

> +		if (headphone)
> +			for (gain = 0; gain <= max(gain_l, gain_r); gain++) {
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_l));
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_r));
> +			}
> +		rk3308_codec_resync_adcs(rk3308);
> +	} else {
> +		/* Vendor DAC disable steps 3-19, after the external amps are off. */
> +		if (hp & (RK3308_DAC_L_HPOUT_EN | RK3308_DAC_R_HPOUT_EN))
> +			for (gain = max(gain_l, gain_r); gain >= 0; gain--) {
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_l));
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_r));
> +				usleep_range(200, 300);
> +			}

The ramp down has a delay but the ramp up doesn't, is that intentional?

> -	SND_SOC_DAPM_ADC("ADC1_WORK", "Capture", RK3308_ADC_ANA_CON05(0), 2, 0),
> -	SND_SOC_DAPM_ADC("ADC2_WORK", "Capture", RK3308_ADC_ANA_CON05(0), 6, 0),
> -	SND_SOC_DAPM_ADC("ADC3_WORK", "Capture", RK3308_ADC_ANA_CON05(1), 2, 0),
> -	SND_SOC_DAPM_ADC("ADC4_WORK", "Capture", RK3308_ADC_ANA_CON05(1), 6, 0),
> -	SND_SOC_DAPM_ADC("ADC5_WORK", "Capture", RK3308_ADC_ANA_CON05(2), 2, 0),
> -	SND_SOC_DAPM_ADC("ADC6_WORK", "Capture", RK3308_ADC_ANA_CON05(2), 6, 0),
> -	SND_SOC_DAPM_ADC("ADC7_WORK", "Capture", RK3308_ADC_ANA_CON05(3), 2, 0),
> -	SND_SOC_DAPM_ADC("ADC8_WORK", "Capture", RK3308_ADC_ANA_CON05(3), 6, 0),
> +	SND_SOC_DAPM_ADC("ADC1_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(0), 2, 0),
> +	SND_SOC_DAPM_ADC("ADC2_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(0), 6, 0),
> +	SND_SOC_DAPM_ADC("ADC3_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(1), 2, 0),
> +	SND_SOC_DAPM_ADC("ADC4_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(1), 6, 0),
> +	SND_SOC_DAPM_ADC("ADC5_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(2), 2, 0),
> +	SND_SOC_DAPM_ADC("ADC6_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(2), 6, 0),
> +	SND_SOC_DAPM_ADC("ADC7_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(3), 2, 0),
> +	SND_SOC_DAPM_ADC("ADC8_WORK", "HiFi Capture", RK3308_ADC_ANA_CON05(3), 6, 0),

Why the rename?

-- 
Mark Brown <broonie at kernel.org>