From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: video: fbdev: omapfb: Add missing error check for clk_get() Date: Wed, 11 Mar 2026 13:28:19 +1000 Message-ID: In-Reply-To: <20260310085625.2648545-1-nichen@iscas.ac.cn> References: <20260310085625.2648545-1-nichen@iscas.ac.cn> <20260310085625.2648545-1-nichen@iscas.ac.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The IS_ERR check is correct and needed.** The `clk_get()` call at line 95= 2 is indeed unchecked, and adding the check is the right thing to do: ```c hwa742.sys_ck =3D clk_get(NULL, "hwa_sys_ck"); + if (IS_ERR(hwa742.sys_ck)) + return PTR_ERR(hwa742.sys_ck); ``` This is fine =E2=80=94 at this point in `hwa742_init()`, no resources have = been acquired yet (the `int_ctrl->init()` and `extif->init()` calls happen = later), so returning directly is correct rather than going through the erro= r unwind labels. **The `clk_put()` in `err1` is problematic.** Looking at the error paths: ```c err1: + clk_put(hwa742.sys_ck); return r; ``` The `err1` label is reached from line 956-957: ```c if ((r =3D hwa742.int_ctrl->init(fbdev, 1, req_vram)) < 0) goto err1; ``` And also falls through from `err2`: ```c err2: hwa742.int_ctrl->cleanup(); err1: + clk_put(hwa742.sys_ck); return r; ``` Since `clk_get()` succeeded if we reach any of these labels (the IS_ERR che= ck returns early), calling `clk_put()` at `err1` is correct. Similarly, `er= r3` and `err4` flow through `err2` =E2=86=92 `err1`, so all error paths wil= l now properly release the clock. This is actually fine. **The `clk_put()` in `hwa742_cleanup()` is correct:** ```c clk_disable_unprepare(hwa742.sys_ck); + clk_put(hwa742.sys_ck); ``` The clock needs to be released on driver cleanup. The ordering (disable/unp= repare first, then put) is correct. **Minor observation:** This driver uses the legacy `clk_get(NULL, ...)` / `= clk_put()` API rather than `devm_clk_get()`. Converting to the managed API = would be a further improvement but is outside the scope of this fix and wou= ld be a separate cleanup patch. **Overall:** The patch is correct and addresses a real bug. The error handl= ing and cleanup are properly structured.=20 Reviewed-by looks appropriate for this patch. --- Generated by Claude Code Patch Reviewer