Re: [PATCH 01/11] drm: bridge: Add Samsung DSIM bridge driver

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Jagan,

On 08.04.2022 18:20, Jagan Teki wrote:
> Samsung MIPI DSIM controller is common DSI IP that can be used in various
> SoCs like Exynos, i.MX8M Mini/Nano.
>
> In order to access this DSI controller between various platform SoCs, the
> ideal way to incorporate this in the drm stack is via the drm bridge driver.
>
> This patch is trying to differentiate platform-specific and bridge driver
> code and keep maintaining the exynos_drm_dsi.c code as platform-specific
> glue code and samsung-dsim.c as a common bridge driver code.
>
> - Exynos specific glue code is exynos specific te_irq, host_attach, and
>    detach code along with conventional component_ops.
>
> - Samsung DSIM is a bridge driver which is common across all platforms and
>    the respective platform-specific glue will initialize at the end of the
>    probe. The platform-specific operations and other glue calls will invoke
>    on associate code areas.
>
> Updated MAINTAINERS file for this bridge with exynos drm maintainers along
> with Andrzej as he is the original author.
>
> Tomasz Figa has been not included in MAINTAINERS as he is not available via
> samsung.com.
>
> v1:
> * Don't maintain component_ops in bridge driver
> * Don't maintain platform glue code in bridge driver
> * Add platform-specific glue code and make a common bridge
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

Well, it took me a while to make this working on Exynos. I'm not really 
happy of the design, although I didn't spent much time thinking how to 
improve it and clarify some ambiguities. It doesn't even look that one 
has compiled the Exynos code after this conversion.

The following changes are needed to get it to the same working state as 
before this patch (the next patches however break it even further):

commit e358ee6239305744062713c5aa2e8d44f740b81a (HEAD)
Author: Marek Szyprowski <m.szyprowski@samsung.com>
Date:   Tue Apr 12 11:30:26 2022 +0200

     drm: exynos: dsi: fixup driver after conversion

     Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index ee5d7e5518a6..8e0064282ce6 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -17,7 +17,6 @@

  #include <linux/clk.h>
  #include <linux/delay.h>
-#include <linux/gpio/consumer.h>
  #include <linux/irq.h>
  #include <linux/of_device.h>
  #include <linux/phy/phy.h>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 97167c5ffc78..bbfacb22d99d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -8,6 +8,7 @@
   */

  #include <linux/component.h>
+#include <linux/gpio/consumer.h>

  #include <drm/bridge/samsung-dsim.h>
  #include <drm/drm_probe_helper.h>
@@ -25,17 +26,19 @@ struct exynos_dsi {
         struct samsung_dsim_plat_data pdata;
  };

-static void exynos_dsi_enable_irq(void *priv)
+static void exynos_dsi_enable_irq(struct samsung_dsim *priv)
  {
-       struct exynos_dsi *dsi = priv;
+       const struct samsung_dsim_plat_data *pdata = priv->plat_data;
+       struct exynos_dsi *dsi = container_of(pdata, struct exynos_dsi, 
pdata);

         if (dsi->te_gpio)
                 enable_irq(gpiod_to_irq(dsi->te_gpio));
  }

-static void exynos_dsi_disable_irq(void *priv)
+static void exynos_dsi_disable_irq(struct samsung_dsim *priv)
  {
-       struct exynos_dsi *dsi = priv;
+       const struct samsung_dsim_plat_data *pdata = priv->plat_data;
+       struct exynos_dsi *dsi = container_of(pdata, struct exynos_dsi, 
pdata);

         if (dsi->te_gpio)
                 disable_irq(gpiod_to_irq(dsi->te_gpio));
@@ -92,15 +95,15 @@ static void exynos_dsi_unregister_te_irq(struct 
exynos_dsi *dsi)
         }
  }

