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@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> > > > Cc: Conor Dooley <conor+dt@xxxxxxxxxx>
> > > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > > ---
> > > > 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
