On 21-01-21, 17:23, Srinivas Kandagatla wrote: > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > > > > > > > On 19/01/2021 19:09, Pierre-Louis Bossart wrote: > > > > > > > > > currently we have > > > > > /sys/kernel/debug/soundwire/master-* > > > > > > > > > > Are you suggesting that we have something like this: > > > > > > > > > > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? > > > > > > > > Yes this is what I was thinking about. > > > > > > Vinod/Pierre, > > > > > > One Question here, > > > > > > Why is link_id part of "struct sdw_bus", should it not be part of > > > "struct sdw_master_device " ? > > > > > > Given that "There is one Link per each Master" > > > > it's true that link == master == bus at the concept level. > > > > but we have an existing code base with different structures and we can't > > break too many things at once. > > > > In the existing flow, the 'bus' is created and setup first, the > > sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is > > already set. This routine only creates a device and in the rest of the > > code we keep using the 'bus' pointer, so there's no real short-term > > scope for moving the information into the 'sdw_master_device' structure > > - that would be a lot of surgery when nothing is really broken. > > I totally agree! > > If I understand it correctly in Intel case there will be only one Link ID > per bus. Yes IIUC there would be one link id per bus. the ida approach gives us unique id for each master,bus I would like to propose using that everywhere > > > Does this change look good to you? > > ---------------->cut<--------------- > > diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c > index b6cad0d59b7b..f22868614f09 100644 > --- a/drivers/soundwire/debugfs.c > +++ b/drivers/soundwire/debugfs.c > @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus) > return; > > /* create the debugfs master-N */ > + bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), > sdw_debugfs_root); > snprintf(name, sizeof(name), "master-%d", bus->link_id); > - bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root); > + bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs); > } > > void sdw_bus_debugfs_exit(struct sdw_bus *bus) > { > - debugfs_remove_recursive(bus->debugfs); > + debugfs_remove_recursive(bus->controller_debugfs); > } > > #define RD_BUF (3 * PAGE_SIZE) > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index b198f471bea8..242bde30d8bd 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -877,6 +877,7 @@ struct sdw_bus { > struct sdw_master_prop prop; > struct list_head m_rt_list; > #ifdef CONFIG_DEBUG_FS > + struct dentry *controller_debugfs; > struct dentry *debugfs; > #endif > struct sdw_defer defer_msg; > > ---------------->cut<--------------- > > With this change I get something like this on my board: > > ~# find /sys/kernel/debug/soundwire/ > /sys/kernel/debug/soundwire/ > /sys/kernel/debug/soundwire/sdw-master-2 > /sys/kernel/debug/soundwire/sdw-master-2/master-0 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers > /sys/kernel/debug/soundwire/sdw-master-1 > /sys/kernel/debug/soundwire/sdw-master-1/master-0 > /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3 > /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers > /sys/kernel/debug/soundwire/sdw-master-0 > /sys/kernel/debug/soundwire/sdw-master-0/master-0 > /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4 > /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers > > > > Thanks, > srini -- ~Vinod