| Message ID | 1531822342-4293-2-git-send-email-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: > cleanup err check in exynos_tmu_work as clk internal > framework will perform if clk is enable/disable > so drop the double check of IS_ERR and other such references. I do not understand the statement. Clock framework will perform if clk is enable/disable? How clock can be "enable" or "disable"? You mean gate clock? you mean clock pointer is an ERR pointer? > CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > drivers/thermal/samsung/exynos_tmu.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index 0164c9e..2dbde97 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) > > mutex_lock(&data->lock); > clk_enable(data->clk); > - if (!IS_ERR(data->clk_sec)) > - clk_enable(data->clk_sec); > + clk_enable(data->clk_sec); > > status = readb(data->base + EXYNOS_TMU_REG_STATUS); > if (!status) { > @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) > err: > clk_disable(data->clk); > mutex_unlock(&data->lock); > - if (!IS_ERR(data->clk_sec)) > - clk_disable(data->clk_sec); > + clk_disable(data->clk_sec); > out: > return ret; > } > @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work) > struct exynos_tmu_data *data = container_of(work, > struct exynos_tmu_data, irq_work); > > - if (!IS_ERR(data->clk_sec)) > - clk_enable(data->clk_sec); > - if (!IS_ERR(data->clk_sec)) > - clk_disable(data->clk_sec); > - > thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED); > > mutex_lock(&data->lock); > clk_enable(data->clk); > + clk_enable(data->clk_sec); You are changing here the logic completely. Before the "enable" was followed immediately by "disable". Now you are moving disable somewhere else... All this looks suspicious... Best regards, Krzysztof > > /* TODO: take action based on particular interrupt */ > data->tmu_clear_irqs(data); > > + clk_disable(data->clk_sec); > clk_disable(data->clk); > mutex_unlock(&data->lock); > enable_irq(data->irq); > @@ -1134,8 +1129,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) > err_sclk: > clk_disable_unprepare(data->sclk); > err_clk_sec: > - if (!IS_ERR(data->clk_sec)) > - clk_disable_unprepare(data->clk_sec); > + clk_disable_unprepare(data->clk_sec); > err_clk: > clk_disable_unprepare(data->clk); > err_sensor: > @@ -1155,8 +1149,7 @@ static int exynos_tmu_remove(struct platform_device *pdev) > > clk_disable_unprepare(data->sclk); > clk_disable_unprepare(data->clk); > - if (!IS_ERR(data->clk_sec)) > - clk_disable_unprepare(data->clk_sec); > + clk_disable_unprepare(data->clk_sec); > > if (!IS_ERR(data->regulator)) > regulator_disable(data->regulator); > -- > 2.7.4 >
Hi Krzysztof, On 17 July 2018 at 17:54, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >> cleanup err check in exynos_tmu_work as clk internal >> framework will perform if clk is enable/disable >> so drop the double check of IS_ERR and other such references. > > I do not understand the statement. Clock framework will perform if clk > is enable/disable? How clock can be "enable" or "disable"? You mean > gate clock? you mean clock pointer is an ERR pointer? > if (!IS_ERR(data->clk_sec)) check if the pointer is valid or not this check is again performed in clk_enable. >> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >> --- >> drivers/thermal/samsung/exynos_tmu.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c >> index 0164c9e..2dbde97 100644 >> --- a/drivers/thermal/samsung/exynos_tmu.c >> +++ b/drivers/thermal/samsung/exynos_tmu.c >> @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) >> >> mutex_lock(&data->lock); >> clk_enable(data->clk); >> - if (!IS_ERR(data->clk_sec)) >> - clk_enable(data->clk_sec); >> + clk_enable(data->clk_sec); >> >> status = readb(data->base + EXYNOS_TMU_REG_STATUS); >> if (!status) { >> @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) >> err: >> clk_disable(data->clk); >> mutex_unlock(&data->lock); >> - if (!IS_ERR(data->clk_sec)) >> - clk_disable(data->clk_sec); >> + clk_disable(data->clk_sec); >> out: >> return ret; >> } >> @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work) >> struct exynos_tmu_data *data = container_of(work, >> struct exynos_tmu_data, irq_work); >> >> - if (!IS_ERR(data->clk_sec)) >> - clk_enable(data->clk_sec); >> - if (!IS_ERR(data->clk_sec)) >> - clk_disable(data->clk_sec); >> - >> thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED); >> >> mutex_lock(&data->lock); >> clk_enable(data->clk); >> + clk_enable(data->clk_sec); > > You are changing here the logic completely. Before the "enable" was > followed immediately by "disable". Now you are moving disable > somewhere else... All this looks suspicious... I chose to move enable/disable of clk_sec this under the mutex lock for safe which dose the same sequence with different order. Second approach: We should get rid of clk_enable/disable in exynos_tmu_work as this looks unnecessary for toggle clk's on every update. Best Regards -Anand
On 17 July 2018 at 22:08, Anand Moon <linux.amoon@gmail.com> wrote: > Hi Krzysztof, > > On 17 July 2018 at 17:54, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >>> cleanup err check in exynos_tmu_work as clk internal >>> framework will perform if clk is enable/disable >>> so drop the double check of IS_ERR and other such references. >> >> I do not understand the statement. Clock framework will perform if clk >> is enable/disable? How clock can be "enable" or "disable"? You mean >> gate clock? you mean clock pointer is an ERR pointer? >> > > if (!IS_ERR(data->clk_sec)) > check if the pointer is valid or not > this check is again performed in > clk_enable. This should be then written in commit msg. >>> CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >>> --- >>> drivers/thermal/samsung/exynos_tmu.c | 19 ++++++------------- >>> 1 file changed, 6 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c >>> index 0164c9e..2dbde97 100644 >>> --- a/drivers/thermal/samsung/exynos_tmu.c >>> +++ b/drivers/thermal/samsung/exynos_tmu.c >>> @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) >>> >>> mutex_lock(&data->lock); >>> clk_enable(data->clk); >>> - if (!IS_ERR(data->clk_sec)) >>> - clk_enable(data->clk_sec); >>> + clk_enable(data->clk_sec); >>> >>> status = readb(data->base + EXYNOS_TMU_REG_STATUS); >>> if (!status) { >>> @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) >>> err: >>> clk_disable(data->clk); >>> mutex_unlock(&data->lock); >>> - if (!IS_ERR(data->clk_sec)) >>> - clk_disable(data->clk_sec); >>> + clk_disable(data->clk_sec); >>> out: >>> return ret; >>> } >>> @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work) >>> struct exynos_tmu_data *data = container_of(work, >>> struct exynos_tmu_data, irq_work); >>> >>> - if (!IS_ERR(data->clk_sec)) >>> - clk_enable(data->clk_sec); >>> - if (!IS_ERR(data->clk_sec)) >>> - clk_disable(data->clk_sec); >>> - >>> thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED); >>> >>> mutex_lock(&data->lock); >>> clk_enable(data->clk); >>> + clk_enable(data->clk_sec); >> >> You are changing here the logic completely. Before the "enable" was >> followed immediately by "disable". Now you are moving disable >> somewhere else... All this looks suspicious... > > I chose to move enable/disable of clk_sec this under the mutex lock for safe > which dose the same sequence with different order. > > Second approach: > We should get rid of clk_enable/disable in exynos_tmu_work > as this looks unnecessary for toggle clk's on every update. I already sent a cleanup for this: https://patchwork.kernel.org/patch/10529971/ Best regards, Krzysztof
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 0164c9e..2dbde97 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -300,8 +300,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) mutex_lock(&data->lock); clk_enable(data->clk); - if (!IS_ERR(data->clk_sec)) - clk_enable(data->clk_sec); + clk_enable(data->clk_sec); status = readb(data->base + EXYNOS_TMU_REG_STATUS); if (!status) { @@ -334,8 +333,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev) err: clk_disable(data->clk); mutex_unlock(&data->lock); - if (!IS_ERR(data->clk_sec)) - clk_disable(data->clk_sec); + clk_disable(data->clk_sec); out: return ret; } @@ -789,19 +787,16 @@ static void exynos_tmu_work(struct work_struct *work) struct exynos_tmu_data *data = container_of(work, struct exynos_tmu_data, irq_work); - if (!IS_ERR(data->clk_sec)) - clk_enable(data->clk_sec); - if (!IS_ERR(data->clk_sec)) - clk_disable(data->clk_sec); - thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED); mutex_lock(&data->lock); clk_enable(data->clk); + clk_enable(data->clk_sec); /* TODO: take action based on particular interrupt */ data->tmu_clear_irqs(data); + clk_disable(data->clk_sec); clk_disable(data->clk); mutex_unlock(&data->lock); enable_irq(data->irq); @@ -1134,8 +1129,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) err_sclk: clk_disable_unprepare(data->sclk); err_clk_sec: - if (!IS_ERR(data->clk_sec)) - clk_disable_unprepare(data->clk_sec); + clk_disable_unprepare(data->clk_sec); err_clk: clk_disable_unprepare(data->clk); err_sensor: @@ -1155,8 +1149,7 @@ static int exynos_tmu_remove(struct platform_device *pdev) clk_disable_unprepare(data->sclk); clk_disable_unprepare(data->clk); - if (!IS_ERR(data->clk_sec)) - clk_disable_unprepare(data->clk_sec); + clk_disable_unprepare(data->clk_sec); if (!IS_ERR(data->regulator)) regulator_disable(data->regulator);
cleanup err check in exynos_tmu_work as clk internal framework will perform if clk is enable/disable so drop the double check of IS_ERR and other such references. CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- drivers/thermal/samsung/exynos_tmu.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)