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/hibmc: Do not use cpp from struct drm_format_info Date: Thu, 23 Apr 2026 10:03:23 +1000 Message-ID: In-Reply-To: <20260420121130.200133-4-tzimmermann@suse.de> References: <20260420121130.200133-1-tzimmermann@suse.de> <20260420121130.200133-4-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Status: Good, one minor observation** Two changes: 1. Replaces `width * cpp[0]` with `drm_format_info_min_pitch()`: ```c reg =3D drm_format_info_min_pitch(fb->format, 0, fb->width); ``` This is the preferred API for computing line widths and handles multi-plane= formats correctly. 2. Replaces the `cpp[0] * 8 / 16` formula with an explicit switch on the fo= urcc: ```c switch (fb->format->format) { case DRM_FORMAT_XRGB8888: reg |=3D HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 2); break; case DRM_FORMAT_RGB565: reg |=3D HIBMC_FIELD(HIBMC_CRT_DISP_CTL_FORMAT, 1); break; } ``` The magic values 2 and 1 match the old formula's results (`4*8/16=3D2`, `2*= 8/16=3D1`) and are presumably the hardware register encoding. This is clean= er and eliminates the problematic dependency on cpp. **Minor observation**: The switch has no `default` case. Since patch 2 rest= ricts formats to only XRGB8888 and RGB565, an unhandled format reaching thi= s point would be a framework bug. However, a `default: WARN_ONCE(...)` or `= default: break;` could be added for defensive programming. Not blocking =E2= =80=94 the mask is cleared before the switch, so the worst case is the form= at field being left at 0, which may represent an undefined or 8bpp mode. Gi= ven that `atomic_check` validates the format list, this is fine in practice. There's a minor inconsistency where `line_l =3D new_state->fb->pitches[0]` = still uses `new_state->fb` instead of the newly-introduced `fb` local varia= ble, but patch 4 cleans this up. --- Generated by Claude Code Patch Reviewer