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/hisilicon/hibmc: use clock to look up the PLL value Date: Fri, 13 Mar 2026 14:23:19 +1000 Message-ID: In-Reply-To: <20260312074202.1491504-5-shiyongbang@huawei.com> References: <20260312074202.1491504-1-shiyongbang@huawei.com> <20260312074202.1491504-5-shiyongbang@huawei.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **1. Unused `CLOCK_TOLERANCE` define:** The patch adds `#define CLOCK_TOLER= ANCE 100` but never uses it. The tolerance check in `hibmc_get_best_clock_i= dx()` uses `mode->clock / 100` (1% tolerance) instead. The `CLOCK_TOLERANCE= ` define should either be removed or actually used. **2. Tolerance logic for low clocks:** The expression `diff < mode->clock /= 100` uses integer division. For the 25000 kHz entry, the tolerance is 250 = kHz which is fine. But if a mode somehow had a very low clock, the toleranc= e could round to 0 due to integer division, making the match impossible. No= t a practical problem given the table entries, but `<=3D 0` should be guard= ed (e.g., `mode->clock / 100` should be at least 1).=20 **3. `get_pll_config` now takes non-const mode pointer:** ```c -static void get_pll_config(u64 x, u64 y, u32 *pll1, u32 *pll2) +static void get_pll_config(struct drm_display_mode *mode, u32 *pll1, u32 *= pll2) ``` The `mode` parameter should be `const struct drm_display_mode *mode` since = it's not modified. The `hibmc_get_best_clock_idx` already takes a `const` p= ointer, so this is just a missed annotation on `get_pll_config`. **4. Good refactoring:** Consolidating the table lookup into `hibmc_get_bes= t_clock_idx()` and reusing it in both `hibmc_crtc_mode_valid()` and `get_pl= l_config()` is a nice cleanup that eliminates code duplication. **5. `hibmc_dislay_pll_config` typo:** The struct name `hibmc_dislay_pll_co= nfig` has a typo =E2=80=94 it should be `hibmc_display_pll_config`. This is= pre-existing and not introduced by this patch, but could be cleaned up whi= le touching this code. --- Generated by Claude Code Patch Reviewer