Claude review: accel/ivpu: Add support for limiting NPU frequency

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

Patch Review

**Bug: Duplicate function declaration in header**

The header `ivpu_jsm_msg.h` has the declaration duplicated:

```c
+int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 pn_ratio, u16 max_ratio);
+int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 pn_ratio, u16 max_ratio);
```

This will generate a compiler warning. Remove the duplicate line.

**Bug: Type mismatch between declaration and definition**

The header declares:
```c
int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 pn_ratio, u16 max_ratio);
```

But the callers and the struct fields (`cfg_min_ratio`, `cfg_max_ratio`) use `u8` for ratios, while the JSM payload struct uses `u32` fields. The function implementation in `ivpu_jsm_msg.c` takes `u16` parameters which are assigned to `u32` fields — this works but is inconsistent. Since the PLL ratios are stored as `u8` in `ivpu_hw_info` and the JSM payload uses `u32`, the function signature should probably use `u8` to match the callers (or at minimum be consistent with one width).

**Issue: `dpu_mhz_to_pll_ratio_lnl` only works for LNL, used unconditionally**

The reverse conversion function:
```c
+static u8 dpu_mhz_to_pll_ratio_lnl(u32 freq_mhz)
+{
+	return clamp_t(u32, freq_mhz / (PLL_REF_CLK_FREQ_MHZ / 2), 0, U8_MAX);
+}
```

This is named `_lnl` and uses the LNL formula (`freq = ratio * 50 / 2`, so `ratio = freq / 25`), but `ivpu_hw_btrs_cfg_max_freq_set()` and `ivpu_hw_btrs_cfg_min_freq_set()` call it unconditionally without checking the hardware generation. Since `set_min_freq`/`set_max_freq` are only exposed for `IVPU_HW_IP_50XX` and newer (which are LNL+), this is probably correct in practice, but it would be cleaner to either:
- Add a comment explaining this is LNL-only by design, or
- Add a platform-dispatching wrapper matching `pll_ratio_to_mhz`

**Issue: No locking around cfg_min_ratio / cfg_max_ratio updates**

In `ivpu_hw_btrs_cfg_freq_set()`, the values are updated after the JSM call succeeds:
```c
+	vdev->hw->pll.cfg_min_ratio = cfg_min_ratio;
+	vdev->hw->pll.cfg_max_ratio = cfg_max_ratio;
```

These can be written concurrently from two sysfs store calls (one to `set_min_freq`, one to `set_max_freq`). The read of `cfg_min_ratio` / `cfg_max_ratio` in `ivpu_hw_btrs_cfg_min_freq_set` / `ivpu_hw_btrs_cfg_max_freq_set` is also unprotected. A mutex should guard these read-modify-write sequences, or at minimum a comment explaining why races are acceptable.

**Issue: Stale comment in set_max_freq_store**

```c
+	/* Convert MHz to Hz and set max frequency */
+	ret = ivpu_hw_btrs_cfg_max_freq_set(vdev, freq_mhz);
```

The comment says "Convert MHz to Hz" but the function takes and works with MHz directly. This is a copy-paste leftover.

**Minor: Missing `const` on attribute group**

```c
+static struct attribute_group ivpu_freq_attr_group = {
```

Should be `static const struct attribute_group` to match kernel conventions and allow the compiler to place it in read-only data.

**Design observation: sysfs_add_file_to_group for set_min/set_max**

The conditional addition of `set_min_freq` and `set_max_freq` using `sysfs_add_file_to_group()` after creating the group is functional but differs from the typical kernel pattern of using `is_visible` callbacks in the attribute group. Using `.is_visible` would be more idiomatic and would also handle cleanup automatically via `devm_device_add_group`. The current approach also doesn't have matching removal — `sysfs_add_file_to_group` files won't be removed by the `devm_device_add_group` cleanup of the parent group.

**Observation: `ivpu_hw_btrs_cfg_freq_init` placement**

The call to `ivpu_hw_btrs_cfg_freq_init()` is inside `if (!ivpu_fw_is_warm_boot(vdev))` which means frequency config is only sent on cold boot. This is probably intentional (firmware remembers across warm boots), but worth confirming — if the firmware doesn't preserve frequency config across warm boots, this would silently lose user-configured limits.

**Minor: `PLL_REF_CLK_FREQ_MHZ / 2` integer division**

In `dpu_mhz_to_pll_ratio_lnl`:
```c
+	return clamp_t(u32, freq_mhz / (PLL_REF_CLK_FREQ_MHZ / 2), 0, U8_MAX);
```

`PLL_REF_CLK_FREQ_MHZ` is 50, so `50 / 2 = 25`. This works fine, but the formula isn't the exact inverse of `pll_ratio_to_mhz_lnl` which is `(ratio * 50) / 2`. For `ratio * 50 / 2 → freq`, the inverse should be `freq * 2 / 50` (i.e., `freq / 25`). Both produce the same result here, but `freq_mhz * 2 / PLL_REF_CLK_FREQ_MHZ` would be more clearly the algebraic inverse.

**Summary**: The critical fixes needed are the duplicate declaration in the header and the type consistency for the JSM function signature. The locking concern around `cfg_min_ratio`/`cfg_max_ratio` should also be addressed or explicitly justified. The rest are minor improvements.

---
Generated by Claude Code Patch Reviewer