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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jun 23, 2023 at 08:09:53PM +0200, Geert Uytterhoeven wrote:
> 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.

Indeed, my bad/ devm_drm_of_get_bridge() is better.

> > 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? ;-)

I'd vote for dropping LCDC support from the SH users. Does anyone
*really* need it ? If they do, they should convert the board to DT.

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

-- 
Regards,

Laurent Pinchart