Thanks for the feedback Cezary.
+static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct sdw_slave *slave = file->private_data; + unsigned int reg; + char *buf; + ssize_t ret; + int i, j;
+ buf = kzalloc(RD_BUF, GFP_KERNEL); + if (!buf) + return -ENOMEM;
+ ret = scnprintf(buf, RD_BUF, "Register Value\n"); + ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
+ for (i = 0; i < 6; i++) + ret += sdw_sprintf(slave, buf, ret, i);
In most cases explicit reg macro is used, here it's implicit. Align with the rest?
I don't see what you are referring to, or I need more coffee. we use this function sdw_printf in a number of places. Or are you referring to the magic value 6? That should indeed be a macro.
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n"); + ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN); + for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++) + ret += sdw_sprintf(slave, buf, ret, i);
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n"); + ret += sdw_sprintf(slave, buf, ret, + SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET); + for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET; + i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++) + ret += sdw_sprintf(slave, buf, ret, i);
I'd advice to revisit macros declarations first. There should be SDW_DP0_SAMPLECTRL1_B(bank) declared. In general all macros for SDW should be "bank-less" (name wise). Additionally, SDW_BANK_OFFSET(bank) could be provided for convenience i.e.: return 0 for bank0. Yeah, there might be some speed loss in terms of operation count but in most cases it is negligible.
Would simplify this entire reg dump greatly. const array on top with {0, 1} elements and replacing explicit "bank0/1" strings with "bank%d" gets code size reduced while not losing on readability.
This could require a lot of changes in other parts of the code, and I don't want to do this just for debugfs. It's valid point that maybe the code can be simplified, but the changes are an across-the-board change to be done when we don't add new functionality. I'll keep this on the todo list.
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n"); + for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++) + ret += sdw_sprintf(slave, buf, ret, i); + for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++) + ret += sdw_sprintf(slave, buf, ret, i);
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n"); + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0); + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n"); + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1); + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
+ for (i = 1; i < 14; i++) {
Explicit valid slave addresses would be preferred.
no, these are ports. we should use a macro instead of the magic 14 but it's fine to try and read all ports. As I explained it's a good way to figure out how many ports the Slave device supports even in the absence of documentation. This also helps figure out if the DisCo properties make sense.
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i); + reg = SDW_DPN_INT(i); + for (j = 0; j < 6; j++) + ret += sdw_sprintf(slave, buf, ret, reg + j);
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n"); + reg = SDW_DPN_CHANNELEN_B0(i); + for (j = 0; j < 9; j++) + ret += sdw_sprintf(slave, buf, ret, reg + j);
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n"); + reg = SDW_DPN_CHANNELEN_B1(i); + for (j = 0; j < 9; j++) + ret += sdw_sprintf(slave, buf, ret, reg + j);
Some sort of MAX_CHANNELS would be nice here too.
Yes, need to use macros indeed.
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave) +{ + struct dentry *master; + struct dentry *d; + char name[32];
+ master = slave->bus->debugfs;
+ /* create the debugfs slave-name */ + snprintf(name, sizeof(name), "%s", dev_name(&slave->dev)); + d = debugfs_create_dir(name, master);
+ debugfs_create_file("registers", 0400, d, slave, &sdw_slave_reg_fops);
Pointer returned by _create_file gets completely ignored here. At least dbg msg would be nice if it fails.
+ return d;
I understood that Greg KH doesn't want us to depend on the result of debugfs calls, but a dev_dbg is likely ok.