On 7/25/19 5:15 PM, Guennadi Liakhovetski wrote:
Hi Pierre,
A couple of nitpicks:
Thanks for the feedback!
create mode 100644 drivers/soundwire/debugfs.c
[snip]
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..06ac4adb0074 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -18,6 +18,30 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
void sdw_extract_slave_id(struct sdw_bus *bus,
u64 addr, struct sdw_slave_id *id);
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct dentry *d);
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct dentry *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
static?
+
+void sdw_bus_debugfs_exit(struct dentry *d) {}
+
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct dentry *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
Same for all the above. You could also declare them inline, but I really hope
the compiler will be smart enough to do that itself.
yes, I'll add static inline for all this.
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+ struct dentry *d;
I would remove the above
+ char name[16];
+
+ if (!sdw_debugfs_root)
+ return NULL;
+
+ /* create the debugfs master-N */
+ snprintf(name, sizeof(name), "master-%d", bus->link_id);
+ d = debugfs_create_dir(name, sdw_debugfs_root);
+
+ return d;
And just do
+ return debugfs_create_dir(name, sdw_debugfs_root);
yep, will do.
+static ssize_t sdw_sprintf(struct sdw_slave *slave,
+ char *buf, size_t pos, unsigned int reg)
+{
+ int value;
+
+ value = sdw_read(slave, reg);
I personally would join the two lines above, but that's just a personal
preference.
I prefer splitting variables and code, I just can't mentally split the two.
+
+ if (value < 0)
+ return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
+ else
I think it's advised to not use an else in such cases.
Thanks
Guennadi
+ return scnprintf(buf + pos, RD_BUF - pos,
+ "%3x\t%2x\n", reg, value);
+}
The intent was to provide a visual cue that the register is not
implemented, which is quite useful. Not all registers are mandatory and
not all vendors document the entire set of registers, so it's a good way
to figure things out. The value is not used for any functional purpose,
it's just a register dump for the integrator to look at. I'll add a note
to explain the idea.
