From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-03.galae.net (smtpout-03.galae.net [185.246.85.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 925E83BA233 for ; Mon, 9 Mar 2026 12:47:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.85.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773060457; cv=none; b=tBxwzOs3nXJnuAddILKf3h2iUmRPQqQwHGNEEVHUfw+S4C/obToG7gZ5xLJxoZCYoqAz+K/IzMbF/+r5ufzZQrhZ3PPHBWB/hO6xt+dY0o9uWLLlC0DpcPH4KMCpxTjFPLWLnoaNMY+jJmIvfwR5Xov6ByFOpQXglP5g+o1QdFw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773060457; c=relaxed/simple; bh=nEAiOJiRoBRADi1pbzdHOoZOqFIoLJRvwG35ofZSO8s=; h=Mime-Version:Content-Type:Date:Message-Id:Subject:Cc:To:From: References:In-Reply-To; b=TmbyxLXs4qE/AEhXc1fEADtWRSVtV+LdnPp4HcVAGNC/TRai5XFuxghrTlMY395sALiYO4twmsfdJmUX9dQ7iAMidG/DRW3VFWNldIs37XDlWK++A+CDL4dXNIcSY0J7ZmMiUN8yX9oRjRXwHpIaPkN8J8EJvYMNjP+qAm/eO1Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=el/ScjM4; arc=none smtp.client-ip=185.246.85.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="el/ScjM4" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-03.galae.net (Postfix) with ESMTPS id 0316A4E425DC; Mon, 9 Mar 2026 12:47:33 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id C9B875FFB8; Mon, 9 Mar 2026 12:47:32 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 09B6810369B97; Mon, 9 Mar 2026 13:47:19 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1773060451; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=wCTUnhuxvulHzW4p0uUdfASMjS854LLOHF+KryEvCLg=; b=el/ScjM4Ue552Q5u0YpglfOiNwlnWqvRvgPZRn0f2Rw9gC2K6miO3fjP58Kezv524ydiSu TnJe8U2jt+A1EJ6DB1yTs7y0A6UQYZvTUFLzohffbj2jx9dbDYRqYRIm/DKsAlA08WUE5o h+sY8NUKOa8F7OPKU7GdMADmAbHvAOWT3Xycjjt/+QEJcuQ8UijP4u56/6n693iG274Qyw TW2fwFCVxVMyU/LU5PH1DEo0plMmeu9xMuFSnZIDm/fLs/dYFW8oWIDiOD5qWXRnYonHxk qlKDmbIRISO2L4epJFOCo80mqRQNmcnKOiRv5ruerLteKpajtRCpOmq8Cv4YXQ== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 09 Mar 2026 13:47:18 +0100 Message-Id: Subject: Re: [PATCH v7 3/8] drm: verisilicon: add a driver for Verisilicon display controllers Cc: "Philipp Zabel" , "Dmitry Baryshkov" , "Michal Wilczynski" , "Han Gao" , "Yao Zi" , , , , , "Icenowy Zheng" To: "Icenowy Zheng" , "Andrzej Hajda" , "Neil Armstrong" , "Robert Foss" , "Laurent Pinchart" , "Jonas Karlman" , "Jernej Skrabec" , "Maarten Lankhorst" , "Maxime Ripard" , "Thomas Zimmermann" , "David Airlie" , "Simona Vetter" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Drew Fustini" , "Guo Ren" , "Fu Wei" From: "Luca Ceresoli" X-Mailer: aerc 0.20.1 References: <20260129023922.1527729-1-zhengxingda@iscas.ac.cn> <20260129023922.1527729-4-zhengxingda@iscas.ac.cn> In-Reply-To: <20260129023922.1527729-4-zhengxingda@iscas.ac.cn> X-Last-TLS-Session-Version: TLSv1.3 Hello Icenowy Zheng, On Thu Jan 29, 2026 at 3:39 AM CET, Icenowy Zheng wrote: > From: Icenowy Zheng > > This is a from-scratch driver targeting Verisilicon DC-series display > controllers, which feature self-identification functionality like their > GC-series GPUs. > > Only DC8200 is being supported now, and only the main framebuffer is set > up (as the DRM primary plane). Support for more DC models and more > features is my further targets. > > As the display controller is delivered to SoC vendors as a whole part, > this driver does not use component framework and extra bridges inside a > SoC is expected to be implemented as dedicated bridges (this driver > properly supports bridge chaining). > > Signed-off-by: Icenowy Zheng > Signed-off-by: Icenowy Zheng > Tested-by: Han Gao > Tested-by: Michal Wilczynski > Reviewed-by: Thomas Zimmermann I have reviewed the bridge part of this patch and have a few remarks, see below. [...] > +++ b/drivers/gpu/drm/verisilicon/vs_bridge.c > @@ -0,0 +1,371 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2025 Icenowy Zheng > + */ > + > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "vs_bridge.h" > +#include "vs_bridge_regs.h" > +#include "vs_crtc.h" > +#include "vs_dc.h" > + > +static int vs_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder, > + enum drm_bridge_attach_flags flags) > +{ > + struct vs_bridge *vbridge =3D drm_bridge_to_vs_bridge(bridge); > + > + return drm_bridge_attach(encoder, vbridge->next_bridge, > + bridge, flags); > +} > + > +struct vsdc_dp_format { > + u32 linux_fmt; > + bool is_yuv; > + u32 vsdc_fmt; > +}; Moving the bool after the two 'u32's would be better for packing and spatial locality (especially in case more fields are added in the future). > + > +static struct vsdc_dp_format vsdc_dp_supported_fmts[] =3D { > + /* default to RGB888 */ > + { MEDIA_BUS_FMT_FIXED, false, VSDC_DISP_DP_CONFIG_FMT_RGB888 }, > + { MEDIA_BUS_FMT_RGB888_1X24, false, VSDC_DISP_DP_CONFIG_FMT_RGB888 }, > + { MEDIA_BUS_FMT_RGB565_1X16, false, VSDC_DISP_DP_CONFIG_FMT_RGB565 }, > + { MEDIA_BUS_FMT_RGB666_1X18, false, VSDC_DISP_DP_CONFIG_FMT_RGB666 }, > + { MEDIA_BUS_FMT_RGB101010_1X30, > + false, VSDC_DISP_DP_CONFIG_FMT_RGB101010 }, You can put up to 100 chars per line and avoid the newline here to make this table more readable. Same below. > + { MEDIA_BUS_FMT_UYVY8_1X16, true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY8 }, > + { MEDIA_BUS_FMT_UYVY10_1X20, true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY10 }= , > + { MEDIA_BUS_FMT_YUV8_1X24, true, VSDC_DISP_DP_CONFIG_YUV_FMT_YUV8 }, > + { MEDIA_BUS_FMT_YUV10_1X30, true, VSDC_DISP_DP_CONFIG_YUV_FMT_YUV10 }, > + { MEDIA_BUS_FMT_UYYVYY8_0_5X24, > + true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY8 }, > + { MEDIA_BUS_FMT_UYYVYY10_0_5X30, > + true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY10 }, > +}; > + [...] > +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev, > + struct vs_crtc *crtc) > +{ > + unsigned int output =3D crtc->id; > + struct vs_bridge *bridge; In common practice a variable named 'bridge' is used to point to a 'struct drm_bridge', so it feels weird when it is used for another type. Can you rename to 'vbridge' or 'vsbridge' or similar, to clarify it's the "Verisilicon bridge"? This is after all what you did in vs_bridge_attach() above, where the ambiguity of the 'bridge' name used for a driver-specific struct is evident. > + struct drm_bridge *next; > + enum vs_bridge_output_interface intf; > + const struct drm_bridge_funcs *bridge_funcs; > + int ret, enctype; > + > + intf =3D vs_bridge_detect_output_interface(drm_dev->dev->of_node, > + output); > + if (intf =3D=3D -ENODEV) { > + drm_dbg(drm_dev, "Skipping output %u\n", output); > + return NULL; > + } > + > + next =3D devm_drm_of_get_bridge(drm_dev->dev, drm_dev->dev->of_node, > + output, intf); > + if (IS_ERR(next)) { > + ret =3D PTR_ERR(next); > + if (ret !=3D -EPROBE_DEFER) > + drm_err(drm_dev, > + "Cannot get downstream bridge of output %u\n", > + output); 100 chars per line are allowed, so this could fit on a single line being nicer to read. This applies to a lot places in this driver, of logging calls in particular. I understand this would be annoying to change on an already reviewed patch and at v7 so up to you, but it would be good to keep it in mind for the future. > + return ERR_PTR(ret); > + } > + > + if (intf =3D=3D VSDC_OUTPUT_INTERFACE_DPI) > + bridge_funcs =3D &vs_dpi_bridge_funcs; > + else > + bridge_funcs =3D &vs_dp_bridge_funcs; > + > + bridge =3D devm_drm_bridge_alloc(drm_dev->dev, struct vs_bridge, base, > + bridge_funcs); The 'struct drm_bridge' field embedded in a driver-specific struct is conventionally called 'bridge', so renaming it from 'base' to 'bridge' would make it more consistent with other drivers. That would go in sync with the coding convention I mentioned above: 'bridge' for struct drm_bridge, bridge or just for a custom driver struct embedding a bridge. > + if (IS_ERR(bridge)) > + return ERR_PTR(PTR_ERR(bridge)); > + > + bridge->crtc =3D crtc; > + bridge->intf =3D intf; > + bridge->next_bridge =3D next; There is now a next_bridge field in struct drm_bridge, which handles the bridge lifetime in a safer way and more simply [0], so you could use it: bridge->base.next_bridge =3D next; Or, after the renames I suggested above: vbridge->bridge.next_bridge =3D next; [0] https://elixir.bootlin.com/linux/v7.0-rc2/source/include/drm/drm_bridge= .h#L1269-L1278 Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com