Re: [PATCH v4 12/14] drm/mediatek: Improve compatibility of display driver

["Shawn Sung (宋孝謙)" <Shawn.Sung@mediatek.com>]

On Wed, 2023-06-21 at 10:15 +0200, AngeloGioacchino Del Regno wrote:
> >     
> > >   
> > > +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;
> >

We intentionally ignored null dev and didn't return error here, since
MT8195 and MT8188 shares the same component list, there could be
components that are not probed and therefore is null here (For
example, the new hardware Padding in MT8188 only).
 
> > > +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.
> >

Thanks for the advice, submitted a new version using
mtk_ddp_comp_funcs.
 
> > > +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?!
> >

Yes, we ignored the unknown type intentionally, but this part has been
removed in the new patch since 'switch' are all removed after re-using
mtk_ddp_comp_funcs.
 
> > >   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?

Got it, has submitted a new version that returns the error immediately.
 
The reason we break here instead of returning error, is trying to make
one function only has one return. For example, always return at the end
of a function, so if someday we add a printk() at somewhere in that
function for debugging, it won't be skipped by the 'return' before it.
 
Not sure if this convention is not recommended when writing kernel
codes? Thanks.
 
Regards,
Hsiao Chien Sung