From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/colorop: Add DRM_COLOROP_FIXED_MATRIX Date: Sun, 12 Apr 2026 13:07:22 +1000 Message-ID: In-Reply-To: <20260408051514.608781-2-chaitanya.kumar.borah@intel.com> References: <20260408051514.608781-1-chaitanya.kumar.borah@intel.com> <20260408051514.608781-2-chaitanya.kumar.borah@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Overall**: Clean UAPI addition. The new colorop type and its initialization follow existing patterns well. **Issue (minor)**: Kernel-doc tag mismatch. The kdoc comment says `@DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB` but the actual enum value is `DRM_COLOROP_FM_YCBCR2020_FULL_RGB_NC`: ```c /** * @DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB: // <-- wrong ... */ DRM_COLOROP_FM_YCBCR2020_FULL_RGB_NC, // <-- actual name ``` This will generate a kernel-doc warning. The `@` tag must exactly match the symbol name. **Observation**: The `supported_fm` parameter is `u64` which supports up to 64 fixed matrix types, but `DRM_COLOROP_FM_COUNT` is currently 6. The overflow check `(supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0` is correct. **Question for discussion**: `DRM_COLOROP_FM_YCBCR_LIMITED_FULL` represents range correction, not a color space conversion matrix. Mixing these two concepts in one enum is workable but may be confusing to userspace. Consider whether this warrants a separate colorop type (though the current approach is pragmatic for hardware that implements both as fixed-function blocks). --- Generated by Claude Code Patch Reviewer