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 [off-list ref] 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

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)

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]

Use a better name, like "mppll_preserve".

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

-Dan