Messages in this thread Patch in this message | | | Date | Wed, 21 Jun 2023 10:15:19 +0200 | Subject | Re: [PATCH v4 12/14] drm/mediatek: Improve compatibility of display driver | From | AngeloGioacchino Del Regno <> |
| |
Il 21/06/23 05:19, Hsiao Chien Sung ha scritto: > - Add new functions to enable/disable components, and > reuse them when clock enable/disable > - Check if the component is defined before using it since > some modules are MT8188 only (ex. PADDING) > - Control components according to its type rather than ID > - Use a for-loop to add/remove components in an arrays, > so we can only maintain this array to make sure every > component will be initialized properly > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 186 +++++++++--------- > 1 file changed, 96 insertions(+), 90 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > index c0a38f5217ee..69ae531294ff 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > @@ -51,6 +51,7 @@ enum mtk_ovl_adaptor_comp_id { > > struct ovl_adaptor_comp_match { > enum mtk_ovl_adaptor_comp_type type; > + enum mtk_ddp_comp_id comp_id; > int alias_id; > }; > > @@ -67,21 +68,78 @@ static const char * const private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = { > }; > > static const struct ovl_adaptor_comp_match comp_matches[OVL_ADAPTOR_ID_MAX] = { > - [OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_RDMA, 0 }, > - [OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_RDMA, 1 }, > - [OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_RDMA, 2 }, > - [OVL_ADAPTOR_MDP_RDMA3] = { OVL_ADAPTOR_TYPE_RDMA, 3 }, > - [OVL_ADAPTOR_MDP_RDMA4] = { OVL_ADAPTOR_TYPE_RDMA, 4 }, > - [OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_RDMA, 5 }, > - [OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_RDMA, 6 }, > - [OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_RDMA, 7 }, > - [OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, 1 }, > - [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, 2 }, > - [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, 3 }, > - [OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, 4 }, > - [OVL_ADAPTOR_ETHDR0] = { OVL_ADAPTOR_TYPE_ETHDR, 0 }, > + [OVL_ADAPTOR_ETHDR0] = { OVL_ADAPTOR_TYPE_ETHDR, DDP_COMPONENT_ETHDR_MIXER, 0 }, > + [OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA0, 0 }, > + [OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA1, 1 }, > + [OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA2, 2 }, > + [OVL_ADAPTOR_MDP_RDMA3] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA3, 3 }, > + [OVL_ADAPTOR_MDP_RDMA4] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA4, 4 }, > + [OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA5, 5 }, > + [OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA6, 6 }, > + [OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA7, 7 }, > + [OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE1, 1 }, > + [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE2, 2 }, > + [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE3, 3 }, > + [OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE4, 4 }, > }; > > +static int mtk_ovl_adaptor_enable(struct device *dev, enum mtk_ovl_adaptor_comp_type type) > +{ > + int ret = 0;
int ret;
> + > + if (!dev)
if (!dev) return -ENODEV;
> + goto end; > + > + switch (type) { > + case OVL_ADAPTOR_TYPE_ETHDR: > + ret = mtk_ethdr_clk_enable(dev); > + break; > + case OVL_ADAPTOR_TYPE_MERGE: > + ret = mtk_merge_clk_enable(dev);
We already have a .clk_enable() callback in struct mtk_ddp_comp_funcs: to greatly enhance your nice cleanup, you could use that instead, which basically eliminates the need of having any if branch and/or switch.
> + break; > + case OVL_ADAPTOR_TYPE_RDMA: > + // only LARB users need to do this
Please, C-style comments only.
> + ret = pm_runtime_get_sync(dev); > + if (ret < 0) { > + dev_err(dev, "Failed to enable power domain, error(%d)\n", ret); > + goto end; > + } > + ret = mtk_mdp_rdma_clk_enable(dev); > + if (ret) > + pm_runtime_put(dev); > + break; > + default: > + dev_err(dev, "Unknown type: %d\n", type);
Are we supposed to return 0 for unknown type?!
> + } > + > + if (ret) > + dev_err(dev, "Failed to enable clock: error(%d)\n", ret);
if (ret) { dev_err(...) return ret; }
return 0;
> + > +end: > + return ret; > +} > + > +static void mtk_ovl_adaptor_disable(struct device *dev, enum mtk_ovl_adaptor_comp_type type) > +{ > + if (!dev) > + return; > + > + switch (type) { > + case OVL_ADAPTOR_TYPE_ETHDR: > + mtk_ethdr_clk_disable(dev); > + break; > + case OVL_ADAPTOR_TYPE_MERGE: > + mtk_merge_clk_disable(dev); > + break; > + case OVL_ADAPTOR_TYPE_RDMA: > + mtk_mdp_rdma_clk_disable(dev); > + pm_runtime_put(dev); > + break; > + default: > + dev_err(dev, "Unknown type: %d\n", type); > + } > +} > + > void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx, > struct mtk_plane_state *state, > struct cmdq_pkt *cmdq_pkt) > @@ -186,72 +244,30 @@ void mtk_ovl_adaptor_stop(struct device *dev) > int mtk_ovl_adaptor_clk_enable(struct device *dev) > { > struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev); > - struct device *comp; > - int ret; > + int ret = 0;
Keep `int ret;`
> int i; > > - for (i = 0; i < OVL_ADAPTOR_MERGE0; i++) { > - comp = ovl_adaptor->ovl_adaptor_comp[i]; > - ret = pm_runtime_get_sync(comp); > - if (ret < 0) { > - dev_err(dev, "Failed to enable power domain %d, err %d\n", i, ret); > - goto pwr_err; > - } > - } > - > for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) { > - comp = ovl_adaptor->ovl_adaptor_comp[i]; > - > - if (i < OVL_ADAPTOR_MERGE0) > - ret = mtk_mdp_rdma_clk_enable(comp); > - else if (i < OVL_ADAPTOR_ETHDR0) > - ret = mtk_merge_clk_enable(comp); > - else > - ret = mtk_ethdr_clk_enable(comp); > + ret = mtk_ovl_adaptor_enable(ovl_adaptor->ovl_adaptor_comp[i], > + comp_matches[i].type); > if (ret) { > - dev_err(dev, "Failed to enable clock %d, err %d\n", i, ret); > - goto clk_err; > + while (--i >= 0) > + mtk_ovl_adaptor_disable(ovl_adaptor->ovl_adaptor_comp[i], > + comp_matches[i].type); > + break;
Instead of a break here, just return ret?
> } > } > - > - return ret; > - > -clk_err: > - while (--i >= 0) { > - comp = ovl_adaptor->ovl_adaptor_comp[i]; > - if (i < OVL_ADAPTOR_MERGE0) > - mtk_mdp_rdma_clk_disable(comp); > - else if (i < OVL_ADAPTOR_ETHDR0) > - mtk_merge_clk_disable(comp); > - else > - mtk_ethdr_clk_disable(comp); > - } > - i = OVL_ADAPTOR_MERGE0; > - > -pwr_err: > - while (--i >= 0) > - pm_runtime_put(ovl_adaptor->ovl_adaptor_comp[i]); > - > return ret;
...and return 0 here.
> } >
Regards, Angelo
| |