Skip to contentSkip to conversation

ASoC: wm_adsp: Add basic debugfs entries

PATCH In review Richard Fitzgerald 2 participants 5 messages Jun 8, 2015 – Jun 9, 2015

Subsystems (3)
Sound ASoC Wolfson microelectronics drivers
lore
Share & tools
lore

Richard Fitzgerald

author
#0 lore raw

PATCH 1/1 ASoC: wm_adsp: Add basic debugfs entries

This patch adds some debugfs nodes to get information about the currently running firmware.

Signed-off-by Richard Fitzgerald <[email protected]>
---
 sound/soc/codecs/wm5102.c  |    4 +
 sound/soc/codecs/wm5110.c  |    9 ++-
 sound/soc/codecs/wm_adsp.c |  182 ++++++++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/wm_adsp.h |   25 ++++++-
 4 files changed, 212 insertions(+), 8 deletions(-)
sound/soc/codecs/wm5102.c +4 -0
index 11eba0e..22fc9e9 100644
@@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec)
 	struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
 	int ret;
 
+	wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
+
 	ret = snd_soc_add_codec_controls(codec, wm_adsp2_fw_controls, 2);
 	if (ret != 0)
 		return ret;

@@ -1893,6 +1895,8 @@ static int wm5102_codec_remove(struct snd_soc_codec *codec)
 {
 	struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
 
+	wm_adsp_cleanup_debugfs(&priv->core.adsp[0]);
+
 	priv->core.arizona->dapm = NULL;
 
 	return 0;
sound/soc/codecs/wm5110.c +8 -1
index d65364e..f9ca1a9 100644
@@ -1599,7 +1599,10 @@ static struct snd_soc_dai_driver wm5110_dai[] = {
 static int wm5110_codec_probe(struct snd_soc_codec *codec)
 {
 	struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec);
-	int ret;
+	int i, ret;
+
+	for (i = 0; i < WM5110_NUM_ADSP; i++)
+		wm_adsp_init_debugfs(&priv->core.adsp[i], codec);
 
 	priv->core.arizona->dapm = &codec->dapm;
 

@@ -1621,6 +1624,10 @@ static int wm5110_codec_probe(struct snd_soc_codec *codec)
 static int wm5110_codec_remove(struct snd_soc_codec *codec)
 {
 	struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec);
+	int i;
+
+	for (i = 0; i < WM5110_NUM_ADSP; i++)
+		wm_adsp_cleanup_debugfs(&priv->core.adsp[i]);
 
 	priv->core.arizona->dapm = NULL;
sound/soc/codecs/wm_adsp.c +176 -6

232 lines not on this page · raw message

sound/soc/codecs/wm_adsp.h +24 -1
index 0e5f07c..1d563fe 100644
@@ -46,17 +46,25 @@ struct wm_adsp {
 	struct list_head alg_regions;
 
 	int fw_id;
+	int fw_id_version;
 
 	const struct wm_adsp_region *mem;
 	int num_mems;
 
 	int fw;
 	int fw_ver;
-	bool running;
+	u32 running;
 
 	struct list_head ctl_list;
 
 	struct work_struct boot_work;
+
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs_root;
+	char *wmfw_file_name;
+	char *bin_file_name;
+#endif
+
 };
 
 #define WM_ADSP1(wname, num) \

@@ -79,6 +87,21 @@ extern const struct snd_kcontrol_new wm_adsp2_fw_controls[];
 
 int wm_adsp1_init(struct wm_adsp *dsp);
 int wm_adsp2_init(struct wm_adsp *dsp);
+
+#ifdef CONFIG_DEBUG_FS
+void wm_adsp_init_debugfs(struct wm_adsp *dsp, struct snd_soc_codec *codec);
+void wm_adsp_cleanup_debugfs(struct wm_adsp *dsp);
+#else
+static inline void wm_adsp_init_debugfs(struct wm_adsp *dsp,
+					struct snd_soc_codec *codec)
+{
+}
+
+void wm_adsp_cleanup_debugfs(struct wm_adsp *dsp)
+{
+}
+#endif
+
 int wm_adsp1_event(struct snd_soc_dapm_widget *w,
 		   struct snd_kcontrol *kcontrol, int event);
 int wm_adsp2_early_event(struct snd_soc_dapm_widget *w,
Signature · 1 line
1.7.2.5
maintainer
#1 lore raw

sound/soc/codecs/wm5102.c static int wm5102_codec_probe(struct snd_soc_codec *codec) · line 1879

Quoted 7 lines · Richard Fitzgerald

+++ b/sound/soc/codecs/wm5102.c

sound/soc/codecs/wm5102.c +2 -0
@@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec)
 	struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
 	int ret;
 
+	wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
+

Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?

Quoted 10 lines
sound/soc/codecs/wm_adsp.c +10 -0
+#ifdef CONFIG_DEBUG_FS
+static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_clear(struct wm_adsp *dsp);
+#else
+static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
+						 const char *s)
+{
+}
+

sound/soc/codecs/wm_adsp.c struct wm_coeff_ctl { · line 266

Quoted 10 lines
sound/soc/codecs/wm_adsp.c +10 -0
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
+						const char *s)
+{
+}
+
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp)
+{
+}
+#endif
+

