Claude review: drm/rockchip: vop2: Add clock rate mode check

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

Patch Review

**Positive aspects:**
- Straightforward approach using the existing `mode_valid` callback pattern, consistent with the existing `max_output.width` check just above.
- Naming (`max_pixel_clock_rate`) is clear and distinguishes from the VOP input clock.
- Values for rk3568 (600/200/150 MHz) and rk3588 VP3 (150 MHz) look reasonable and straightforward.

**Issues:**

1. **rk3576 VP0 multiplier inconsistency (medium concern):** The rk3576 VP0 sets `max_pixel_clock_rate = 600000000U * 4` (2.4 GHz effective), but its `pixel_rate` is set to `2` (line 785), not 4. The rk3576 `set_intf_mux` at line 1496 divides by `port_pix_rate` (which is 2), and when `crtc_clock > 600000` it applies `dclk_div = 2`. This suggests the hardware does 2 pixels per cycle, not 4, giving a max effective pixel clock of 600 MHz * 2 = 1.2 GHz. The `* 4` multiplier may be too permissive for rk3576, allowing modes the hardware cannot actually drive. This should be confirmed with Andy Yan — it might need to be `* 2` instead.

2. **rk3588 VP0-VP2 have no `pixel_rate` set (informational):** The rk3588 video ports VP0-VP2 get `max_pixel_clock_rate = 600000000U * 4` but don't set the `pixel_rate` field (defaulting to 0). The rk3588 code path uses a separate `rk3588_calc_cru_cfg()` function that hardcodes `dclk_core_rate = v_pixclk >> 2` rather than reading `pixel_rate`, so this is correct behavior — but it's worth noting the asymmetry with rk3576 for future maintainability.

3. **No guard against `max_pixel_clock_rate == 0` (minor):** If a future SoC variant is added and the developer forgets to set `max_pixel_clock_rate`, the field defaults to 0, and the check:
   ```c
   if (mode->clock > vp->data->max_pixel_clock_rate / 1000)
       return MODE_CLOCK_HIGH;
   ```
   would reject **all** modes (since `mode->clock` is always > 0). This is a fail-closed behavior which is arguably safe (you'd notice immediately), but a defensive guard like:
   ```c
   if (vp->data->max_pixel_clock_rate &&
       mode->clock > vp->data->max_pixel_clock_rate / 1000)
   ```
   would be more forgiving during bringup of new SoCs. Alternatively, a `WARN_ON(!vp->data->max_pixel_clock_rate)` during probe would catch the missing initialization early.

4. **Integer arithmetic precision (nit):** The comparison `mode->clock > vp->data->max_pixel_clock_rate / 1000` truncates via integer division. For the values used here (all multiples of 1,000,000), this is fine. But if a future value isn't evenly divisible by 1000, the truncation would make the limit slightly stricter than intended. Not an issue with current values.

5. **Commit message clarity (nit):** The commit message says "up to 4 pixels in parallel when needed resulting in the extra multiplier" but doesn't explain *which* specific ports get the multiplier and why. Since this is the core question the RFC is asking about, it would help reviewers to have a table in the commit message mapping each SoC/VP to its base clock, multiplier, and effective max pixel clock.

---
Generated by Claude Code Patch Reviewer