Hi Sam, Laurent,
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.
>
> 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@xxxxxxxxxxxxxx
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
