| Message ID | 20220114163947.790078-1-sean.anderson@seco.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: ulpi: Add debugfs support | expand |
On Fri, Jan 14, 2022 at 11:39:47AM -0500, Sean Anderson wrote: > This adds a debugfs file for ULPI devices which contains a dump of their > registers. This is useful for debugging basic connectivity problems. The > file is created in ulpi_register because many devices will never have a > driver bound (as they are managed in hardware by the USB controller > device). > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > drivers/usb/common/ulpi.c | 100 ++++++++++++++++++++++++++++++++++-- > include/linux/ulpi/driver.h | 3 ++ > 2 files changed, 99 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > index 4169cf40a03b..a39c48e04013 100644 > --- a/drivers/usb/common/ulpi.c > +++ b/drivers/usb/common/ulpi.c > @@ -13,6 +13,7 @@ > #include <linux/module.h> > #include <linux/slab.h> > #include <linux/acpi.h> > +#include <linux/debugfs.h> > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/clk/clk-conf.h> > @@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi) > return 0; > } > > +static int __maybe_unused ulpi_regs_read(struct seq_file *seq, void *data) > +{ > + struct ulpi *ulpi = seq->private; > + > +#define ulpi_print(name, reg) do { \ > + int ret = ulpi_read(ulpi, reg); \ > + if (ret < 0) \ > + return ret; \ > + seq_printf(seq, name " %.02x\n", ret); \ > +} while (0) > + > + ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW); > + ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH); > + ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW); > + ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH); > + ulpi_print("Function Control ", ULPI_FUNC_CTRL); > + ulpi_print("Interface Control ", ULPI_IFC_CTRL); > + ulpi_print("OTG Control ", ULPI_OTG_CTRL); > + ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE); > + ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL); > + ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS); > + ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH); > + ulpi_print("Debug ", ULPI_DEBUG); > + ulpi_print("Scratch Register ", ULPI_SCRATCH); > + ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL); > + ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY); > + ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN); > + ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS); > + ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH); > + ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL); > + ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH); > + ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH); > + ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY); > + > + return 0; > +} > + > +static int __maybe_unused ulpi_regs_open(struct inode *inode, struct file *f) > +{ > + struct ulpi *ulpi = inode->i_private; > + > + return single_open(f, ulpi_regs_read, ulpi); > +} > + > +static const struct file_operations __maybe_unused ulpi_regs_ops = { > + .owner = THIS_MODULE, > + .open = ulpi_regs_open, > + .release = single_release, > + .read = seq_read, > + .llseek = seq_lseek > +}; > + > +static struct dentry *ulpi_root = (void *)-EPROBE_DEFER; There is no need for this variable, nor is there ever a need to set this to an error value like this. If you need to find the root, just look it up! > + > static int ulpi_register(struct device *dev, struct ulpi *ulpi) > { > int ret; > + struct dentry *regs; > > ulpi->dev.parent = dev; /* needed early for ops */ > ulpi->dev.bus = &ulpi_bus; > @@ -245,16 +301,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > ret = ulpi_read_id(ulpi); > if (ret) > - return ret; > + goto err_of; > > ret = device_register(&ulpi->dev); > if (ret) > - return ret; > + goto err_of; > + > + if (IS_ENABLED(CONFIG_DEBUG_FS)) { This check is not needed, the compiler will handle it all for your automatically. > + ulpi->root = debugfs_create_dir(dev_name(dev), ulpi_root); > + if (IS_ERR(ulpi->root)) { No need to check this, just keep moving on. debugfs return values shoudl NEVER be checked as your code should not care what happens. > + ret = PTR_ERR(ulpi->root); > + goto err_dev; > + } > + > + regs = debugfs_create_file("regs", 0444, ulpi->root, ulpi, > + &ulpi_regs_ops); Also, there is no need to save the dentry of "root", just make that a local variable and look it up when you want to remove it. > + if (IS_ERR(regs)) { Again, no need to check this at all. > + ret = PTR_ERR(regs); > + goto err_debugfs; > + } > + } All of this logic here can be reduced to 2 lines of code and one variable: struct dentry *dir; ... dir = debugfs_create_dir(dev_name(dev), debugfs_lookup(KBUILD_MODULE_NAME, NULL)); debugfs_create_file("regs", 0444, dir, ulpi, &ulpi_regs_ops); and that's it. > > dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n", > ulpi->id.vendor, ulpi->id.product); > > return 0; > + > +err_debugfs: > + debugfs_remove(ulpi->root); debugfs_remove_recursive()? > +err_dev: > + device_unregister(&ulpi->dev); > +err_of: > + of_node_put(ulpi->dev.of_node); > + return ret; > } > > /** > @@ -296,8 +375,9 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface); > */ > void ulpi_unregister_interface(struct ulpi *ulpi) > { > - of_node_put(ulpi->dev.of_node); > + debugfs_remove_recursive(ulpi->root); again, look up the name you want to remove, no need to store it around anywhere: debugfs_remove_recursive(debugfs_lookup(dev_name(ulpi->dev), debugfs_lookup(KBUILD_MODULE_NAME, NULL))); > device_unregister(&ulpi->dev); > + of_node_put(ulpi->dev.of_node); > } > EXPORT_SYMBOL_GPL(ulpi_unregister_interface); > > @@ -305,13 +385,25 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface); > > static int __init ulpi_init(void) > { > - return bus_register(&ulpi_bus); > + int ret; > + > + if (IS_ENABLED(CONFIG_DEBUG_FS)) { Again, no need to check > + ulpi_root = debugfs_create_dir("ulpi", NULL); Again, no need to keep this, it can just be: debugfs_create_dir(KBUILD_MODULE_NAME, NULL); > + if (IS_ERR(ulpi_root)) > + return PTR_ERR(ulpi_root); > + } > + > + ret = bus_register(&ulpi_bus); > + if (ret) > + debugfs_remove(ulpi_root); > + return ret; > } > subsys_initcall(ulpi_init); > > static void __exit ulpi_exit(void) > { > bus_unregister(&ulpi_bus); > + debugfs_remove(ulpi_root); debugfs_remove_recursive(debugfs_lookup(KBUILD_MODULE_NAME, NULL)); > } > module_exit(ulpi_exit); > > diff --git a/include/linux/ulpi/driver.h b/include/linux/ulpi/driver.h > index c7a1810373e3..083ea2d2e873 100644 > --- a/include/linux/ulpi/driver.h > +++ b/include/linux/ulpi/driver.h > @@ -6,6 +6,7 @@ > > #include <linux/device.h> > > +struct dentry; > struct ulpi_ops; > > /** > @@ -13,10 +14,12 @@ struct ulpi_ops; > * @id: vendor and product ids for ULPI device > * @ops: I/O access > * @dev: device interface > + * @root: root directory for debugfs files No need for this, as pointed out above. This should make you patch a _lot_ smaller. thanks, greg k-h
On Mon, Jan 24, 2022 at 11:28:26AM -0500, Sean Anderson wrote: > > > On 1/23/22 6:27 AM, Greg Kroah-Hartman wrote: > > On Fri, Jan 14, 2022 at 11:39:47AM -0500, Sean Anderson wrote: > >> This adds a debugfs file for ULPI devices which contains a dump of their > >> registers. This is useful for debugging basic connectivity problems. The > >> file is created in ulpi_register because many devices will never have a > >> driver bound (as they are managed in hardware by the USB controller > >> device). > >> > >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> > >> --- > >> > >> drivers/usb/common/ulpi.c | 100 ++++++++++++++++++++++++++++++++++-- > >> include/linux/ulpi/driver.h | 3 ++ > >> 2 files changed, 99 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > >> index 4169cf40a03b..a39c48e04013 100644 > >> --- a/drivers/usb/common/ulpi.c > >> +++ b/drivers/usb/common/ulpi.c > >> @@ -13,6 +13,7 @@ > >> #include <linux/module.h> > >> #include <linux/slab.h> > >> #include <linux/acpi.h> > >> +#include <linux/debugfs.h> > >> #include <linux/of.h> > >> #include <linux/of_device.h> > >> #include <linux/clk/clk-conf.h> > >> @@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi) > >> return 0; > >> } > >> > >> +static int __maybe_unused ulpi_regs_read(struct seq_file *seq, void *data) > >> +{ > >> + struct ulpi *ulpi = seq->private; > >> + > >> +#define ulpi_print(name, reg) do { \ > >> + int ret = ulpi_read(ulpi, reg); \ > >> + if (ret < 0) \ > >> + return ret; \ > >> + seq_printf(seq, name " %.02x\n", ret); \ > >> +} while (0) > >> + > >> + ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW); > >> + ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH); > >> + ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW); > >> + ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH); > >> + ulpi_print("Function Control ", ULPI_FUNC_CTRL); > >> + ulpi_print("Interface Control ", ULPI_IFC_CTRL); > >> + ulpi_print("OTG Control ", ULPI_OTG_CTRL); > >> + ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE); > >> + ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL); > >> + ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS); > >> + ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH); > >> + ulpi_print("Debug ", ULPI_DEBUG); > >> + ulpi_print("Scratch Register ", ULPI_SCRATCH); > >> + ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL); > >> + ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY); > >> + ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN); > >> + ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS); > >> + ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH); > >> + ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL); > >> + ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH); > >> + ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH); > >> + ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY); > >> + > >> + return 0; > >> +} > >> + > >> +static int __maybe_unused ulpi_regs_open(struct inode *inode, struct file *f) > >> +{ > >> + struct ulpi *ulpi = inode->i_private; > >> + > >> + return single_open(f, ulpi_regs_read, ulpi); > >> +} > >> + > >> +static const struct file_operations __maybe_unused ulpi_regs_ops = { > >> + .owner = THIS_MODULE, > >> + .open = ulpi_regs_open, > >> + .release = single_release, > >> + .read = seq_read, > >> + .llseek = seq_lseek > >> +}; > >> + > >> +static struct dentry *ulpi_root = (void *)-EPROBE_DEFER; > > > > There is no need for this variable, nor is there ever a need to set this > > to an error value like this. If you need to find the root, just look it > > up! > > The reason why it is set to a non-zero value is so that it doesn't get > coalesced with other zero-initialized static variables. That doesn't matter, you shouldn't need to initialize this. > >> + > >> static int ulpi_register(struct device *dev, struct ulpi *ulpi) > >> { > >> int ret; > >> + struct dentry *regs; > >> > >> ulpi->dev.parent = dev; /* needed early for ops */ > >> ulpi->dev.bus = &ulpi_bus; > >> @@ -245,16 +301,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > >> > >> ret = ulpi_read_id(ulpi); > >> if (ret) > >> - return ret; > >> + goto err_of; > >> > >> ret = device_register(&ulpi->dev); > >> if (ret) > >> - return ret; > >> + goto err_of; > >> + > >> + if (IS_ENABLED(CONFIG_DEBUG_FS)) { > > > > This check is not needed, the compiler will handle it all for your > > automatically. > > > >> + ulpi->root = debugfs_create_dir(dev_name(dev), ulpi_root); > >> + if (IS_ERR(ulpi->root)) { > > > > No need to check this, just keep moving on. debugfs return values > > shoudl NEVER be checked as your code should not care what happens. > > OK. The reason we have the above check is so we don't fail here because > if we don't have CONFIG_DEBUG_FS then debugfs_create_dir() will fail > with -ENODEV. That's fine, there is no need to check a debugfs call, and any result returned by a debugfs call can be passed to another debugfs call with no problems. thanks, greg k-h
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index 4169cf40a03b..a39c48e04013 100644 --- a/drivers/usb/common/ulpi.c +++ b/drivers/usb/common/ulpi.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/debugfs.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/clk/clk-conf.h> @@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi) return 0; } +static int __maybe_unused ulpi_regs_read(struct seq_file *seq, void *data) +{ + struct ulpi *ulpi = seq->private; + +#define ulpi_print(name, reg) do { \ + int ret = ulpi_read(ulpi, reg); \ + if (ret < 0) \ + return ret; \ + seq_printf(seq, name " %.02x\n", ret); \ +} while (0) + + ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW); + ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH); + ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW); + ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH); + ulpi_print("Function Control ", ULPI_FUNC_CTRL); + ulpi_print("Interface Control ", ULPI_IFC_CTRL); + ulpi_print("OTG Control ", ULPI_OTG_CTRL); + ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE); + ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL); + ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS); + ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH); + ulpi_print("Debug ", ULPI_DEBUG); + ulpi_print("Scratch Register ", ULPI_SCRATCH); + ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL); + ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY); + ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN); + ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS); + ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH); + ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL); + ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH); + ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH); + ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY); + + return 0; +} + +static int __maybe_unused ulpi_regs_open(struct inode *inode, struct file *f) +{ + struct ulpi *ulpi = inode->i_private; + + return single_open(f, ulpi_regs_read, ulpi); +} + +static const struct file_operations __maybe_unused ulpi_regs_ops = { + .owner = THIS_MODULE, + .open = ulpi_regs_open, + .release = single_release, + .read = seq_read, + .llseek = seq_lseek +}; + +static struct dentry *ulpi_root = (void *)-EPROBE_DEFER; + static int ulpi_register(struct device *dev, struct ulpi *ulpi) { int ret; + struct dentry *regs; ulpi->dev.parent = dev; /* needed early for ops */ ulpi->dev.bus = &ulpi_bus; @@ -245,16 +301,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ret = ulpi_read_id(ulpi); if (ret) - return ret; + goto err_of; ret = device_register(&ulpi->dev); if (ret) - return ret; + goto err_of; + + if (IS_ENABLED(CONFIG_DEBUG_FS)) { + ulpi->root = debugfs_create_dir(dev_name(dev), ulpi_root); + if (IS_ERR(ulpi->root)) { + ret = PTR_ERR(ulpi->root); + goto err_dev; + } + + regs = debugfs_create_file("regs", 0444, ulpi->root, ulpi, + &ulpi_regs_ops); + if (IS_ERR(regs)) { + ret = PTR_ERR(regs); + goto err_debugfs; + } + } dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n", ulpi->id.vendor, ulpi->id.product); return 0; + +err_debugfs: + debugfs_remove(ulpi->root); +err_dev: + device_unregister(&ulpi->dev); +err_of: + of_node_put(ulpi->dev.of_node); + return ret; } /** @@ -296,8 +375,9 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface); */ void ulpi_unregister_interface(struct ulpi *ulpi) { - of_node_put(ulpi->dev.of_node); + debugfs_remove_recursive(ulpi->root); device_unregister(&ulpi->dev); + of_node_put(ulpi->dev.of_node); } EXPORT_SYMBOL_GPL(ulpi_unregister_interface); @@ -305,13 +385,25 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface); static int __init ulpi_init(void) { - return bus_register(&ulpi_bus); + int ret; + + if (IS_ENABLED(CONFIG_DEBUG_FS)) { + ulpi_root = debugfs_create_dir("ulpi", NULL); + if (IS_ERR(ulpi_root)) + return PTR_ERR(ulpi_root); + } + + ret = bus_register(&ulpi_bus); + if (ret) + debugfs_remove(ulpi_root); + return ret; } subsys_initcall(ulpi_init); static void __exit ulpi_exit(void) { bus_unregister(&ulpi_bus); + debugfs_remove(ulpi_root); } module_exit(ulpi_exit); diff --git a/include/linux/ulpi/driver.h b/include/linux/ulpi/driver.h index c7a1810373e3..083ea2d2e873 100644 --- a/include/linux/ulpi/driver.h +++ b/include/linux/ulpi/driver.h @@ -6,6 +6,7 @@ #include <linux/device.h> +struct dentry; struct ulpi_ops; /** @@ -13,10 +14,12 @@ struct ulpi_ops; * @id: vendor and product ids for ULPI device * @ops: I/O access * @dev: device interface + * @root: root directory for debugfs files */ struct ulpi { struct ulpi_device_id id; const struct ulpi_ops *ops; + struct dentry *root; struct device dev; };
This adds a debugfs file for ULPI devices which contains a dump of their registers. This is useful for debugging basic connectivity problems. The file is created in ulpi_register because many devices will never have a driver bound (as they are managed in hardware by the USB controller device). Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/usb/common/ulpi.c | 100 ++++++++++++++++++++++++++++++++++-- include/linux/ulpi/driver.h | 3 ++ 2 files changed, 99 insertions(+), 4 deletions(-)