[alsa-devel] [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver

[jeff_chang(張世佳)]

Dear Mark:



        Thanks for your replying. I consider that there was only one item I should modify. So I kept resending.



        Please refer my red comment after your comment.



        Thanks a lot!



Thanks & BRs

Jeff





-----Original Message-----
From: Mark Brown [mailto:broonie at kernel.org]
Sent: Thursday, December 12, 2019 10:54 PM
To: Jeff Chang <richtek.jeff.chang at gmail.com>
Cc: lgirdwood at gmail.com; perex at perex.cz; tiwai at suse.com; matthias.bgg at gmail.com; alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; jeff_chang(張世佳) <jeff_chang at richtek.com>
Subject: Re: [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver



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 at gmail.com<mailto:richtek.jeff.chang at 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.



        OK! I will do it at next patch!



> +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?



        I will remove it.



> +++ 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.



        OK!



> +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.



        OK!



> +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.



        OK!



> +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.



        The chip default cannot be modified anymore. How can I do if I really need write these setting in our drivers?



> +   { 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.



        OK! I will remove it.



> +   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.



        It is prepare for more flexible using. Anyway, I will remove it.



> +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.



        Ok!

************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!