Claude review: accel/amdxdna: Add IOCTL to retrieve realtime NPU power estimate

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Commit message nit**: The line `(Update comment to indicate power and utilization)` between the Reviewed-by tags and the Signed-off-by is unusual. This annotation should go below the `---` separator or in the cover letter, not in the permanent commit message area.

**`AIE2_GET_PMF_NPU_DATA` macro — dead assignment on error** (`aie2_pci.h`):

```c
#define AIE2_GET_PMF_NPU_DATA(field, val)				\
({									\
	struct amd_pmf_npu_metrics _npu_metrics;			\
	int _ret;							\
									\
	_ret = amd_pmf_get_npu_data(&_npu_metrics);			\
	val = _ret ? U32_MAX : _npu_metrics.field;			\
	(_ret);								\
})
```

When `_ret` is non-zero, `val` is set to `U32_MAX`, but the caller immediately returns:

```c
ret = AIE2_GET_PMF_NPU_DATA(npu_power, sensor.input);
if (ret)
    return ret;
```

So the `U32_MAX` assignment is dead code. This is harmless but the macro design is misleading — it suggests the caller might continue with a fallback value.

**`#else` stub — fragile naming convention** (`aie2_pci.h`):

```c
#define SENSOR_DEFAULT_npu_power	U32_MAX
#define AIE2_GET_PMF_NPU_DATA(field, val)				\
({									\
	val = SENSOR_DEFAULT_##field;					\
	(-EOPNOTSUPP);							\
})
```

The token-pasting `SENSOR_DEFAULT_##field` requires a matching `#define` for every field name used. Currently only `npu_power` has one. This is brittle and the whole construct is unnecessary since the macro always returns `-EOPNOTSUPP` and the caller returns immediately on error — the value assigned to `val` is never used. This could be simplified to just:

```c
#define AIE2_GET_PMF_NPU_DATA(field, val) (-EOPNOTSUPP)
```

**`unitm` / `units` inconsistency** (`aie2_pci.c`):

```c
sensor.unitm = -3;
scnprintf(sensor.units, sizeof(sensor.units), "mW");
```

Per the UAPI documentation: `unitm` translates values via `pow(10, unitm) * value`. The PMF struct has `u16 npu_power`. If the raw value is already in milliwatts, then `unitm` should be `0` and `units = "mW"`, or `unitm = -3` and `units = "W"`. Having `unitm = -3` with `units = "mW"` means userspace would compute `pow(10, -3) * value` in milliwatts, yielding microwatts — likely not the intent. Please clarify which is correct.

**Missing `buffer_size` check parity with other handlers**: `aie2_get_clock_metadata()` doesn't validate `buffer_size` before `copy_to_user` — it just copies. Here `aie2_get_sensors()` does validate. Inconsistent, but the validation here is the right thing to do; just noting the discrepancy.

**No `#include <linux/limits.h>` justification**: The patch adds `#include <linux/limits.h>` to `aie2_pci.h`, presumably for `U32_MAX`, but `U32_MAX` is defined in `<linux/limits.h>` only indirectly. It's actually in `<linux/types.h>` or `<linux/kernel.h>` which are likely already pulled in. Minor, but worth verifying the include is actually needed.

---
Generated by Claude Code Patch Reviewer