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 Subject: Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine Date: Tue, 30 Jul 2019 15:19:35 +0100 [thread overview] Message-ID: <20190730141935.GF4264@sirena.org.uk> (raw) In-Reply-To: <20190730120937.16271-4-thomas.preston@codethink.co.uk> [-- Attachment #1: Type: text/plain, Size: 1411 bytes --] On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote: > + struct dentry *debugfs; > + struct mutex diagnostic_mutex; > +}; It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver. > +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status) > +{ > + struct device *dev = &tda7802->i2c->dev; > + int err_status, err; > + unsigned int val; > + u8 state[NUM_IB]; > + /* We must wait 20ms for device to settle, otherwise diagnostics will > + * not start and regmap poll will timeout. > + */ > + msleep(DIAGNOSTIC_SETTLE_MS); The comment and define might go out of sync... > + err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4); > + if (err < 0) { > + dev_err(dev, "Could not read channel status, %d\n", err); > + goto diagnostic_restore; > + } ...but here we use a magic number for the array size :( > +static int tda7802_diagnostic_show(struct seq_file *f, void *p) > +{ > + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL); We neither use nor free buf? > +static int tda7802_probe(struct snd_soc_component *component) > +{ > + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component); > + struct device *dev = &tda7802->i2c->dev; > + int err; Why is this done at the component level? [-- 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: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, Charles Keepax <ckeepax@opensource.cirrus.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Kirill Marinushkin <kmarinushkin@birdec.tech>, Cheng-Yi Chiang <cychiang@chromium.org>, Marco Felsch <m.felsch@pengutronix.de>, Takashi Iwai <tiwai@suse.com>, Annaliese McDermond <nh6z@nh6z.net>, Liam Girdwood <lgirdwood@gmail.com>, Paul Cercueil <paul@crapouillou.net>, Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, linux-kernel@vger.kernel.org, Jerome Brunet <jbrunet@baylibre.com> Subject: Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine Date: Tue, 30 Jul 2019 15:19:35 +0100 [thread overview] Message-ID: <20190730141935.GF4264@sirena.org.uk> (raw) In-Reply-To: <20190730120937.16271-4-thomas.preston@codethink.co.uk> [-- Attachment #1.1: Type: text/plain, Size: 1411 bytes --] On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote: > + struct dentry *debugfs; > + struct mutex diagnostic_mutex; > +}; It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver. > +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status) > +{ > + struct device *dev = &tda7802->i2c->dev; > + int err_status, err; > + unsigned int val; > + u8 state[NUM_IB]; > + /* We must wait 20ms for device to settle, otherwise diagnostics will > + * not start and regmap poll will timeout. > + */ > + msleep(DIAGNOSTIC_SETTLE_MS); The comment and define might go out of sync... > + err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4); > + if (err < 0) { > + dev_err(dev, "Could not read channel status, %d\n", err); > + goto diagnostic_restore; > + } ...but here we use a magic number for the array size :( > +static int tda7802_diagnostic_show(struct seq_file *f, void *p) > +{ > + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL); We neither use nor free buf? > +static int tda7802_probe(struct snd_soc_component *component) > +{ > + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component); > + struct device *dev = &tda7802->i2c->dev; > + int err; Why is this done at the component level? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]
next prev parent reply other threads:[~2019-07-30 14:20 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 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 [this message] 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=20190730141935.GF4264@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=nh6z@nh6z.net \ --cc=paul@crapouillou.net \ --cc=perex@perex.cz \ --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.