Why not just put the functions here rather than prototypes?

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 1994

Quoted 11 lines
sound/soc/codecs/wm_adsp.c +11 -0
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
+					  char __user *user_buf,
+					  size_t count, loff_t *ppos,
+					  const char *string)
+{
+	char *temp;
+	int len;
+	ssize_t ret;
+
+	if (!string || !dsp->running)
+		return 0;

Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2000

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +5 -0
+	temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!temp)
+		return -ENOMEM;
+
+	len = snprintf(temp, PAGE_SIZE, "%s\n", string);

Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2038

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +4 -0
+} wm_adsp_debugfs_fops[] = {
+	{
+		.name = "wmfw_file",
 
+		.name = "bin_file",

Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.

author
#2 lore raw
Quoted 7 lines · Mark Brown
Quoted 5 lines · Richard Fitzgerald

+++ b/sound/soc/codecs/wm5102.c

sound/soc/codecs/wm5102.c +0 -0
@@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec)
 	struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
 	int ret;
Quoted 6 lines

sound/soc/codecs/wm5102.c static int wm5102_codec_probe(struct snd_soc_codec *codec) · line 1879

Quoted 2 lines
sound/soc/codecs/wm5102.c +2 -0
+	wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
+

Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?

Because we call the existing wm_adsp_init() early in probe and at that point we haven't registered the codec yet.

Quoted 14 lines
Quoted 14 lines
sound/soc/codecs/wm_adsp.c +14 -0
+#ifdef CONFIG_DEBUG_FS
+static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_clear(struct wm_adsp *dsp);
+#else
+static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
+						 const char *s)
+{
+}
+
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
+						const char *s)
+{
+}
Quoted 9 lines

sound/soc/codecs/wm_adsp.c struct wm_coeff_ctl { · line 266

Quoted 6 lines
sound/soc/codecs/wm_adsp.c +6 -0
+
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp)
+{
+}
+#endif
+

Why not just put the functions here rather than prototypes?

It was just personal preference, I like to have the important code higher up in source files and keep the clutter of debug code near the end where it's not in the way but I can turn it around.

Quoted 35 lines

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 1994

Quoted 11 lines
sound/soc/codecs/wm_adsp.c +11 -0
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
+					  char __user *user_buf,
+					  size_t count, loff_t *ppos,
+					  const char *string)
+{
+	char *temp;
+	int len;
+	ssize_t ret;
+
+	if (!string || !dsp->running)
+		return 0;

Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2000

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +5 -0
+	temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!temp)
+		return -ENOMEM;
+
+	len = snprintf(temp, PAGE_SIZE, "%s\n", string);

Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2038

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +4 -0
+} wm_adsp_debugfs_fops[] = {
+	{
+		.name = "wmfw_file",
 
+		.name = "bin_file",

Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.

maintainer
#3 lore raw
Quoted 7 lines · Richard Fitzgerald
Quoted 2 lines · Mark Brown

Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?

Because we call the existing wm_adsp_init() early in probe and at that point we haven't registered the codec yet.

Can you not either hang it off the device or move the registration later?

author
#4 lore raw
In reply to Mark Brown
Quoted 36 lines · Mark Brown

sound/soc/codecs/wm5102.c static int wm5102_codec_probe(struct snd_soc_codec *codec) · line 1879

Quoted 7 lines · Richard Fitzgerald

+++ b/sound/soc/codecs/wm5102.c

sound/soc/codecs/wm5102.c +2 -0
@@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec)
 	struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
 	int ret;
 
+	wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
+

Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?

Quoted 10 lines
sound/soc/codecs/wm_adsp.c +10 -0
+#ifdef CONFIG_DEBUG_FS
+static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s);
+static void wm_adsp_debugfs_clear(struct wm_adsp *dsp);
+#else
+static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
+						 const char *s)
+{
+}
+

sound/soc/codecs/wm_adsp.c struct wm_coeff_ctl { · line 266

Quoted 10 lines
sound/soc/codecs/wm_adsp.c +10 -0
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
+						const char *s)
+{
+}
+
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp)
+{
+}
+#endif
+

Why not just put the functions here rather than prototypes?

Quoted 17 lines

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 1994

Quoted 11 lines
sound/soc/codecs/wm_adsp.c +11 -0
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
+					  char __user *user_buf,
+					  size_t count, loff_t *ppos,
+					  const char *string)
+{
+	char *temp;
+	int len;
+	ssize_t ret;
+
+	if (!string || !dsp->running)
+		return 0;

Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.

If simple_read_from_buffer() is off the end of the buffer or count is 0 it returns 0 not EOF.
Also I checked procfs for cases where things aren't always valid and it returns 0. So this seems to be accepted behaviour.

Quoted 18 lines

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2000

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +5 -0
+	temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!temp)
+		return -ENOMEM;
+
+	len = snprintf(temp, PAGE_SIZE, "%s\n", string);

Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?

sound/soc/codecs/wm_adsp.c int wm_adsp2_init(struct wm_adsp *dsp) · line 2038

Quoted 5 lines
sound/soc/codecs/wm_adsp.c +4 -0
+} wm_adsp_debugfs_fops[] = {
+	{
+		.name = "wmfw_file",
 
+		.name = "bin_file",

Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.