Claude review: drm/msm/dsi: setup RC model properly

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

Patch Review

**Positive aspects:**
- The version-based dispatch logic is sound and mirrors how other drivers (e.g., `intel_vdsc.c`) differentiate between DSC parameter sets.
- Good use of the existing `drm_dsc_setup_rc_params()` API with the appropriate enum values.
- The error messages are improved with version information for debugging.

**Issues:**

1. **Commit message format**: The version format specifier `%x.%x` implies hex formatting, but the subject says "DSC 1.1 or 1.2" using decimal. If the version fields are actually decimal values (1, 1) and (1, 2), then `%d.%d` or `%u.%u` would be more appropriate and less confusing. If they really are stored as hex-encoded BCD, `%x` is fine but the commit message should clarify.

```c
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "Unsupported DSC version: %x.%x\n",
+			      dsc->dsc_version_major, dsc->dsc_version_minor);
```

and:

```c
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters for DSC version: %x.%x\n",
+			      dsc->dsc_version_major, dsc->dsc_version_minor);
```

2. **Uninitialized `ret` on default path**: In the `default` case of the switch, `ret` is set to `-EOPNOTSUPP`, which is fine. However, the subsequent `if (ret)` block will print "could not find DSC RC parameters" which is misleading for an unsupported version — the error from the earlier `dsc_version_major != 1` check ("Unsupported DSC version") would be more appropriate here too. Consider printing a distinct message in the default case, or folding the default into the major version check.

3. **Comment accuracy**: The comment says "the upstream lacks SCR parameters" but looking at the `drm_dsc_helper.h` enum, there is no `DRM_DSC_1_1_SCR` variant — only `DRM_DSC_1_1_PRE_SCR` exists. The comment is useful context but "upstream lacks SCR parameters" is somewhat vague. It would be clearer to say something like "DSC 1.1 SCR RC parameters are not yet defined in drm_dsc_helper".

```c
+		/*
+		 * For DSC1.1. the upstream lacks SCR parameters, the downstream
+		 * parameters are unverified here, we support pre-SCR only.
+		 */
```

4. **Minor**: There's a double period in the comment: `"For DSC1.1."` — should be `"For DSC 1.1,"` (also add a space between "DSC" and "1.1").

5. **Missing Fixes tag?**: The commit message says "we should use the correct [parameters]" which implies the current code is buggy (albeit benignly). If this is intended as a correctness fix rather than a new feature, a `Fixes:` tag pointing to the commit that introduced the current `DRM_DSC_1_1_PRE_SCR` usage would be helpful for backporting decisions — even if the visual impact is nil.

6. **No handling for DSC 1.2 with `bits_per_component` validation**: The Intel driver (`intel_vdsc.c:345-352`) checks `compressed_bpp` and `bits_per_component` before choosing between `DRM_DSC_1_1_PRE_SCR` and `DRM_DSC_1_2_444`. This patch doesn't validate those constraints for DSC 1.2. It's worth considering whether certain `bits_per_component` values might not have RC parameter tables defined, which would cause `drm_dsc_setup_rc_params()` to fail silently or return an error. The existing error path would catch it, but it might be worth a comment noting that the helper validates internally.

Overall this is a clean, low-risk improvement. The main actionable items are the double-period typo and considering whether the `%x` format specifier is intentional.

---
Generated by Claude Code Patch Reviewer