Re: [PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Sergei Shtylyov]

On 01/15/2018 11:32 PM, Laurent Pinchart wrote:


The LVDS encoders used to be described in DT as part of the DU. They now
have their own DT node, linked to the DU using the OF graph bindings.
This allows moving internal LVDS encoder support to a separate driver
modelled as a DRM bridge. Backward compatibility is retained as legacy
DT is patched live to move to the new bindings.

Signed-off-by: Laurent Pinchart



[...]


diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 6e02c762a557..06a3fbdd728a
100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c


[...]


   /* 
@@ -74,7 +75,6 @@ static const struct rcar_du_device_info
rzg1_du_r8a7745_info = {
.port = 1,

  >  },

},
-   .num_lvds = 0,
   };
   
   static const struct rcar_du_device_info rcar_du_r8a7779_info = {

@@ -95,14 +95,13 @@ static const struct rcar_du_device_info
rcar_du_r8a7779_info = {
.port = 1,
},
},
-   .num_lvds = 0,
   };
   
   static const struct rcar_du_device_info rcar_du_r8a7790_info = {

.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-   .quirks = RCAR_DU_QUIRK_ALIGN_128B | RCAR_DU_QUIRK_LVDS_LANES,
+   .quirks = RCAR_DU_QUIRK_ALIGN_128B,
.num_crtcs = 3,
.routes = {
/*
@@ -164,7 +163,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7792_info = {
.port = 1,
},
},
-   .num_lvds = 0,
   };
   
   static const struct rcar_du_device_info rcar_du_r8a7794_info = {

@@ -186,7 +184,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7794_info = {
.port = 1,
},
},
-   .num_lvds = 0,


I think you can remove *all* such initializers and the field itself with
them -- otherwise it looks like you're doing a not-quite-related drive-by
clean up...


The OF compatibility code uses the .num_lvds field, that's why I haven't
removed it.


   Ah, I haven't yet reviewed that patch! But then I would leave the 
initializers alone...


MBR, Sergei
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Laurent Pinchart]

Hi Sergei,

On Monday, 15 January 2018 22:25:16 EET Sergei Shtylyov wrote:
> On 01/13/2018 02:14 AM, Laurent Pinchart wrote:
> > The LVDS encoders used to be described in DT as part of the DU. They now
> > have their own DT node, linked to the DU using the OF graph bindings.
> > This allows moving internal LVDS encoder support to a separate driver
> > modelled as a DRM bridge. Backward compatibility is retained as legacy
> > DT is patched live to move to the new bindings.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 6e02c762a557..06a3fbdd728a
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> 
> [...]
> 
> >   /* 
> > @@ -74,7 +75,6 @@ static const struct rcar_du_device_info
> > rzg1_du_r8a7745_info = {
> > .port = 1,
 >  },
> > },
> > -   .num_lvds = 0,
> >   };
> >   
> >   static const struct rcar_du_device_info rcar_du_r8a7779_info = {
> > @@ -95,14 +95,13 @@ static const struct rcar_du_device_info
> > rcar_du_r8a7779_info = {
> > .port = 1,
> > },
> > },
> > -   .num_lvds = 0,
> >   };
> >   
> >   static const struct rcar_du_device_info rcar_du_r8a7790_info = {
> > .gen = 2,
> > .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> >   | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> > -   .quirks = RCAR_DU_QUIRK_ALIGN_128B | RCAR_DU_QUIRK_LVDS_LANES,
> > +   .quirks = RCAR_DU_QUIRK_ALIGN_128B,
> > .num_crtcs = 3,
> > .routes = {
> > /*
> > @@ -164,7 +163,6 @@ static const struct rcar_du_device_info
> > rcar_du_r8a7792_info = {
> > .port = 1,
> > },
> > },
> > -   .num_lvds = 0,
> >   };
> >   
> >   static const struct rcar_du_device_info rcar_du_r8a7794_info = {
> > @@ -186,7 +184,6 @@ static const struct rcar_du_device_info
> > rcar_du_r8a7794_info = {
> > .port = 1,
> > },
> > },
> > -   .num_lvds = 0,
> 
> I think you can remove *all* such initializers and the field itself with
> them -- otherwise it looks like you're doing a not-quite-related drive-by
> clean up...

The OF compatibility code uses the .num_lvds field, that's why I haven't 
removed it.

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Sergei Shtylyov]

Hello!

On 01/13/2018 02:14 AM, Laurent Pinchart wrote:


The LVDS encoders used to be described in DT as part of the DU. They now
have their own DT node, linked to the DU using the OF graph bindings.
This allows moving internal LVDS encoder support to a separate driver
modelled as a DRM bridge. Backward compatibility is retained as legacy
DT is patched live to move to the new bindings.

Signed-off-by: Laurent Pinchart 

[...]

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c 
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 6e02c762a557..06a3fbdd728a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c

[...]

  /* 
-
@@ -74,7 +75,6 @@ static const struct rcar_du_device_info rzg1_du_r8a7745_info 
= {
.port = 1,
},
},
-   .num_lvds = 0,
  };
  
  static const struct rcar_du_device_info rcar_du_r8a7779_info = {

@@ -95,14 +95,13 @@ static const struct rcar_du_device_info 
rcar_du_r8a7779_info = {
.port = 1,
},
},
-   .num_lvds = 0,
  };
  
  static const struct rcar_du_device_info rcar_du_r8a7790_info = {

.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-   .quirks = RCAR_DU_QUIRK_ALIGN_128B | RCAR_DU_QUIRK_LVDS_LANES,
+   .quirks = RCAR_DU_QUIRK_ALIGN_128B,
.num_crtcs = 3,
.routes = {
/*
@@ -164,7 +163,6 @@ static const struct rcar_du_device_info 
rcar_du_r8a7792_info = {
.port = 1,
},
},
-   .num_lvds = 0,
  };
  
  static const struct rcar_du_device_info rcar_du_r8a7794_info = {

@@ -186,7 +184,6 @@ static const struct rcar_du_device_info 
rcar_du_r8a7794_info = {
.port = 1,
},
},
-   .num_lvds = 0,


   I think you can remove *all* such initializers and the field itself with 
them -- otherwise it looks like you're doing a not-quite-related drive-by 
clean up...


[...]

MBR, Sergei
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Laurent Pinchart]

Hi Geert,

On Monday, 15 January 2018 10:30:23 EET Geert Uytterhoeven wrote:
> On Sat, Jan 13, 2018 at 12:14 AM, Laurent Pinchart wrote:
> > The LVDS encoders used to be described in DT as part of the DU. They now
> > have their own DT node, linked to the DU using the OF graph bindings.
> > This allows moving internal LVDS encoder support to a separate driver
> > modelled as a DRM bridge. Backward compatibility is retained as legacy
> > DT is patched live to move to the new bindings.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > 
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > 
> > +static void rcar_lvds_enable(struct drm_bridge *bridge)
> > +{
> > +   struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > +   u32 lvdhcr;
> > +   int ret;
> > +
> > +   WARN_ON(lvds->enabled);
> > +
> > +   ret = clk_prepare_enable(lvds->clock);
> 
> What about starting to use Runtime PM to manage the clock, and thus calling
> pm_runtime_get_sync() here instead?
> That will make the driver future-proof w.r.t. LVDS blocks in power domains.
> 
> You do need a "power-domains" property, though, which may complicate DT
> runtime patching.

And that's why I've left it out for now. It's on my to-do list, but I will do 
so in a separate patch series. Depending on the complexity I might use runtime 
PM for new DTs only and leave the runtime-patched devices trees out.

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Geert Uytterhoeven]

Hi Laurent,

On Sat, Jan 13, 2018 at 12:14 AM, Laurent Pinchart
 wrote:
> The LVDS encoders used to be described in DT as part of the DU. They now
> have their own DT node, linked to the DU using the OF graph bindings.
> This allows moving internal LVDS encoder support to a separate driver
> modelled as a DRM bridge. Backward compatibility is retained as legacy
> DT is patched live to move to the new bindings.
>
> Signed-off-by: Laurent Pinchart 

> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c

> +static void rcar_lvds_enable(struct drm_bridge *bridge)
> +{
> +   struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +   u32 lvdhcr;
> +   int ret;
> +
> +   WARN_ON(lvds->enabled);
> +
> +   ret = clk_prepare_enable(lvds->clock);

What about starting to use Runtime PM to manage the clock, and thus calling
pm_runtime_get_sync() here instead?
That will make the driver future-proof w.r.t. LVDS blocks in power domains.

You do need a "power-domains" property, though, which may complicate DT
runtime patching.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 04/12] drm: rcar-du: Convert LVDS encoder code to bridge driver

[Laurent Pinchart]

The LVDS encoders used to be described in DT as part of the DU. They now
have their own DT node, linked to the DU using the OF graph bindings.
This allows moving internal LVDS encoder support to a separate driver
modelled as a DRM bridge. Backward compatibility is retained as legacy
DT is patched live to move to the new bindings.

Signed-off-by: Laurent Pinchart 
---
Changes since v1:

- Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
- Update to the -lvds compatible string format
---
 drivers/gpu/drm/rcar-du/Kconfig   |   4 +-
 drivers/gpu/drm/rcar-du/Makefile  |   3 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c |  21 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h |   5 -
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 175 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.h |  12 -
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |  14 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c |  93 -
 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h |  24 --
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 264 ---
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h |  64 
 drivers/gpu/drm/rcar-du/rcar_lvds.c   | 546 ++
 12 files changed, 583 insertions(+), 642 deletions(-)
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
 delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_lvds.c

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 3f83352a7313..edde8d4b87a3 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -19,8 +19,8 @@ config DRM_RCAR_DW_HDMI
  Enable support for R-Car Gen3 internal HDMI encoder.
 
 config DRM_RCAR_LVDS
-   bool "R-Car DU LVDS Encoder Support"
-   depends on DRM_RCAR_DU
+   tristate "R-Car DU LVDS Encoder Support"
+   depends on DRM && DRM_BRIDGE && OF
select DRM_PANEL
select OF_FLATTREE
select OF_OVERLAY
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index 8ed569a0f462..247df6a3c81d 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -4,13 +4,12 @@ rcar-du-drm-y := rcar_du_crtc.o \
 rcar_du_encoder.o \
 rcar_du_group.o \
 rcar_du_kms.o \
-rcar_du_lvdscon.o \
 rcar_du_of.o \
 rcar_du_plane.o
 
-rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)+= rcar_du_lvdsenc.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)+= rcar_du_of_lvds.dtb.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
 
 obj-$(CONFIG_DRM_RCAR_DU)  += rcar-du-drm.o
 obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
+obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c 
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 6e02c762a557..06a3fbdd728a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -29,6 +29,7 @@
 
 #include "rcar_du_drv.h"
 #include "rcar_du_kms.h"
+#include "rcar_du_of.h"
 #include "rcar_du_regs.h"
 
 /* 
-
@@ -74,7 +75,6 @@ static const struct rcar_du_device_info rzg1_du_r8a7745_info 
= {
.port = 1,
},
},
-   .num_lvds = 0,
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7779_info = {
@@ -95,14 +95,13 @@ static const struct rcar_du_device_info 
rcar_du_r8a7779_info = {
.port = 1,
},
},
-   .num_lvds = 0,
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7790_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-   .quirks = RCAR_DU_QUIRK_ALIGN_128B | RCAR_DU_QUIRK_LVDS_LANES,
+   .quirks = RCAR_DU_QUIRK_ALIGN_128B,
.num_crtcs = 3,
.routes = {
/*
@@ -164,7 +163,6 @@ static const struct rcar_du_device_info 
rcar_du_r8a7792_info = {
.port = 1,
},
},
-   .num_lvds = 0,
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7794_info = {
@@ -186,7 +184,6 @@ static const struct rcar_du_device_info 
rcar_du_r8a7794_info = {
.port = 1,
},
},
-   .num_lvds = 0,
 };
 
 static const struct rcar_du_device_info rcar_du_r8a7795_info = {
@@ -434,7 +431,19 @@ static struct platform_driver rcar_du_platform_driver = {
},
 };
 
-module_platform_driver(rcar_du_platform_driver);
+static int __init rcar_du_init(void)
+{
+   rcar_du_of_init(rcar_du_of_table);
+
+   return platform_driver_register(_du_platform_driver);
+}