Hi Dafna, thank you very much for spending time to review the patch,
your time spent is very much appreciated.
On Thu, Aug 5, 2021 at 4:06 PM Dafna Hirschfeld
<dafna.hirschfeld@xxxxxxxxxxxxx> wrote:
> > +err_clk_prepare_enable:
> > + for (--i; i >= 0; i--) {
> > + if (IS_ERR(comp->clk[i]))
> > + continue;
> > + clk_disable_unprepare(comp->clk[i]);
> > }
> > +
> > + return status;
>
> There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
> so you can just use it.
As per Enric's suggestion earlier in this email thread, are you OK
with me making this change in a follow-up patch, particularly since
the logic as it is was preserved from previous functionality?
> > -static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> > +static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> > {
> > - struct device *dev = &mdp->pdev->dev;
> > struct mtk_mdp_comp *comp_node;
> > + int status;
> > + struct device *dev = &mdp->pdev->dev;
> > + int err;
> >
> > - list_for_each_entry(comp_node, &mdp->comp_list, node)
> > - mtk_mdp_comp_clock_on(dev, comp_node);
> > + list_for_each_entry(comp_node, &mdp->comp_list, node) {
> > + err = mtk_mdp_comp_clock_on(dev, comp_node);
> > + if (err) {
> > + status = err;
>
> You can get rid of the new var 'status' and just return ret in case of error
This seems like a nit (please let me know if you disagree), and it's
also cleaned up in a follow-on patch in the series ("don't
pm_run_time_get/put for master comp in clock_on"). Is making the
change you are suggesting here something that should require uploading
a new series version?
Eizan
