| Message ID | 20210802220943.v6.1.I9db0d408ef79d300672ec0311a6bee9556801631@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor MTK MDP driver into core/components | expand |
Hi Eizan, Thank you for your patch. Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag. 2021 a les 14:13: > > Up until this change, many errors were logged but ignored when powering > on clocks inside mtk_mdp_core. This change tries to do a better job at > propagating errors back to the power management framework. > > Signed-off-by: Eizan Miyamoto <eizan@chromium.org> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > (no changes since v1) > > drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++----- > drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 +- > drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++----- > 3 files changed, 39 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > index b3426a551bea..76e295c8d9bc 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > @@ -13,10 +13,9 @@ > > #include "mtk_mdp_comp.h" > > - > -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > { > - int i, err; > + int i, err, status; > > if (comp->larb_dev) { > err = mtk_smi_larb_get(comp->larb_dev); > @@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > if (IS_ERR(comp->clk[i])) > continue; > err = clk_prepare_enable(comp->clk[i]); > - if (err) > - dev_err(dev, > - "failed to enable clock, err %d. type:%d i:%d\n", > - err, comp->type, i); > + if (err) { > + status = err; > + dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i); > + goto err_clk_prepare_enable; > + } > + } > + > + return 0; > + > +err_clk_prepare_enable: > + for (--i; i >= 0; i--) { > + if (IS_ERR(comp->clk[i])) > + continue; > + clk_disable_unprepare(comp->clk[i]); nit: We could use bulk functions here, but that's probably a follow-up patch. > } > + > + return status; > } > > void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp) > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > index 7897766c96bb..92ab5249bcad 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > @@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node, > struct mtk_mdp_comp *comp, > enum mtk_mdp_comp_type comp_type); > void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp); > -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); > +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); > void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp); > > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > index 976aa1f4829b..412bbec0f735 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > @@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = { > }; > MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids); > > -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; > + goto err_mtk_mdp_comp_clock_on; > + } > + } > + > + return 0; > + > +err_mtk_mdp_comp_clock_on: > + list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) > + mtk_mdp_comp_clock_off(dev, comp_node); > + > + return status; > } > > static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp) > @@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev) > { > struct mtk_mdp_dev *mdp = dev_get_drvdata(dev); > > - mtk_mdp_clock_on(mdp); > - > - return 0; > + return mtk_mdp_clock_on(mdp); > } > > static int __maybe_unused mtk_mdp_suspend(struct device *dev) > -- > 2.32.0.554.ge1b32706d8-goog >
On 02.08.21 14:12, Eizan Miyamoto wrote: > Up until this change, many errors were logged but ignored when powering > on clocks inside mtk_mdp_core. This change tries to do a better job at > propagating errors back to the power management framework. > > Signed-off-by: Eizan Miyamoto <eizan@chromium.org> > --- > > (no changes since v1) > > drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++----- > drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 +- > drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++----- > 3 files changed, 39 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > index b3426a551bea..76e295c8d9bc 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c > @@ -13,10 +13,9 @@ > > #include "mtk_mdp_comp.h" > > - > -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > { > - int i, err; > + int i, err, status; > > if (comp->larb_dev) { > err = mtk_smi_larb_get(comp->larb_dev); > @@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) > if (IS_ERR(comp->clk[i])) > continue; > err = clk_prepare_enable(comp->clk[i]); > - if (err) > - dev_err(dev, > - "failed to enable clock, err %d. type:%d i:%d\n", > - err, comp->type, i); > + if (err) { > + status = err; > + dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i); > + goto err_clk_prepare_enable; > + } > + } > + > + return 0; > + > +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. > } > > void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp) > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > index 7897766c96bb..92ab5249bcad 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h > @@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node, > struct mtk_mdp_comp *comp, > enum mtk_mdp_comp_type comp_type); > void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp); > -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); > +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); > void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp); > > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > index 976aa1f4829b..412bbec0f735 100644 > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c > @@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = { > }; > MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids); > > -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 > + goto err_mtk_mdp_comp_clock_on; > + } > + } > + > + return 0; > + > +err_mtk_mdp_comp_clock_on: > + list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) > + mtk_mdp_comp_clock_off(dev, comp_node); > + > + return status; > } > > static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp) > @@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev) > { > struct mtk_mdp_dev *mdp = dev_get_drvdata(dev); > > - mtk_mdp_clock_on(mdp); > - > - return 0; > + return mtk_mdp_clock_on(mdp); > } > > static int __maybe_unused mtk_mdp_suspend(struct device *dev) >
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@collabora.com> 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
Hi, On 09.08.21 05:23, Eizan Miyamoto wrote: > 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@collabora.com> 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? sure, I just give suggestions. A follow-up patch would be nice. > >>> -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? Hi, no, I am fine with it. No need for new version. Thanks, Dafna > > Eizan >
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote: > Up until this change, many errors were logged but ignored when > powering > on clocks inside mtk_mdp_core. This change tries to do a better job > at > propagating errors back to the power management framework. > > Signed-off-by: Eizan Miyamoto <eizan@chromium.org> > --- Reviewed-by: Houlong Wei <houlong.wei@mediatek.com> > (no changes since v1) [...]
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c index b3426a551bea..76e295c8d9bc 100644 --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c @@ -13,10 +13,9 @@ #include "mtk_mdp_comp.h" - -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) { - int i, err; + int i, err, status; if (comp->larb_dev) { err = mtk_smi_larb_get(comp->larb_dev); @@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp) if (IS_ERR(comp->clk[i])) continue; err = clk_prepare_enable(comp->clk[i]); - if (err) - dev_err(dev, - "failed to enable clock, err %d. type:%d i:%d\n", - err, comp->type, i); + if (err) { + status = err; + dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i); + goto err_clk_prepare_enable; + } + } + + return 0; + +err_clk_prepare_enable: + for (--i; i >= 0; i--) { + if (IS_ERR(comp->clk[i])) + continue; + clk_disable_unprepare(comp->clk[i]); } + + return status; } void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp) diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h index 7897766c96bb..92ab5249bcad 100644 --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h @@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node, struct mtk_mdp_comp *comp, enum mtk_mdp_comp_type comp_type); void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp); -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp); void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp); diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c index 976aa1f4829b..412bbec0f735 100644 --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c @@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = { }; MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids); -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; + goto err_mtk_mdp_comp_clock_on; + } + } + + return 0; + +err_mtk_mdp_comp_clock_on: + list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) + mtk_mdp_comp_clock_off(dev, comp_node); + + return status; } static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp) @@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev) { struct mtk_mdp_dev *mdp = dev_get_drvdata(dev); - mtk_mdp_clock_on(mdp); - - return 0; + return mtk_mdp_clock_on(mdp); } static int __maybe_unused mtk_mdp_suspend(struct device *dev)
Up until this change, many errors were logged but ignored when powering on clocks inside mtk_mdp_core. This change tries to do a better job at propagating errors back to the power management framework. Signed-off-by: Eizan Miyamoto <eizan@chromium.org> --- (no changes since v1) drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++----- drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 +- drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++----- 3 files changed, 39 insertions(+), 15 deletions(-)