On 29/06/2026 08:10, Shivani Nittor wrote:

Introduce a DTS-based PMU driver that discovers PMU information
from the device tree and registers a PowerPC PMU instance at
runtime.

The driver parses basic PMU properties such as the number of
counters and MMCR register definitions, creates sysfs event
entries from the device tree event descriptions, and registers
the PMU with the perf subsystem.

To support dynamic PMU registration, add PMU unregistration
support and create a platform device for PMU nodes described
in the device tree.

This forms the foundation for moving PMU descriptions out of
architecture-specific kernel code and into device tree data.

The driver implements the following functionality:

Device Tree Parsing:
- Reads PMU properties including counter count (nr_pmc), PMU name,
  version, and platform information
- Parses MMCR (Monitor Mode Control Register) definitions from the
  sprs/mmcr node hierarchy, storing the MMCR register SPRNs
- Extracts event definitions from the events node, supporting both
  32-bit and 64-bit event codes

Dynamic Event Registration:
- Creates sysfs event entries dynamically from device tree event
  descriptions
- Generates event attributes with format "event=0x<code>" for each
  discovered event
- Exposes events under /sys/devices/<pmu-name>/events/

PMU Registration:
- Registers a power_pmu instance with the perf subsystem using the
  device tree-provided PMU name
- Adds PMU unregistration support to enable proper cleanup
- Creates platform device for "ibm,power-pmu" compatible nodes

Sysfs Interface:
- Provides format attributes (event, pmcsel) under
  /sys/devices/<pmu-name>/format/
- Exposes device tree debug information (nr_pmc) under
  /sys/devices/<pmu-name>/dt/

Where did you document the ABI?

...

+static struct power_pmu dts_pmu = {
+	.name           = "cpu_dts",
+	.n_counter      = MAX_PMU_COUNTERS,
+	.attr_groups    = pmu_dts_attr_groups,
+};
+
+/* Device Tree match */

That's obvious. Can of_device_id be something else than DT match?

+static const struct of_device_id pmu_dts_of_match[] = {
+	{ .compatible = "ibm,power-pmu" },

Where is ABI documented?

+	{ }
+};
+MODULE_DEVICE_TABLE(of, pmu_dts_of_match);
+
+/* Probe function */
+static int pmu_dts_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node, *child, *events_np;
+	struct device_node *sprs_np, *mmcr_np, *mmcr_child;
+	struct pmu_dts_event *evt;
+	u64 code;
+	u32 code32, sprn;
+	u32 code64[2];
+	u32 code128[4];
+	int cells;
+	const char *str;
+
+	pr_info("PMU DTS probe node = %s\n", np->full_name);

This does not look like useful printk message. Drivers should be silent
on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

+
+	if (!of_property_present(np, "nr_pmc")) {
+
+		for_each_child_of_node(np, child) {
+			pr_info("child node = %s\n", child->full_name);

Why are you sending debugging code?

+
+			if (of_property_present(child, "nr_pmc")) {
+				np = child;
+				break;
+			}
+		}
+	}
+
+	if (of_property_read_u32(np, "nr_pmc", &pmu_dts_nr_pmc)) {
+		pr_err("pmu_dts: nr_pmc not found in %s\n", np->full_name);

Why isn't this using dev interface?

+		return -EINVAL;
+	}
+
+	if (!of_property_read_string(np, "pmu-name", &str))
+		pr_info("PMU Name: %s\n", str);
+
+	if (!of_property_read_string(np, "pmu-version", &str))
+		pr_info("PMU Version: %s\n", str);
+
+	if (!of_property_read_string(np, "platform", &str))

No, you cannot just add bunch of undocumented ABI and call it a day.

+		pr_info("Platform: %s\n", str);
+
+	sprs_np = of_get_child_by_name(np, "sprs");
+	if (!sprs_np) {
+		pr_err("pmu_dts: no sprs node\n");
+		return -EINVAL;
+	}
+
+	mmcr_np = of_get_child_by_name(sprs_np, "mmcr");
+	if (!mmcr_np) {
+		pr_err("pmu_dts: no mmcr node\n");
+		return -EINVAL;
+	}
+
+	mmcr_count = 0;
+	for_each_child_of_node(mmcr_np, mmcr_child) {
+
+		if (of_property_read_u32(mmcr_child, "sprn", &sprn))
+			continue;
+
+		mmcr_regs_sprs[mmcr_count++] = sprn;
+		pr_info("pmu_dts: MMCR[%d] = %u (%s)\n", mmcr_count - 1,
+				sprn, mmcr_child->name);
+
+		if (mmcr_count >= MAX_MMCR)
+			break;

Where is cleanup?

+	}
+
+	if (!mmcr_count) {
+		pr_err("pmu_dts: no MMCR SPRs found\n");
+		return -EINVAL;
+	}
+
+	/* Parse events */
+	events_np = of_get_child_by_name(np, "events");
+	if (!events_np) {
+		pr_err("pmu_dts: no events node found\n");
+		return -EINVAL;
+	}
+
+	dts_event_count = 0;
+	for_each_child_of_node(events_np, child) {
+		if (!of_device_is_available(child))

Why are you open-coding available variant of loop?

+			continue;
+
+		cells = of_property_count_u32_elems(child, "event_code");
+		if (cells == 1) {
+			if (of_property_read_u32(child, "event_code", &code32))
+				continue;
+
+			code = code32;
+
+		} else if (cells == 2) {
+			if (of_property_read_u32_array(child, "event_code", code64, 2))
+				continue;
+			code = ((u64)code64[0] << 32) | code64[1];
+
+		} else if (cells == 4) {
+			if (of_property_read_u32_array(child, "event_code", code128, 4))
+				continue;
+			code = ((u64)code128[1] << 32) | code128[3];
+
+		} else {
+			pr_warn("pmu_dts: invalid event_code for %s\n", child->name);
+			continue;
+		}
+
+		evt = kzalloc(sizeof(*evt), GFP_KERNEL);
+		if (!evt)
+			continue;
+
+		snprintf(evt->name, sizeof(evt->name), "%s", child->name);
+		snprintf(evt->config, sizeof(evt->config), "event=0x%llx", code);
+
+		sysfs_attr_init(&evt->attr.attr);
+		evt->attr.attr.name = evt->name;
+		evt->attr.attr.mode = 0444;
+		evt->attr.show = pmu_dts_event_show;
+		dts_events[dts_event_count] = evt;
+		pmu_dts_events_attrs[dts_event_count] = &evt->attr.attr;
+		dts_event_count++;
+
+		if (dts_event_count >= MAX_DTS_EVENTS)
+			break;

And node cleanup?

+	}
+	pmu_dts_events_attrs[dts_event_count] = NULL;
+
+	/* Register PMU */

No.

+	pr_info("pmu_dts: registering PMU\n");

No.

+	return register_power_pmu(&dts_pmu);
+}
+
+/* Platform driver */

This is ridicilous comment.

+static struct platform_driver pmu_dts_driver = {
+	.probe = pmu_dts_probe,
+	.driver = {
+		.name = "pmu_dts",
+		.of_match_table = pmu_dts_of_match,
+	},
+};
+
+static int __init pmu_dts_init(void)
+{
+	pr_info("pmu_dts: init\n");

NAK, init functions do not print anything. RFC does not mean you can
send debugging code with poor quality and...

.... with completely broken indentation style.

+	opal_pmus_init_dev();
+
 	/* Create leds platform devices */
 	leds = of_find_node_by_path("/ibm,opal/leds");
 	if (leds) {

Best regards,
Krzysztof