Madhavan Srinivasan [off-list ref] writes:

How does dir and name gets used?
It isn't that obvious, so maybe a comment will be gr8.

+static u32 nodeindex;
+static u32 nodalchipindex;
+static u32 coreindexonchip;
+static u32 htmtype;
+static struct dentry *htmdump_debugfs_dir;
+static struct htmdump_entry *ent;
+
+#define BUFFER_SIZE PAGE_SIZE
+
+static ssize_t htmdump_read(struct file *filp, char __user *ubuf,
+			     size_t count, loff_t *ppos)
+{
+	struct htmdump_entry *ent = filp->private_data;
+	unsigned long page, read_size, available;
+	loff_t offset;
+	long rc;
+
+	page = ALIGN_DOWN(*ppos, BUFFER_SIZE);
+	offset = (*ppos) % BUFFER_SIZE;
+
+	rc = htm_get_dump_hardware(nodeindex, nodalchipindex, coreindexonchip,
+				   htmtype, virt_to_phys(ent->buf), BUFFER_SIZE, page);
+
+	switch (rc) {
+	case H_SUCCESS:
+	case H_PARTIAL:
+		break;
+	case H_NOT_AVAILABLE:
+		return 0;

Minor nits for error returns here...

Is returning 0 correct here? Maybe it is (since 0 means no data read),
but wanted to confirm if we should return -ENODATA, or -ENODEV 
(not sure what does H_NOT_AVAILABLE here means)

#define	ENODATA		61	/* No data available */

+	case H_BUSY:
+	case H_LONG_BUSY_ORDER_1_MSEC:
+	case H_LONG_BUSY_ORDER_10_MSEC:
+	case H_LONG_BUSY_ORDER_100_MSEC:
+	case H_LONG_BUSY_ORDER_1_SEC:
+	case H_LONG_BUSY_ORDER_10_SEC:
+	case H_LONG_BUSY_ORDER_100_SEC:

Similarly for above maybe -EBUSY perhaps, instead of -EINVAL

#define	EBUSY		16	/* Device or resource busy */

+	case H_PARAMETER:
+	case H_P2:
+	case H_P3:
+	case H_P4:
+	case H_P5:
+	case H_P6:
+	case H_STATE:
+	case H_AUTHORITY:
+		return -EINVAL;
+	}
+
+	available = BUFFER_SIZE - offset;
+	read_size = min(count, available);
+	*ppos += read_size;
+	return simple_read_from_buffer(ubuf, count, &offset, ent->buf, available);
+}
+
+static const struct file_operations htmdump_fops = {
+	.llseek = default_llseek,
+	.read	= htmdump_read,
+	.open	= simple_open,
+};
+
+static int htmdump_init_debugfs(void)
+{
+	ent = kcalloc(1, sizeof(struct htmdump_entry), GFP_KERNEL);
+	if (!ent) {
+		pr_err("Failed to allocate ent\n");
+		return -EINVAL;

return value can be -ENOMEM;

+	}
+
+	ent->buf = kmalloc(BUFFER_SIZE, GFP_KERNEL);
+	if (!ent->buf) {
+		pr_err("Failed to allocate htmdump buf\n");
+		return -ENOMEM;

kfree(ent)?

+	}
+
+	pr_debug("%s: ent:%lx buf:%lx\n",
+			__func__, (long unsigned int)ent, (long unsigned int)ent->buf);
+

maybe %p perhaps? 


<Documentation/core-api/printk-formats.rst>
Plain Pointers
--------------

::

	%p	abcdef12 or 00000000abcdef12

Pointers printed without a specifier extension (i.e unadorned %p) are
hashed to prevent leaking information about the kernel memory layout. This
has the added benefit of providing a unique identifier.
<...>
...the aim of printing the address is to provide
more information for debugging, use %p and boot the kernel with the
``no_hash_pointers`` parameter during debugging, which will print all %p
addresses unmodified. If you *really* always want the unmodified address, see
%px below.

+	htmdump_debugfs_dir = debugfs_create_dir("htmdump",
+						  arch_debugfs_dir);
+
+	debugfs_create_u32("nodeindex", 0600,
+			htmdump_debugfs_dir, &nodeindex);
+	debugfs_create_u32("nodalchipindex", 0600,
+			htmdump_debugfs_dir, &nodalchipindex);
+	debugfs_create_u32("coreindexonchip", 0600,
+			htmdump_debugfs_dir, &coreindexonchip);
+	debugfs_create_u32("htmtype", 0600,
+			htmdump_debugfs_dir, &htmtype);
+	debugfs_create_file("trace", 0400, htmdump_debugfs_dir, ent, &htmdump_fops);
+
+	return 0;
+}
+
+static int __init htmdump_init(void)
+{
+	if (htmdump_init_debugfs())
+		return -EINVAL;
+
+	return 0;
+}
+
+static void __exit htmdump_exit(void)
+{
+	debugfs_remove_recursive(htmdump_debugfs_dir);
+	kfree(ent->buf);
+	kfree(ent);
+}
+
+module_init(htmdump_init);
+module_exit(htmdump_exit);
+MODULE_DESCRIPTION("PHYP Hardware Trace Macro (HTM) data dumper");
+MODULE_LICENSE("GPL");
-- 
2.45.2