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@mediatek.com 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@mediatek.com
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