Re: [PATCH v9 02/10] drm/mediatek: add *driver_data for different hardware settings

[Daniel Kurtz]

Hi YT,

I don't see a reason to handle device_data in such a generic way at
the generic mtk_ddp_comp layer.
The device data is very component specific, so just define different
structs for different comp types, ie:

struct mtk_disp_ovl_driver_data {
    unsigned int reg_ovl_addr;
    unsigned int fmt_rgb565;
    unsigned int fmt_rgb888;
};

struct mtk_disp_rdma_driver_data {
    unsigned int fifo_pseudo_size;
};

struct mtk_disp_color_driver_data {
    unsigned int color_offset;
};

Then add typed pointers to the local structs that use them, for example:

struct mtk_disp_ovl {
        struct mtk_ddp_comp             ddp_comp;
        struct drm_crtc                 *crtc;
        const struct mtk_disp_ovl_driver_data *data;
};

And fetch the device specific driver data directly in .probe, as you
are already doing:

static int mtk_disp_ovl_probe(struct platform_device *pdev) {
  ...
  priv->data = of_device_get_match_data(dev);
  ...
}

More comments in-line...

On Fri, Nov 11, 2016 at 7:55 PM, YT Shen <yt.shen@xxxxxxxxxxxx> wrote:
> There are some hardware settings changed, between MT8173 & MT2701:
> DISP_OVL address offset changed, color format definition changed.
> DISP_RDMA fifo size changed.
> DISP_COLOR offset changed.
> MIPI_TX pll setting changed.
> And add prefix for mtk_ddp_main & mtk_ddp_ext & mutex_mod.

Nit: I think it would make sense to combine this patch with
drm/mediatek: rename macros, add chip prefix

>
> Signed-off-by: YT Shen <yt.shen@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c     | 27 ++++++++++++++++-----------
>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c    | 11 +++++++++--
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c      | 11 +++++++----
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 +++++++++++++++++++++------
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c      | 25 ++++++++++++++++++-------
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h      |  8 ++++++++
>  drivers/gpu/drm/mediatek/mtk_mipi_tx.c      | 24 +++++++++++++++++++++++-
>  8 files changed, 115 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 019b7ca..1139834 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -35,13 +35,10 @@
>  #define DISP_REG_OVL_PITCH(n)                  (0x0044 + 0x20 * (n))
>  #define DISP_REG_OVL_RDMA_CTRL(n)              (0x00c0 + 0x20 * (n))
>  #define DISP_REG_OVL_RDMA_GMC(n)               (0x00c8 + 0x20 * (n))
> -#define DISP_REG_OVL_ADDR(n)                   (0x0f40 + 0x20 * (n))

Also, I would still use the "#define macros", for example
"DISP_REG_OVL_ADDR offsets, and use the named constant in the
driver_data:

#define DISP_REG_OVL_ADDR_MT8173  0x0f40

(and in a later patch:
#define DISP_REG_OVL_ADDR_MT2701  0x0040
)

Also, I would still use the macro rather than open coding the "0x20 *
(n)", and just pass 'ovl' to the overlay macros that depend on
hardware type.
Something like the following:

#define DISP_REG_OVL_ADDR(ovl, n)  ((ovl)->data->ovl_addr + 0x20 * (n))

>
>  #define        OVL_RDMA_MEM_GMC        0x40402020
>
>  #define OVL_CON_BYTE_SWAP      BIT(24)
> -#define OVL_CON_CLRFMT_RGB565  (0 << 12)
> -#define OVL_CON_CLRFMT_RGB888  (1 << 12)

This seems like a really random and unnecessary hardware change.
Why chip designers, why!!?!?

For this one, it seems the polarity is either one way or the other, so
we can just use a bool to distinguish:

  bool fmt_rgb565_is_0;

> +static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = {
> +       .ovl = { DISP_REG_OVL_ADDR_MT8173, .fmt_rgb565_is_0 = true }
> +};

For use at runtime, the defines could become:

#define OVL_CON_CLRFMT_RGB565(ovl)  ((ovl)->data->fmt_rgb565_is_0 ? 0
: OVL_CON_CLRFMT_RGB888)
#define OVL_CON_CLRFMT_RGB888(ovl)  ((ovl)->data->fmt_rgb565_is_0 ?
OVL_CON_CLRFMT_RGB888 : 0)

>  #define OVL_CON_CLRFMT_RGBA8888        (2 << 12)
>  #define OVL_CON_CLRFMT_ARGB8888        (3 << 12)
>  #define        OVL_CON_AEN             BIT(8)
> @@ -137,18 +134,18 @@ static void mtk_ovl_layer_off(struct mtk_ddp_comp *comp, unsigned int idx)
>         writel(0x0, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
>  }
>
> -static unsigned int ovl_fmt_convert(unsigned int fmt)
> +static unsigned int ovl_fmt_convert(struct mtk_ddp_comp *comp, unsigned int fmt)
>  {
>         switch (fmt) {
>         default:
>         case DRM_FORMAT_RGB565:
> -               return OVL_CON_CLRFMT_RGB565;
> +               return comp->data->ovl.fmt_rgb565;

It will be nice to define a helper function for converting from the
generic 'mtk_ddp_comp' to the specific 'mtk_disp_ovl':

static inline struct mtk_disp_ovl *comp_to_ovl(struct mtk_ddp_comp *comp) {
  return container_of(comp, struct mtk_disp_ovl, ddp_comp);
}

Then these could become:
   return OVL_CON_CLRFMT_RGB565(comp_to_ovl(comp));

Or maybe cleaner, do the conversion once at the top of the function:
    struct mtk_disp_ovl *ovl = comp_to_ovl(comp);

And then just:
   return OVL_CON_CLRFMT_RGB565(ovl);


>         case DRM_FORMAT_BGR565:
> -               return OVL_CON_CLRFMT_RGB565 | OVL_CON_BYTE_SWAP;
> +               return comp->data->ovl.fmt_rgb565 | OVL_CON_BYTE_SWAP;
>         case DRM_FORMAT_RGB888:
> -               return OVL_CON_CLRFMT_RGB888;
> +               return comp->data->ovl.fmt_rgb888;
>         case DRM_FORMAT_BGR888:
> -               return OVL_CON_CLRFMT_RGB888 | OVL_CON_BYTE_SWAP;
> +               return comp->data->ovl.fmt_rgb888 | OVL_CON_BYTE_SWAP;
>         case DRM_FORMAT_RGBX8888:
>         case DRM_FORMAT_RGBA8888:
>                 return OVL_CON_CLRFMT_ARGB8888;

[snip]


> diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> index 1c366f8..935a8ef 100644
> --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> @@ -16,6 +16,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
>
> @@ -87,6 +88,9 @@
>
>  #define MIPITX_DSI_PLL_CON2    0x58
>
> +#define MIPITX_DSI_PLL_TOP     0x64
> +#define RG_DSI_MPPLL_PRESERVE          (0xff << 8)
> +
>  #define MIPITX_DSI_PLL_PWR     0x68
>  #define RG_DSI_MPPLL_SDM_PWR_ON                BIT(0)
>  #define RG_DSI_MPPLL_SDM_ISO_EN                BIT(1)
> @@ -123,10 +127,15 @@
>  #define SW_LNT2_HSTX_PRE_OE            BIT(24)
>  #define SW_LNT2_HSTX_OE                        BIT(25)
>
> +struct mtk_mipitx_data {
> +       const u32 data;

Use a better name, like "mppll_preserve".

Ok, that's it for now.
Actually, the patch set in general looks pretty good.

-Dan