-static int exynos_dsi_host_attach(void *priv, struct mipi_dsi_device 
*device)
+static int exynos_dsi_host_attach(struct samsung_dsim *priv, struct 
mipi_dsi_device *device)
  {
-       struct exynos_dsi *dsi = priv;
-       struct samsung_dsim *_priv = dsi->priv;
+       const struct samsung_dsim_plat_data *pdata = priv->plat_data;
+       struct exynos_dsi *dsi = container_of(pdata, struct exynos_dsi, 
pdata);
         struct drm_encoder *encoder = &dsi->encoder;
         struct drm_device *drm = encoder->dev;
         int ret;

-       drm_bridge_attach(encoder, &_priv->bridge, NULL, 0);
+       drm_bridge_attach(encoder, &priv->bridge, NULL, 0);

         /*
          * This is a temporary solution and should be made by more 
generic way.
@@ -116,11 +119,11 @@ static int exynos_dsi_host_attach(void *priv, 
struct mipi_dsi_device *device)

         mutex_lock(&drm->mode_config.mutex);

-       _priv->lanes = device->lanes;
-       _priv->format = device->format;
-       _priv->mode_flags = device->mode_flags;
+       priv->lanes = device->lanes;
+       priv->format = device->format;
+       priv->mode_flags = device->mode_flags;
         exynos_drm_crtc_get_by_type(drm, 
EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
-                       !(_priv->mode_flags & MIPI_DSI_MODE_VIDEO);
+                       !(priv->mode_flags & MIPI_DSI_MODE_VIDEO);

         mutex_unlock(&drm->mode_config.mutex);

@@ -130,9 +133,10 @@ static int exynos_dsi_host_attach(void *priv, 
struct mipi_dsi_device *device)
         return 0;
  }

-static int exynos_dsi_host_detach(void *priv, struct mipi_dsi_device 
*device)
+static int exynos_dsi_host_detach(struct samsung_dsim *priv, struct 
mipi_dsi_device *device)
  {
-       struct exynos_dsi *dsi = priv;
+       const struct samsung_dsim_plat_data *pdata = priv->plat_data;
+       struct exynos_dsi *dsi = container_of(pdata, struct exynos_dsi, 
pdata);
         struct drm_device *drm = dsi->encoder.dev;

         if (drm->mode_config.poll_enabled)
@@ -150,8 +154,9 @@ static const struct samsung_dsim_host_ops 
samsung_dsim_exynos_host_ops = {

  static int exynos_dsi_bind(struct device *dev, struct device *master, 
void *data)
  {
-       struct exynos_dsi *dsi = dev_get_drvdata(dev);
-       struct samsung_dsim *priv = dsi->priv;
+       struct samsung_dsim *priv = dev_get_drvdata(dev);
+       const struct samsung_dsim_plat_data *pdata = priv->plat_data;
+       struct exynos_dsi *dsi = container_of(pdata, struct exynos_dsi, 
pdata);
         struct drm_encoder *encoder = &dsi->encoder;
         struct drm_device *drm_dev = data;
         int ret;
@@ -167,8 +172,7 @@ static int exynos_dsi_bind(struct device *dev, 
struct device *master, void *data

  static void exynos_dsi_unbind(struct device *dev, struct device 
*master, void *data)
  {
-       struct exynos_dsi *dsi = dev_get_drvdata(dev);
-       struct samsung_dsim *priv = dsi->priv;
+       struct samsung_dsim *priv = dev_get_drvdata(dev);

priv->bridge.funcs->atomic_disable(&priv->bridge, NULL);

diff --git a/include/drm/bridge/samsung-dsim.h 
b/include/drm/bridge/samsung-dsim.h
index 59a43f9c4477..9f579a798635 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -41,14 +41,18 @@ struct samsung_dsim_driver_data {
         const unsigned int *reg_values;
  };

+struct samsung_dsim;
+
  struct samsung_dsim_host_ops {
-       int (*attach)(void *priv, struct mipi_dsi_device *device);
-       int (*detach)(void *priv, struct mipi_dsi_device *device);
+       int (*attach)(struct samsung_dsim *priv,
+                     struct mipi_dsi_device *device);
+       int (*detach)(struct samsung_dsim *priv,
+                     struct mipi_dsi_device *device);
  };

  struct samsung_dsim_irq_ops {
-       void (*enable)(void *priv);
-       void (*disable)(void *priv);
+       void (*enable)(struct samsung_dsim *priv);
+       void (*disable)(struct samsung_dsim *priv);
  };

  struct samsung_dsim_plat_data {


> ---
>   MAINTAINERS                             |   12 +
>   drivers/gpu/drm/bridge/Kconfig          |   12 +
>   drivers/gpu/drm/bridge/Makefile         |    1 +
>   drivers/gpu/drm/bridge/samsung-dsim.c   | 1676 ++++++++++++++++++++++
>   drivers/gpu/drm/exynos/Kconfig          |    1 +
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1704 +----------------------
>   include/drm/bridge/samsung-dsim.h       |   95 ++
>   7 files changed, 1852 insertions(+), 1649 deletions(-)
>   create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c
>   create mode 100644 include/drm/bridge/samsung-dsim.h
>
> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel