Re: [PATCH v4 77/78] drm/vc4: drv: Support BCM2711

[Maxime Ripard]

Hi Dave,

On Tue, Jul 28, 2020 at 04:30:16PM +0100, Dave Stevenson wrote:
> > @@ -681,10 +684,14 @@ int vc4_kms_load(struct drm_device *dev)
> >         struct vc4_load_tracker_state *load_state;
> >         int ret;
> >
> > -       /* Start with the load tracker enabled. Can be disabled through the
> > -        * debugfs load_tracker file.
> > -        */
> > -       vc4->load_tracker_enabled = true;
> > +       if (!of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
> 
> Is it better to look up the compatible string, or pass something via
> the .data element of the of_device_id table? Probably down to personal
> preference?

It's pretty much equivalent, so I'm not sure one is arguably better than
the other. However, checking for the compatible can be pretty cumbersome
when you have to do it repeatedly (like we do in the HDMI controller),
and when you don't it a lot, having a structure associated to the
compatible is also fairly cumbersome.

> > +               vc4->load_tracker_available = true;
> > +
> > +               /* Start with the load tracker enabled. Can be
> > +                * disabled through the debugfs load_tracker file.
> > +                */
> > +               vc4->load_tracker_enabled = true;
> > +       }
> >
> >         sema_init(&vc4->async_modeset, 1);
> >
> > @@ -698,8 +705,14 @@ int vc4_kms_load(struct drm_device *dev)
> >                 return ret;
> >         }
> >
> > -       dev->mode_config.max_width = 2048;
> > -       dev->mode_config.max_height = 2048;
> > +       if (of_device_is_compatible(dev->dev->of_node, "brcm,bcm2711-vc5")) {
> 
> We're making the same of_device_is_compatible call twice within
> vc4_kms_load. Set a flag based on it and check that instead?

Good idea, thanks!
Maxime

Attachment: signature.asc
Description: PGP signature