Date: Thu, 5 Nov 2015 16:13:11 +0000
From: Mark Brown <broonie@kernel.org>
To: Shunqian Zheng <zhengsq@rock-chips.com>
Cc: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org, heiko@sntech.de,
        lgirdwood@gmail.com, perex@perex.cz, tiwai@suse.com,
        benzh@chromium.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org
Message-ID: <20151105161311.GR18409@sirena.org.uk>
References: <1446717194-8572-1-git-send-email-zhengsq@rock-chips.com>
 <1446717194-8572-2-git-send-email-zhengsq@rock-chips.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="OiITrshLgfui8fEl"
Content-Disposition: inline
In-Reply-To: <1446717194-8572-2-git-send-email-zhengsq@rock-chips.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Subject: Re: [PATCH v2 1/2] ASoC: codec: Inno codec driver for RK3036 SoC
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2986
Lines: 82


--OiITrshLgfui8fEl
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Nov 05, 2015 at 05:53:13PM +0800, Shunqian Zheng wrote:

This is basically all good, a few very minor comments below but nothing
that should take any time to fix.

> +static const char *rk3036_codec_antipop_text[] = {"none", "work"};
> +static const unsigned int rk3036_codec_antipop_values[] = {0x1, 0x2};
> +
> +static SOC_VALUE_ENUM_DOUBLE_DECL(rk3036_codec_antipop_enum, INNO_R09,
> +		INNO_R09_HPL_ANITPOP_SHIFT, INNO_R09_HPR_ANITPOP_SHIFT, 0x3,
> +		rk3036_codec_antipop_text, rk3036_codec_antipop_values);

This looks like a simple boolean control rather than an enum - it looks
like it's just turning antipop on and off.

> +	SOC_DOUBLE_R_RANGE_TLV("Headphone Volume", INNO_R07, INNO_R08,
> +		INNO_HP_GAIN_SHIFT, INNO_HP_GAIN_N39DB,
> +		INNO_HP_GAIN_0DB, 0, rk3036_codec_hp_tlv),
> +	SOC_DOUBLE("Zero Cross Detect", INNO_R06, INNO_R06_VOUTL_CZ_SHIFT,
> +		INNO_R06_VOUTR_CZ_SHIFT, 1, 0),

This should be "Zero Cross Switch".

> +	SOC_DOUBLE("HP Mute", INNO_R09, INNO_R09_HPL_MUTE_SHIFT,
> +		INNO_R09_HPR_MUTE_SHIFT, 1, 1),

This should be "Headphone Switch".

> +static int rk3036_codec_add_widgets(struct snd_soc_codec *codec)
> +{
> +	struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec);
> +
> +	snd_soc_add_codec_controls(codec, rk3036_codec_dapm_controls,
> +		ARRAY_SIZE(rk3036_codec_dapm_controls));
> +
> +	snd_soc_dapm_new_controls(dapm, rk3036_codec_dapm_widgets,
> +		ARRAY_SIZE(rk3036_codec_dapm_widgets));
> +
> +	snd_soc_dapm_add_routes(dapm, rk3036_codec_dapm_routes,
> +		ARRAY_SIZE(rk3036_codec_dapm_routes));

Just point at the tables from the driver structure and let the core do
the initialisation for you.

> +static int rk3036_codec_set_bias_level(struct snd_soc_codec *codec,
> +				       enum snd_soc_bias_level level)
> +{
> +	switch (level) {
> +	case SND_SOC_BIAS_STANDBY:
> +		/* set a big current for capacitor charging. */
> +		snd_soc_write(codec, INNO_R10, INNO_R10_MAX_CUR);
> +		/* start precharge */
> +		snd_soc_write(codec, INNO_R06, INNO_R06_DAC_PRECHARGE);

Do we not need to wait for this to ramp?

--OiITrshLgfui8fEl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWO4AWAAoJECTWi3JdVIfQn1UH/jpOV84jha0/A6lC536Tg7a1
vt3A+PaDPp1vXtY0HOjdtqPjmvE0wLIG8DtaRRxOf/RPrT4yDKjKB1T6tHDyzj0Y
HM7YSEJT1P44GDKEK3bQeBDwcVP2qoCxOxz1DGQeIR+oSV1uHBtHIwCzBWlzJh7/
sTDLOXRU9Zvg25o4za1BFPjC5KzRPn9ewwpWeZYLrieRqZCzc6OIYCQSbFos8Ge9
NU21HIkg8QUhjXRNPkDyPzPeqhH3VhTtla8DkU8HtTJtxmPjvV20AUuUDazdDZxh
RYstNTlFJSXYStHb8dnqL33oCsjy/q1koCeZMaAto+E6dGYuHsAaCTi+XhJiop8=
=zfS4
-----END PGP SIGNATURE-----

--OiITrshLgfui8fEl--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/