From: Mark Brown <broonie@kernel.org> To: Thomas Preston <thomas.preston@codethink.co.uk> Cc: Liam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, Charles Keepax <ckeepax@opensource.cirrus.com>, Jerome Brunet <jbrunet@baylibre.com>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Marco Felsch <m.felsch@pengutronix.de>, Paul Cercueil <paul@crapouillou.net>, Kirill Marinushkin <kmarinushkin@birdec.tech>, Cheng-Yi Chiang <cychiang@chromium.org>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Vinod Koul <vkoul@kernel.org>, Annaliese McDermond <nh6z@nh6z.net>, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Patrick Glaser <pglaser@tesla.com>, Rob Duncan <rduncan@tesla.com>, Nate Case <ncase@tesla.com> Subject: Re: [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802 Date: Tue, 30 Jul 2019 15:58:44 +0100 [thread overview] Message-ID: <20190730145844.GI4264@sirena.org.uk> (raw) In-Reply-To: <20190730120937.16271-3-thomas.preston@codethink.co.uk> [-- Attachment #1: Type: text/plain, Size: 2861 bytes --] On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote: > index 000000000000..0f82a88bc1a4 > --- /dev/null > +++ b/sound/soc/codecs/tda7802.c > @@ -0,0 +1,509 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tda7802.c -- codec driver for ST TDA7802 Please make the entire comment a C++ one so this looks intentional. > +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute) > +{ > + const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED; Please write normal conditional statements to make the code easier to read. > + case SND_SOC_BIAS_STANDBY: > + err = regulator_enable(tda7802->enable_reg); > + if (err < 0) { > + dev_err(component->dev, "Could not enable.\n"); > + return err; > + } > + dev_dbg(component->dev, "Regulator enabled\n"); > + msleep(ENABLE_DELAY_MS); Is this delay needed by the device or is it for the regulator to ramp? If it's for the regulator to ramp then the regulator should be doing it. > + case SND_SOC_BIAS_OFF: > + regcache_mark_dirty(component->regmap); If the regulator is going off you should really be marking the device as cache only. > + err = regulator_disable(tda7802->enable_reg); > + if (err < 0) > + dev_err(component->dev, "Could not disable.\n"); Any non-zero value from regulator_disable() is an error, there's similar error checking issues in other places. > +static const struct snd_kcontrol_new tda7802_snd_controls[] = { > + SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0), > + SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0), > + SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0), > + SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0), These look like simple on/off switches so should have Switch at the end of the control name. It's also not clear to me why this is exported to userspace - why would this change at runtime and won't any changes need to be coordinated with whatever else is connected to the signal? > + SOC_ENUM("Mute time", mute_time), > + SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0), > + SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0), These are also Switch controls. There are *lots* of problems with control names, see control-names.rst. > +static const struct snd_soc_component_driver tda7802_component_driver = { > + .set_bias_level = tda7802_set_bias_level, > + .idle_bias_on = 1, Any reason to keep the device powered up? It looks like the power on process is just powering things up and writing the register cache out and there's not that many registers so the delay is trivial. > + tda7802->enable_reg = devm_regulator_get(dev, "enable"); > + if (IS_ERR(tda7802->enable_reg)) { > + dev_err(dev, "Failed to get enable regulator\n"); It's better to print error codes if you have them and are printing a diagnostic so people have more to go on when debugging problems. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org> To: Thomas Preston <thomas.preston@codethink.co.uk> Cc: Liam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, Charles Keepax <ckeepax@opensource.cirrus.com>, Jerome Brunet <jbrunet@baylibre.com>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Marco Felsch <m.felsch@pengutronix.de>, Paul Cercueil <paul@crapouillou.net>, Kirill Marinushkin <kmarinushkin@birdec.tech>, Cheng-Yi Chiang <cychiang@chromium.org>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Vinod Koul <vkoul@kernel.org>, Annaliese McDermond <nh6z@nh6z.net>, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Patrick Glaser <pglaser@tesla.com>, Rob Duncan <rduncan@tesla.com>, Nate Subject: Re: [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802 Date: Tue, 30 Jul 2019 15:58:44 +0100 [thread overview] Message-ID: <20190730145844.GI4264@sirena.org.uk> (raw) In-Reply-To: <20190730120937.16271-3-thomas.preston@codethink.co.uk> [-- Attachment #1: Type: text/plain, Size: 2861 bytes --] On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote: > index 000000000000..0f82a88bc1a4 > --- /dev/null > +++ b/sound/soc/codecs/tda7802.c > @@ -0,0 +1,509 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tda7802.c -- codec driver for ST TDA7802 Please make the entire comment a C++ one so this looks intentional. > +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute) > +{ > + const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED; Please write normal conditional statements to make the code easier to read. > + case SND_SOC_BIAS_STANDBY: > + err = regulator_enable(tda7802->enable_reg); > + if (err < 0) { > + dev_err(component->dev, "Could not enable.\n"); > + return err; > + } > + dev_dbg(component->dev, "Regulator enabled\n"); > + msleep(ENABLE_DELAY_MS); Is this delay needed by the device or is it for the regulator to ramp? If it's for the regulator to ramp then the regulator should be doing it. > + case SND_SOC_BIAS_OFF: > + regcache_mark_dirty(component->regmap); If the regulator is going off you should really be marking the device as cache only. > + err = regulator_disable(tda7802->enable_reg); > + if (err < 0) > + dev_err(component->dev, "Could not disable.\n"); Any non-zero value from regulator_disable() is an error, there's similar error checking issues in other places. > +static const struct snd_kcontrol_new tda7802_snd_controls[] = { > + SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0), > + SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0), > + SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0), > + SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0), These look like simple on/off switches so should have Switch at the end of the control name. It's also not clear to me why this is exported to userspace - why would this change at runtime and won't any changes need to be coordinated with whatever else is connected to the signal? > + SOC_ENUM("Mute time", mute_time), > + SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0), > + SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0), These are also Switch controls. There are *lots* of problems with control names, see control-names.rst. > +static const struct snd_soc_component_driver tda7802_component_driver = { > + .set_bias_level = tda7802_set_bias_level, > + .idle_bias_on = 1, Any reason to keep the device powered up? It looks like the power on process is just powering things up and writing the register cache out and there's not that many registers so the delay is trivial. > + tda7802->enable_reg = devm_regulator_get(dev, "enable"); > + if (IS_ERR(tda7802->enable_reg)) { > + dev_err(dev, "Failed to get enable regulator\n"); It's better to print error codes if you have them and are printing a diagnostic so people have more to go on when debugging problems. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-07-30 14:59 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-30 12:09 [PATCH v2 0/3] ASoC: Codecs: Add TDA7802 codec Thomas Preston 2019-07-30 12:09 ` Thomas Preston 2019-07-30 12:09 ` [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier Thomas Preston 2019-07-30 12:09 ` Thomas Preston 2019-07-30 12:27 ` Charles Keepax 2019-07-30 12:27 ` Charles Keepax 2019-07-30 13:12 ` Marco Felsch 2019-07-30 13:12 ` Marco Felsch 2019-07-30 14:12 ` [alsa-devel] " Thomas Preston 2019-07-30 14:33 ` Mark Brown 2019-07-30 14:10 ` Thomas Preston 2019-07-30 12:09 ` [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802 Thomas Preston 2019-07-30 12:09 ` Thomas Preston 2019-07-30 12:38 ` Charles Keepax 2019-07-30 12:38 ` Charles Keepax 2019-07-30 15:49 ` [alsa-devel] " Thomas Preston 2019-07-30 14:58 ` Mark Brown [this message] 2019-07-30 14:58 ` Mark Brown 2019-07-30 17:26 ` [alsa-devel] " Thomas Preston 2019-07-31 6:06 ` Marco Felsch 2019-07-31 8:57 ` Thomas Preston 2019-07-30 12:09 ` [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine Thomas Preston 2019-07-30 12:09 ` Thomas Preston 2019-07-30 12:41 ` Charles Keepax 2019-07-30 12:41 ` Charles Keepax 2019-07-30 14:04 ` [alsa-devel] " Thomas Preston 2019-07-30 14:18 ` Charles Keepax 2019-07-30 14:18 ` Charles Keepax 2019-07-30 14:20 ` [alsa-devel] " Mark Brown 2019-07-30 15:27 ` Thomas Preston 2019-07-30 14:19 ` Mark Brown 2019-07-30 14:19 ` Mark Brown 2019-07-30 15:25 ` [alsa-devel] " Thomas Preston 2019-07-30 15:50 ` Mark Brown 2019-07-30 16:28 ` Thomas Preston 2019-07-31 8:03 ` Charles Keepax 2019-07-31 8:03 ` Charles Keepax 2019-08-01 23:42 ` Mark Brown 2019-08-02 8:32 ` Thomas Preston 2019-08-02 11:10 ` Mark Brown 2019-08-02 14:51 ` Thomas Preston 2019-08-02 17:27 ` Mark Brown
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=20190730145844.GI4264@sirena.org.uk \ --to=broonie@kernel.org \ --cc=alsa-devel@alsa-project.org \ --cc=ckeepax@opensource.cirrus.com \ --cc=cychiang@chromium.org \ --cc=devicetree@vger.kernel.org \ --cc=jbrunet@baylibre.com \ --cc=kmarinushkin@birdec.tech \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=m.felsch@pengutronix.de \ --cc=mark.rutland@arm.com \ --cc=ncase@tesla.com \ --cc=nh6z@nh6z.net \ --cc=paul@crapouillou.net \ --cc=perex@perex.cz \ --cc=pglaser@tesla.com \ --cc=rduncan@tesla.com \ --cc=robh+dt@kernel.org \ --cc=srinivas.kandagatla@linaro.org \ --cc=thomas.preston@codethink.co.uk \ --cc=tiwai@suse.com \ --cc=vkoul@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.