| Message ID | 20191221181855.31380-2-tiny.windzz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] PM / devfreq: rk3399_dmc: Add missing devfreq_event_disable_edev | expand |
Hi, Please use capital letter for the first char of patch title and better to edit the patch title as following: Actually, it is difficult to understand the role by only reading the function name. It depends on only this driver. So, better to edit it as following because devfreq-event is standard name in linux kernel. I think it is easy to understand what the patch does. - PM / devfreq: exynos-bus: Disable the devfreq-event device when failed 2019년 12월 22일 (일) 오전 3:21, Yangtao Li <tiny.windzz@gmail.com>님이 작성: > > The exynos_bus_profile_init process may fail, but the devfreq event device > remains enabled. Call devfreq_event_disable_edev on the error return path. > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > --- > drivers/devfreq/exynos-bus.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index 7f5917d59072..5e54eaf3cfc6 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -335,10 +335,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus, > ret = exynos_bus_set_event(bus); > if (ret < 0) { > dev_err(dev, "failed to set event to devfreq-event devices\n"); > - return ret; > + goto err_disable_edev; > } > > return 0; > + > +err_disable_edev: err_edev is enough instead of 'err_disable_edev' > + exynos_bus_disable_edev(bus); exynos_bus_disable_edev() has return value for detecting the error. Need to add following warning message. if (ret < 0) dev_warn(dev, "failed to disable the devfreq-event devices\n"); > + return ret; > } > > static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > -- > 2.17.1 > -- Best Regards, Chanwoo Choi
V2 has been sent. Thx, Yangtao
On Mon, Dec 23, 2019 at 1:02 AM Chanwoo Choi <chanwoo@kernel.org> wrote: > > Hi, > > Please use capital letter for the first char of patch title > and better to edit the patch title as following: > Actually, it is difficult to understand the role by only reading > the function name. It depends on only this driver. > So, better to edit it as following because devfreq-event > is standard name in linux kernel. I think it is easy to understand > what the patch does. > > - PM / devfreq: exynos-bus: Disable the devfreq-event device when failed > > > 2019년 12월 22일 (일) 오전 3:21, Yangtao Li <tiny.windzz@gmail.com>님이 작성: > > > > The exynos_bus_profile_init process may fail, but the devfreq event device > > remains enabled. Call devfreq_event_disable_edev on the error return path. > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > > --- > > drivers/devfreq/exynos-bus.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > > index 7f5917d59072..5e54eaf3cfc6 100644 > > --- a/drivers/devfreq/exynos-bus.c > > +++ b/drivers/devfreq/exynos-bus.c > > @@ -335,10 +335,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus, > > ret = exynos_bus_set_event(bus); > > if (ret < 0) { > > dev_err(dev, "failed to set event to devfreq-event devices\n"); > > - return ret; > > + goto err_disable_edev; > > } > > > > return 0; > > + > > +err_disable_edev: > > err_edev is enough instead of 'err_disable_edev' > > > + exynos_bus_disable_edev(bus); > > exynos_bus_disable_edev() has return value for detecting the error. > Need to add following warning message. > > if (ret < 0) > dev_warn(dev, "failed to disable the devfreq-event devices\n"); I'm not sure if it should be like this, it may rewrite the above error code. Yangtao > > > > + return ret; > > } > > > > static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > > -- > > 2.17.1 > > > > > -- > Best Regards, > Chanwoo Choi
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 7f5917d59072..5e54eaf3cfc6 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -335,10 +335,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus, ret = exynos_bus_set_event(bus); if (ret < 0) { dev_err(dev, "failed to set event to devfreq-event devices\n"); - return ret; + goto err_disable_edev; } return 0; + +err_disable_edev: + exynos_bus_disable_edev(bus); + return ret; } static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
The exynos_bus_profile_init process may fail, but the devfreq event device remains enabled. Call devfreq_event_disable_edev on the error return path. Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> --- drivers/devfreq/exynos-bus.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)