Re: [PATCH 39/39] drm: renesas: shmobile: Add DT support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Sam, Laurent,

On Fri, Jun 23, 2023 at 7:54 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> On Fri, Jun 23, 2023 at 08:50:19PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 22, 2023 at 11:21:51AM +0200, Geert Uytterhoeven wrote:
> > > Add DT support, by:
> > >   1. Creating a panel bridge from DT, and attaching it to the encoder,
> > >   2. Replacing the custom connector with a bridge connector,
> > >   3. Obtaining clock configuration based on the compatible value.
> > >
> > > Note that for now the driver uses a fixed clock configuration selecting
> > > the bus clock, as the current code to select other clock inputs needs
> > > changes to support any other SoCs than SH7724.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > > SH-Mobile AG5 (SH73A0) support is untested.
> > >
> > > Unbind crashes when drm_encoder_cleanup() calls drm_bridge_detach(), as
> > > the bridge (allocated by devm_drm_panel_bridge_add()) has already been
> > > freed by that time.
> > > Should I allocate my encoder with devm_kzalloc(), instead of embedding
> > > it inside struct shmob_drm_device?
> >
> > That shouldn't be needed, if you manage the memory for shmob_drm_device
> > with the DRM managed helpers.

Well, Marek said unbind works fine in drivers/gpu/drm/mxsfb/lcdif_drv.c,
where the order is:

    bridge = devm_drm_of_get_bridge(...)
    encoder = devm_kzalloc(...)
    drm_encoder_init(...)

> > Lifetime management of bridges is currently completely broken, there's
> > nothing that prevents bridges from being freed while still in use.
> > That's an issue in DRM, not in your driver.

OK ;-) (or :-(

> > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > @@ -508,9 +508,43 @@ static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > >     .mode_fixup = shmob_drm_encoder_mode_fixup,
> > >  };
> > >
> > > +/* -----------------------------------------------------------------------------
> > > + * Encoder
> > > + */
> > > +
> > > +static int shmob_drm_encoder_init(struct shmob_drm_device *sdev,
> > > +                             struct device_node *enc_node)
> > > +{
> > > +   struct drm_bridge *bridge;
> > > +   struct drm_panel *panel;
> > > +   int ret;
> > > +
> > > +   /* Create a panel bridge */
> > > +   panel = of_drm_find_panel(enc_node);
> >
> > Using drm_of_find_panel_or_bridge() would allow supporting platforms
> > that connect a non-panel device to the SoC, in additional to the already
> > supported panels.
>
> From the documentation of drm_of_find_panel_or_bridge():
>
>  * This function is deprecated and should not be used in new drivers. Use
>  * devm_drm_of_get_bridge() instead.
>
> I suggest to go that route.

OK (do I have the feeling that these helpers are sometimes deprecated
faster than they are written? ;-)

> > > @@ -147,11 +148,13 @@ static int shmob_drm_remove(struct platform_device *pdev)
> > >  static int shmob_drm_probe(struct platform_device *pdev)
> > >  {
> > >     struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> >
> > How about dropping non-DT support ? That would simplify the driver.
>
> +1 for that, without knowing the implications.

That depends on your priorities: do you want to migrate all users of
sh_mobile_lcdc_fb to shmob_drm, or do you want the SuperH users to
stick with sh_mobile_lcdc_fb until they have migrated to DT? ;-)

Regardless of the above, I do not have (visible) access to any of the
affected SH772[234] platforms...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds