| Message ID | 20190815191815.755606-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] usb: typec: tcpm: Use usb_debug_root as root for our debugfs entry | expand |
On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote: > Use usb_debug_root as root for our debugfs entry instead of creating our > own subdirectory under the debugfs root. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/fusb302.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 93244d6c4bff..69a2afaf8f68 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -26,6 +26,7 @@ > #include <linux/spinlock.h> > #include <linux/string.h> > #include <linux/types.h> > +#include <linux/usb.h> > #include <linux/usb/typec.h> > #include <linux/usb/tcpm.h> > #include <linux/usb/pd.h> > @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v) > } > DEFINE_SHOW_ATTRIBUTE(fusb302_debug); > > -static struct dentry *rootdir; > - > static void fusb302_debugfs_init(struct fusb302_chip *chip) > { > mutex_init(&chip->logbuffer_lock); > - if (!rootdir) > - rootdir = debugfs_create_dir("fusb302", NULL); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > - S_IFREG | 0444, rootdir, > + S_IFREG | 0444, usb_debug_root, > chip, &fusb302_debug_fops); > } > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > } > > #else > -- > 2.23.0.rc2 >
On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote: > Use usb_debug_root as root for our debugfs entry instead of creating our > own subdirectory under the debugfs root. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I have one question below. Otherwise: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/tcpm/fusb302.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 93244d6c4bff..69a2afaf8f68 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -26,6 +26,7 @@ > #include <linux/spinlock.h> > #include <linux/string.h> > #include <linux/types.h> > +#include <linux/usb.h> > #include <linux/usb/typec.h> > #include <linux/usb/tcpm.h> > #include <linux/usb/pd.h> > @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v) > } > DEFINE_SHOW_ATTRIBUTE(fusb302_debug); > > -static struct dentry *rootdir; > - > static void fusb302_debugfs_init(struct fusb302_chip *chip) > { > mutex_init(&chip->logbuffer_lock); > - if (!rootdir) > - rootdir = debugfs_create_dir("fusb302", NULL); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > - S_IFREG | 0444, rootdir, > + S_IFREG | 0444, usb_debug_root, > chip, &fusb302_debug_fops); In tcpm.c you named the entries "tcpm-%s", dev_name(... Shouldn't we do something similar with these as well? I mean, even though this is just debugfs, shouldn't we give some hint for the user how to identify these entries? How about if we still continue grouping the entries under the "fusb302" directory, but just place that under usb_debug_root? thanks,
Hi, On 16-08-19 09:51, Heikki Krogerus wrote: > On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote: >> Use usb_debug_root as root for our debugfs entry instead of creating our >> own subdirectory under the debugfs root. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > I have one question below. Otherwise: > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > >> --- >> drivers/usb/typec/tcpm/fusb302.c | 9 ++------- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c >> index 93244d6c4bff..69a2afaf8f68 100644 >> --- a/drivers/usb/typec/tcpm/fusb302.c >> +++ b/drivers/usb/typec/tcpm/fusb302.c >> @@ -26,6 +26,7 @@ >> #include <linux/spinlock.h> >> #include <linux/string.h> >> #include <linux/types.h> >> +#include <linux/usb.h> >> #include <linux/usb/typec.h> >> #include <linux/usb/tcpm.h> >> #include <linux/usb/pd.h> >> @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v) >> } >> DEFINE_SHOW_ATTRIBUTE(fusb302_debug); >> >> -static struct dentry *rootdir; >> - >> static void fusb302_debugfs_init(struct fusb302_chip *chip) >> { >> mutex_init(&chip->logbuffer_lock); >> - if (!rootdir) >> - rootdir = debugfs_create_dir("fusb302", NULL); >> - >> chip->dentry = debugfs_create_file(dev_name(chip->dev), >> - S_IFREG | 0444, rootdir, >> + S_IFREG | 0444, usb_debug_root, >> chip, &fusb302_debug_fops); > > In tcpm.c you named the entries "tcpm-%s", dev_name(... > > Shouldn't we do something similar with these as well? I mean, > even though this is just debugfs, shouldn't we give some hint for the > user how to identify these entries? Well on my test-machinw te dev_name is i2c-fusb302, but I realize the dev_name is not always going to be so descriptive, so I will send out a v2 adding a "fusb302-" prefix, like how we are doing for the tcpm.c debug entry already. > How about if we still continue grouping the entries under the > "fusb302" directory, but just place that under usb_debug_root? I would rather go with a prefix, the whole purpose of dropping the subdir is so that we do not have a resource (the dir) shared between multiple instances of the driver. Regards, Hans
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index 93244d6c4bff..69a2afaf8f68 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -26,6 +26,7 @@ #include <linux/spinlock.h> #include <linux/string.h> #include <linux/types.h> +#include <linux/usb.h> #include <linux/usb/typec.h> #include <linux/usb/tcpm.h> #include <linux/usb/pd.h> @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v) } DEFINE_SHOW_ATTRIBUTE(fusb302_debug); -static struct dentry *rootdir; - static void fusb302_debugfs_init(struct fusb302_chip *chip) { mutex_init(&chip->logbuffer_lock); - if (!rootdir) - rootdir = debugfs_create_dir("fusb302", NULL); - chip->dentry = debugfs_create_file(dev_name(chip->dev), - S_IFREG | 0444, rootdir, + S_IFREG | 0444, usb_debug_root, chip, &fusb302_debug_fops); } static void fusb302_debugfs_exit(struct fusb302_chip *chip) { debugfs_remove(chip->dentry); - debugfs_remove(rootdir); } #else
Use usb_debug_root as root for our debugfs entry instead of creating our own subdirectory under the debugfs root. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/typec/tcpm/fusb302.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)