From: Charles Keepax <ckeepax@opensource.cirrus.com> To: Thomas Preston <thomas.preston@codethink.co.uk> Cc: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.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 13:38:25 +0100 [thread overview] Message-ID: <20190730123825.GG54126@ediswmail.ad.cirrus.com> (raw) In-Reply-To: <20190730120937.16271-3-thomas.preston@codethink.co.uk> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote: > Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier > supports 4 audio channels but can support up to 16 with multiple > devices. > > Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> > Cc: Patrick Glaser <pglaser@tesla.com> > Cc: Rob Duncan <rduncan@tesla.com> > Cc: Nate Case <ncase@tesla.com> > --- > Changes since v1: > - Use ALSA kcontrol interface to expose device controls to userland > - Gain > - Channel diagnostic mode > - Impedance efficiency optimiser. I decided against setting this > as a DT property since it seems like something that can be > changed on the fly. > - Add regmap default values > - Channel unmute by default is added in a downstream patch. > - I'm not sure if I should keep this since they're all zero, > although there are other drivers will all-zero reg_defaults. > - I believe the "//" style is used for SPDX headers in normal C source files. > https://lwn.net/Articles/739183/ > - Drop the "enable" sysfs device attribute. > - Don't set TDM format using magic numbers. > - Set sample rate using hw_params. > - Remove unecessary defines. > - Use DAPM to handle AMP_ON. > - Cosmetic fixups > > sound/soc/codecs/Kconfig | 6 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 517 insertions(+) > create mode 100644 sound/soc/codecs/tda7802.c > > +++ b/sound/soc/codecs/tda7802.c > @@ -0,0 +1,509 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tda7802.c -- codec driver for ST TDA7802 > + * > + * Copyright (C) 2016-2019 Tesla Motors, Inc. > + */ Better to make the whole comment // see something like sound/soc/codecs/cs47l35.c for an example. > +static int tda7802_set_bias_level(struct snd_soc_component *component, > + enum snd_soc_bias_level level) > +{ > + const struct tda7802_priv *tda7802 = > + snd_soc_component_get_drvdata(component); > + struct snd_soc_dapm_context *dapm_context = > + snd_soc_component_get_dapm(component); > + const enum snd_soc_bias_level oldlevel = > + snd_soc_dapm_get_bias_level(dapm_context); > + int err = 0; > + > + dev_dbg(component->dev, "%s level %d\n", __func__, level); > + > + switch (level) { > + case SND_SOC_BIAS_ON: > + break; > + case SND_SOC_BIAS_PREPARE: > + break; > + 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); > + > + if (oldlevel == SND_SOC_BIAS_OFF) { > + dev_dbg(component->dev, "Syncing regcache\n"); > + err = regcache_sync(component->regmap); > + if (err < 0) > + dev_err(component->dev, > + "Could not sync regcache, %d\n", err); If your doing a regcache_sync I would probably have expected to see calls to regcache_cache_only. If the device needs syncing that implies the hardware registers have lost state, so there is little point in writing to them if they are unavailable/about to loose their state. > + } > + break; > + case SND_SOC_BIAS_OFF: > + regcache_mark_dirty(component->regmap); > + err = regulator_disable(tda7802->enable_reg); > + if (err < 0) > + dev_err(component->dev, "Could not disable.\n"); > + break; > + } > + > + return err; > +} Thanks, Charles
WARNING: multiple messages have this Message-ID (diff)
From: Charles Keepax <ckeepax@opensource.cirrus.com> To: Thomas Preston <thomas.preston@codethink.co.uk> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, Marco Felsch <m.felsch@pengutronix.de>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Kirill Marinushkin <kmarinushkin@birdec.tech>, Cheng-Yi Chiang <cychiang@chromium.org>, linux-kernel@vger.kernel.org, Nate Case <ncase@tesla.com>, Takashi Iwai <tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, Paul Cercueil <paul@crapouillou.net>, Vinod Koul <vkoul@kernel.org>, Mark Brown <broonie@kernel.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Annaliese McDermond <nh6z@nh6z.net>, Rob Duncan <rduncan@tesla.com>, Patrick Glaser <pglaser@tesla.com>, Jerome Brunet <jbrunet@baylibre.com> Subject: Re: [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802 Date: Tue, 30 Jul 2019 13:38:25 +0100 [thread overview] Message-ID: <20190730123825.GG54126@ediswmail.ad.cirrus.com> (raw) In-Reply-To: <20190730120937.16271-3-thomas.preston@codethink.co.uk> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote: > Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier > supports 4 audio channels but can support up to 16 with multiple > devices. > > Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> > Cc: Patrick Glaser <pglaser@tesla.com> > Cc: Rob Duncan <rduncan@tesla.com> > Cc: Nate Case <ncase@tesla.com> > --- > Changes since v1: > - Use ALSA kcontrol interface to expose device controls to userland > - Gain > - Channel diagnostic mode > - Impedance efficiency optimiser. I decided against setting this > as a DT property since it seems like something that can be > changed on the fly. > - Add regmap default values > - Channel unmute by default is added in a downstream patch. > - I'm not sure if I should keep this since they're all zero, > although there are other drivers will all-zero reg_defaults. > - I believe the "//" style is used for SPDX headers in normal C source files. > https://lwn.net/Articles/739183/ > - Drop the "enable" sysfs device attribute. > - Don't set TDM format using magic numbers. > - Set sample rate using hw_params. > - Remove unecessary defines. > - Use DAPM to handle AMP_ON. > - Cosmetic fixups > > sound/soc/codecs/Kconfig | 6 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 517 insertions(+) > create mode 100644 sound/soc/codecs/tda7802.c > > +++ b/sound/soc/codecs/tda7802.c > @@ -0,0 +1,509 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tda7802.c -- codec driver for ST TDA7802 > + * > + * Copyright (C) 2016-2019 Tesla Motors, Inc. > + */ Better to make the whole comment // see something like sound/soc/codecs/cs47l35.c for an example. > +static int tda7802_set_bias_level(struct snd_soc_component *component, > + enum snd_soc_bias_level level) > +{ > + const struct tda7802_priv *tda7802 = > + snd_soc_component_get_drvdata(component); > + struct snd_soc_dapm_context *dapm_context = > + snd_soc_component_get_dapm(component); > + const enum snd_soc_bias_level oldlevel = > + snd_soc_dapm_get_bias_level(dapm_context); > + int err = 0; > + > + dev_dbg(component->dev, "%s level %d\n", __func__, level); > + > + switch (level) { > + case SND_SOC_BIAS_ON: > + break; > + case SND_SOC_BIAS_PREPARE: > + break; > + 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); > + > + if (oldlevel == SND_SOC_BIAS_OFF) { > + dev_dbg(component->dev, "Syncing regcache\n"); > + err = regcache_sync(component->regmap); > + if (err < 0) > + dev_err(component->dev, > + "Could not sync regcache, %d\n", err); If your doing a regcache_sync I would probably have expected to see calls to regcache_cache_only. If the device needs syncing that implies the hardware registers have lost state, so there is little point in writing to them if they are unavailable/about to loose their state. > + } > + break; > + case SND_SOC_BIAS_OFF: > + regcache_mark_dirty(component->regmap); > + err = regulator_disable(tda7802->enable_reg); > + if (err < 0) > + dev_err(component->dev, "Could not disable.\n"); > + break; > + } > + > + return err; > +} Thanks, Charles
next prev parent reply other threads:[~2019-07-30 12:40 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 [this message] 2019-07-30 12:38 ` Charles Keepax 2019-07-30 15:49 ` [alsa-devel] " Thomas Preston 2019-07-30 14:58 ` Mark Brown 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=20190730123825.GG54126@ediswmail.ad.cirrus.com \ --to=ckeepax@opensource.cirrus.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --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.