From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: fbdev: Internalize fbcon Date: Mon, 25 May 2026 21:30:31 +1000 Message-ID: In-Reply-To: <20260520160744.130841-1-tzimmermann@suse.de> References: <20260520160744.130841-1-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: fbdev: Internalize fbcon Author: Thomas Zimmermann Patches: 9 Reviewed: 2026-05-25T21:30:31.171741 --- This is a well-structured 4-patch series by Thomas Zimmermann that internalizes fbcon as a private implementation detail of fbdev. The goal is clean: wrap the fbcon interactions (mode changes, blanking, VGA switcheroo remapping) behind fbdev helper functions, then move the declarations from the public `include/linux/fbcon.h` into the private `drivers/video/fbdev/core/fbcon.h`. The series is generally sound and the direction is correct. The helpers consolidate duplicated fbcon call patterns and fix real inconsistencies (e.g., sysfs mode changes not calling `fbcon_modechange_possible`). However, there are two issues worth addressing: 1. **Bug**: Patch 3 uses `#if CONFIG_FB` which, while technically functional for a bool Kconfig symbol, is non-standard kernel style and should be `#ifdef CONFIG_FB`. 2. **Behavioral change in sh_mobile_lcdcfb**: Patch 1 silently changes `fbcon_update_vcs(info, true)` to `fbcon_update_vcs(info, var->activate & FB_ACTIVATE_ALL)` where `var.activate = FB_ACTIVATE_NOW`, making the `all` parameter false instead of true. This may be intentional (consistent with the ioctl path) but should be called out in the commit message. --- --- Generated by Claude Code Patch Reviewer