From: Mark Brown <broonie@kernel.org>
To: Jeff Chang <richtek.jeff.chang@gmail.com>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
tiwai@suse.com, lgirdwood@gmail.com, jeff_chang@richtek.com,
matthias.bgg@gmail.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [alsa-devel] [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver
Date: Fri, 20 Dec 2019 12:11:52 +0000 [thread overview]
Message-ID: <20191220121152.GC4790@sirena.org.uk> (raw)
In-Reply-To: <1576836934-5370-1-git-send-email-richtek.jeff.chang@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2651 bytes --]
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.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-12-20 12:12 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-20 10:15 [alsa-devel] [PATCH] ASoC: Add MediaTek MT6660 Speaker Amp Driver Jeff Chang
2019-12-20 12:11 ` Mark Brown [this message]
[not found] ` <7a9bcf5d414c4a74ae8e101c54c9e46f@ex1.rt.l>
2019-12-24 23:51 ` Mark Brown
2019-12-25 1:45 ` jeff_chang(張世佳)
2019-12-25 17:45 ` Mark Brown
-- strict thread matches above, loose matches on Subject: below --
2019-12-12 12:12 Jeff Chang
2019-12-12 14:53 ` Mark Brown
2019-12-13 7:36 ` jeff_chang(張世佳)
2019-12-13 11:32 ` Mark Brown
2019-12-12 11:08 Jeff Chang
2019-12-12 11:57 ` Matthias Brugger
2019-12-12 12:06 ` Mark Brown
2019-12-17 15:17 ` Greg KH
2019-12-12 9:12 Jeff Chang
2019-12-12 10:36 ` Jaroslav Kysela
2019-12-12 11:05 ` jeff_chang(張世佳)
2019-12-12 8:29 [alsa-devel] [PATCH] ASoC: add " Jeff Chang
2019-12-12 8:52 ` Matthias Brugger
2019-12-12 6:58 Jeff Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191220121152.GC4790@sirena.org.uk \
--to=broonie@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=jeff_chang@richtek.com \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=richtek.jeff.chang@gmail.com \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).