Re: [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]

On Thu, Dec 12, 2019 at 08:12:20PM +0800, Jeff Chang wrote:

> sense, which are able to be monitored via DATAO through proper
> 
> ---
> 
> [PATCH v2] :
> 	1. remove unnecessary space from commit message
> 	2. add Signed-off-by info
> 
> Signed-off-by: Jeff Chang <richtek.jeff.chang@gmail.com>
> ---

You should place the Signed-off-by before the first --- as covered by
submitting-patches.rst.  Please, slow down a bit before resending and
make sure you've checked what you're doing thoroughly.  Look at what
you're sending and how it compares to what others are sending.

> +config SND_SOC_MT6660
> +	tristate "Mediatek MT6660 Speaker Amplifier"
> +	depends on I2C
> +	select CRC32
> +	select CRYPTO_SHA256
> +	select CRYTO_RSA
> +	help

These selects of crypto stuf appear entirely unrelated to anything in
the driver?

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

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

> +static int mt6660_dbg_io_write(void *drvdata, u16 reg,
> +			       const void *val, u16 size)
> +{
> +	struct mt6660_chip *chip = (struct mt6660_chip *)drvdata;
> +	int reg_size = mt6660_get_reg_size(reg);
> +	int i = 0;
> +	unsigned int regval = 0;
> +	u8 *_val = (u8 *)val;

This is duplicating standard regmap functionality.

> +static bool mt6660_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return true;
> +}

There's no need to do this, there's no cache configured.

> +static unsigned int mt6660_component_io_read(
> +	struct snd_soc_component *component, unsigned int reg)
> +{
> +	struct mt6660_chip *chip = snd_soc_component_get_drvdata(component);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, reg, &val);
> +	if (ret < 0) /* ret success -> >= 0, fail -> < - */
> +		return ret;
> +	pr_err("%s val = 0x%x\n", __func__, val);
> +	return val;
> +}

This function appears to be redunddant, ASoC has wrappers for I/O on
components, same for writes.

> +static int data_debug_show(struct seq_file *s, void *data)
> +{
> +	struct dbg_info *di = s->private;
> +	struct dbg_internal *d = &di->internal;

regmap has standard support for dumping the register map via debugfs, no
need to write your own.  You should be able to just remove all the
debugfs code.

> +/*
> + * MT6660 Generic Setting make this chip work normally.
> + * it is tuned by Richtek RDs.
> + */
> +static const struct codec_reg_val generic_reg_inits[] = {
> +	{ MT6660_REG_WDT_CTRL, 0x80, 0x00 },
> +	{ MT6660_REG_SPS_CTRL, 0x01, 0x00 },
> +	{ MT6660_REG_AUDIO_IN2_SEL, 0x1c, 0x04 },

The writes to reserved registers should be fine but things like this
which looks like it's configuring the input path should just be left at
the chip default, we don't want to be configuring for particular boards
since the same driver will be used for every board with the chip.

> +	{ MT6660_REG_HPF1_COEF, 0xffffffff, 0x7fdb7ffe },
> +	{ MT6660_REG_HPF2_COEF, 0xffffffff, 0x7fdb7ffe },

Similarly here.

> +static int mt6660_component_init_setting(struct snd_soc_component *component)
> +{
> +	int i, len, ret;
> +	const struct codec_reg_val *init_table;
> +
> +	pr_info("%s start\n", __func__);

These pr_info() calls are going to be too noisy.

> +	switch (level) {
> +	case SND_SOC_BIAS_OFF:
> +		ret = regmap_read(chip->regmap, MT6660_REG_IRQ_STATUS1, &val);
> +		dev_info(component->dev,
> +			"%s reg0x05 = 0x%x\n", __func__, val);
> +		break;

This is just making noise, it looks like there's nothing to do in this
function at all and the above is only for debugging.  There's lots of
these throughout the driver.

> +static int mt6660_component_put_volsw(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	int put_ret = 0;
> +
> +	pm_runtime_get_sync(component->dev);
> +	put_ret = snd_soc_put_volsw(kcontrol, ucontrol);
> +	if (put_ret < 0)
> +		return put_ret;
> +	pm_runtime_put(component->dev);
> +	return put_ret;
> +}

It would be *much* better to just use a register cache here rather than
open code like this, and given that the device is suspended via the
register map it is more than a little surprising that there's any need
to do anything special here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]