On 1/26/22 8:05 AM, Greg Kroah-Hartman wrote:
> On Mon, Jan 24, 2022 at 12:33:43PM -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).
>>
>> This also modifies the error handling in ulpi_register a bit to ensure
>> that ulpi->dev.of_node is always put.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
>> ---
>>
>> Changes in v2:
>> - Always create debugfs files and ignore errors
>> - Look up dentries dynamically
>>
>> drivers/usb/common/ulpi.c | 71 ++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>> index 4169cf40a03b..87deb514eb78 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 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 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 ulpi_regs_ops = {
>> + .owner = THIS_MODULE,
>> + .open = ulpi_regs_open,
>> + .release = single_release,
>> + .read = seq_read,
>> + .llseek = seq_lseek
>> +};
>> +
>> +#define ULPI_ROOT debugfs_lookup(KBUILD_MODNAME, NULL)
>> +
>> static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>> {
>> int ret;
>> + struct dentry *root;
>>
>> ulpi->dev.parent = dev; /* needed early for ops */
>> ulpi->dev.bus = &ulpi_bus;
>> @@ -251,6 +307,9 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>> if (ret)
>> return ret;
>>
>> + root = debugfs_create_dir(dev_name(dev), ULPI_ROOT);
>> + debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops);
>> +
>> dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
>> ulpi->id.vendor, ulpi->id.product);
>>
>> @@ -296,6 +355,8 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface);
>> */
>> void ulpi_unregister_interface(struct ulpi *ulpi)
>> {
>> + debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev),
>> + ULPI_ROOT));
>> of_node_put(ulpi->dev.of_node);
>> device_unregister(&ulpi->dev);
>> }
>> @@ -305,13 +366,21 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
>>
>> static int __init ulpi_init(void)
>> {
>> - return bus_register(&ulpi_bus);
>> + int ret;
>> + struct dentry *root;
>> +
>> + root = debugfs_create_dir(KBUILD_MODNAME, NULL);
>
> The file could be accesed now, but you don't register the bus until
> after this returns:
>
>> + ret = bus_register(&ulpi_bus);
>> + if (ret)
>> + debugfs_remove(root);
>
> Can you flip the order around please?
This is just registering the holding directory. To ensure that we
don't race like
CPU 1 CPU 2
bus_register()
ulpi_register_interface()
debugfs_lookup() /* whoops, no directory */
debugfs_create_dir()
By registering the directory first, we ensure that devices added
will always have a directory to put their debugfs files under.
--Sean
