Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 20 Dec 2019 12:11:52 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Jeff Chang <richtek.jeff.chang@gmail.com>
Cc:     lgirdwood@gmail.com, perex@perex.cz, tiwai@suse.com,
        matthias.bgg@gmail.com, alsa-devel@alsa-project.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        jeff_chang@richtek.com
Subject: Re: [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver
Message-ID: <20191220121152.GC4790@sirena.org.uk>
References: <1576836934-5370-1-git-send-email-richtek.jeff.chang@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="Y5rl02BVI9TCfPar"
Content-Disposition: inline
In-Reply-To: <1576836934-5370-1-git-send-email-richtek.jeff.chang@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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

On Fri, Dec 20, 2019 at 06:15:34PM +0800, Jeff Chang wrote:

> +++ b/sound/soc/codecs/mt6660.c
> @@ -0,0 +1,653 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +	{ MT6660_REG_DEVID, 2},
> +	{ MT6660_REG_TDM_CFG3, 2},
> +	{ MT6660_REG_HCLIP_CTRL, 2},
> +	{ MT6660_REG_DA_GAIN, 2},

Missing space before the } (the same thing happens in some of the
other tables).

> +static int mt6660_component_get_volsw(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct mt6660_chip *chip = (struct mt6660_chip *)
> +		snd_soc_component_get_drvdata(component);
> +	int ret = -EINVAL;
> +
> +	if (!strcmp(kcontrol->id.name, "Chip_Rev")) {

Why would this be used on a different control?

> +	SOC_SINGLE_EXT("BoostMode", MT6660_REG_BST_CTRL, 0, 3, 0,
> +		       snd_soc_get_volsw, snd_soc_put_volsw),

Boost Mode.  You've also got a lot of these controls that are _EXT but
you then just use standard operations so it's not clear why you're using
_EXT.

> +	SOC_SINGLE_EXT("audio input selection", MT6660_REG_DATAO_SEL, 6, 3, 0,
> +		       snd_soc_get_volsw, snd_soc_put_volsw),

Audio Input Selection, but this looks like it should be a DAPM control
if it's controlling audio routing.  A simple numerical setting
definitely doesn't seem like the right thing.

> +	SOC_SINGLE_EXT("AUD LOOP BACK Switch", MT6660_REG_PATH_BYPASS, 4, 1, 0,
> +		       snd_soc_get_volsw, snd_soc_put_volsw),

This sounds like it should be a DAPM thing too.

> +static int mt6660_component_probe(struct snd_soc_component *component)
> +{
> +	struct mt6660_chip *chip = snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	dev_info(component->dev, "%s\n", __func__);

dev_dbg() at most but probably better to remove this and the other
similar dev_info()s.

> +static inline int _mt6660_chip_id_check(struct mt6660_chip *chip)
> +{
> +	u8 id[2] = {0};
> +	int ret = 0;
> +
> +	ret = i2c_smbus_read_i2c_block_data(chip->i2c, MT6660_REG_DEVID, 2, id);
> +	if (ret < 0)
> +		return ret;
> +	ret = (id[0] << 8) + id[1];
> +	ret &= 0x0ff0;
> +	if (ret != 0x00e0 && ret != 0x01e0)
> +		return -ENODEV;

It'd be better to print an error message saying we don't recognize the
device to help people doing debugging.

> +	if (of_property_read_u32(np, "rt,init_setting_num", &val)) {
> +		dev_info(dev, "no init setting\n");
> +		chip->plat_data.init_setting_num = 0;

You should be adding a DT binding document for any new DT bindings.

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

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl38uogACgkQJNaLcl1U
h9B+kQf6AzhZSat7mqBSwJVubwAXZcJ6ApJ29GlB2ypbTdOhUOTKKR8ytA2kMg6l
Ifw88ajLzpL4jAIHwwS3NOMonMD/9KgNLfQVgMxzuzsYyRYBmj7p9lXsYICNIvoe
nSn2nPJfw5g5PlMZ+qwxyNNMTOkui6qKCFoFS77TJq0hsbTh+x6azMfn8VVRwrL+
VRXPN2C9mAdyrVPw/XPjkTyCrXw8P6brXK6qW23Y14FRQ8dXke0rhMumWytquybM
ebURG75CUEXyykGbNGHoJa4ycypd5AA/LYR86mzRbs7Mxiqlmw3co8/C44n4XKM0
QungC3A3a98p1kNSIiBM9g2Ie+cgBw==
=pOxx
-----END PGP SIGNATURE-----

--Y5rl02BVI9TCfPar--