Re: [PATCH v7 3/8] drm: verisilicon: add a driver for Verisilicon display controllers

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Icenowy,

On Mon Mar 9, 2026 at 5:35 PM CET, Icenowy Zheng wrote:

>> > +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).
>
> Yes this seems to sound right, but doing such rework sounds quite big
> and unnecessary after it's applied...

Ugh, apologies, I sent my reply without noticing this patch had been
applied already. Anyway, as Thomas pointed out, you can still do this
change as a separate patch ifi you want. Or just not do anything, this can
be done later in case new fields are added. Without new fields the
reordering would not produce much benefit anyway.

>> > +static struct vsdc_dp_format vsdc_dp_supported_fmts[] = {
>> > +	/* 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.
>
> Ah I prefer to keep 80 CPL when I can, and the `coding-style.rst`
> document still suggests 80.

coding-style.rst seems very vague  about that, but checkpatch is clear:

  my $max_line_length = 100;

(https://elixir.bootlin.com/linux/v6.19.6/source/scripts/checkpatch.pl#L60)

Anyway, non need to send a patch now that it's committed for this,
definitely. Again, sorry for the noise on an already applied patch.

>> > +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev,
>> > +				 struct vs_crtc *crtc)
>> > +{
>> > +	unsigned int output = 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 sounds right.
>
> BTW where is such kind of common practice documented?

As it often happens, in the code, I'm afraid.

I've been touching a lot of DRM bridge and related code recently as part of
the bridge hotplug work I'm doing, and I gathered an overall view of the
current common practice as well as which conventions are more readable than
others.

>> > +		return ERR_PTR(ret);
>> > +	}
>> > +
>> > +	if (intf == VSDC_OUTPUT_INTERFACE_DPI)
>> > +		bridge_funcs = &vs_dpi_bridge_funcs;
>> > +	else
>> > +		bridge_funcs = &vs_dp_bridge_funcs;
>> > +
>> > +	bridge = 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, <XYZ>bridge or just <XYZZ> for a custom driver struct
>> embedding
>> a bridge.
>
> Ah, all subclasses in this driver call the base class `base`, and I
> still wonder how such convention is documented.

It's a gain the common practice in the code:

$ git grep -h -E '[[:space:]]struct drm_bridge \*[[:alnum:]_]*;' -- drivers/gpu/drm/ | \
  sed 's/^[[:space:]]*//' | grep -v panel_bridge | sort | uniq -c | sort -nr
     92 struct drm_bridge *bridge;
     27 struct drm_bridge *next_bridge;
      3 struct drm_bridge *companion;
      2 struct drm_bridge *next;
      2 struct drm_bridge *iter;
      1 struct drm_bridge *tmp_bridge;
      1 struct drm_bridge *out_bridge;
      1 struct drm_bridge *ext_bridge;
      1 struct drm_bridge *drm_bridge;
[...]

So 'bridge' is by far the most common, and 'base' does never appear.

>> 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:
>
> Glad to hear such a field exists now. Will more code about next_bridge
> lifetime management being shared?

Not sure exactly what you mean here. If you are asking how next_bridge
works: it's very simple, all being in the declaration [0] (where I
explained it to the best I could in the comment) and its usage in the
destruction callback when the last reference is put [1].

[0] https://elixir.bootlin.com/linux/v7.0-rc3/source/include/drm/drm_bridge.h
[1] https://elixir.bootlin.com/linux/v7.0-rc3/source/drivers/gpu/drm/drm_bridge.c#L267

Best regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com