* [PATCH] thermal: exynos: add optional sclk support @ 2014-11-22 7:45 Abhilash Kesavan 2014-11-23 4:51 ` Chanwoo Choi 0 siblings, 1 reply; 9+ messages in thread From: Abhilash Kesavan @ 2014-11-22 7:45 UTC (permalink / raw) To: linux-arm-kernel Exynos7 has a special clock required for the functional operation of the TMU that is not present in earlier SoCs. Add support for this optional clock and update the binding documentation. Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> --- This patch was earlier part of the series adding TMU support for Exynos7 [1]. It has been split out as it does not impact the on-going consolidation in the exynos tmu driver and can be considered independently. .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ drivers/thermal/samsung/exynos_tmu.c | 27 ++++++++++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt index ae738f5..2393eac 100644 --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt @@ -32,10 +32,13 @@ - clocks : The main clocks for TMU device -- 1. operational clock for TMU channel -- 2. optional clock to access the shared registers of TMU channel + -- 3. optional special clock for functional operation - clock-names : Thermal system clock name -- "tmu_apbif" operational clock for current TMU channel -- "tmu_triminfo_apbif" clock to access the shared triminfo register for current TMU channel + -- "tmu_sclk" clock for functional operation of the current TMU + channel - vtmu-supply: This entry is optional and provides the regulator node supplying voltage to TMU. If needed this entry can be placed inside board/platform specific dts file. diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index d44d91d..6627937 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -128,6 +128,7 @@ * @lock: lock to implement synchronization. * @clk: pointer to the clock structure. * @clk_sec: pointer to the clock structure for accessing the base_second. + * @sclk: pointer to the clock structure for accessing the tmu special clk. * @temp_error1: fused value of the first point trim. * @temp_error2: fused value of the second point trim. * @regulator: pointer to the TMU regulator structure. @@ -147,7 +148,7 @@ struct exynos_tmu_data { enum soc_type soc; struct work_struct irq_work; struct mutex lock; - struct clk *clk, *clk_sec; + struct clk *clk, *clk_sec, *sclk; u8 temp_error1, temp_error2; struct regulator *regulator; struct thermal_sensor_conf *reg_conf; @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct platform_device *pdev) goto err_clk_sec; } + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); + if (IS_ERR(data->sclk)) { + dev_err(&pdev->dev, "Failed to get optional special clock\n"); + } else { + ret = clk_prepare_enable(data->sclk); + if (ret) { + dev_err(&pdev->dev, "Failed to enable special clock\n"); + goto err_clk; + } + } + ret = exynos_tmu_initialize(pdev); if (ret) { dev_err(&pdev->dev, "Failed to initialize TMU\n"); - goto err_clk; + goto err_sclk; } exynos_tmu_control(pdev, true); @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) sizeof(struct thermal_sensor_conf), GFP_KERNEL); if (!sensor_conf) { ret = -ENOMEM; - goto err_clk; + goto err_sclk; } sprintf(sensor_conf->name, "therm_zone%d", data->id); sensor_conf->read_temperature = (int (*)(void *))exynos_tmu_read; @@ -928,7 +940,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) ret = exynos_register_thermal(sensor_conf); if (ret) { dev_err(&pdev->dev, "Failed to register thermal interface\n"); - goto err_clk; + goto err_sclk; } data->reg_conf = sensor_conf; @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data); if (ret) { dev_err(&pdev->dev, "Failed to request irq: %d\n", data->irq); - goto err_clk; + goto err_sclk; } return 0; +err_sclk: + if (!IS_ERR(data->sclk)) + clk_disable_unprepare(data->sclk); err_clk: clk_unprepare(data->clk); err_clk_sec: @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct platform_device *pdev) exynos_tmu_control(pdev, false); + if (!IS_ERR(data->sclk)) + clk_disable_unprepare(data->sclk); clk_unprepare(data->clk); if (!IS_ERR(data->clk_sec)) clk_unprepare(data->clk_sec); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-22 7:45 [PATCH] thermal: exynos: add optional sclk support Abhilash Kesavan @ 2014-11-23 4:51 ` Chanwoo Choi 2014-11-23 5:32 ` Abhilash Kesavan 0 siblings, 1 reply; 9+ messages in thread From: Chanwoo Choi @ 2014-11-23 4:51 UTC (permalink / raw) To: linux-arm-kernel Hi Abhilash, On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan <a.kesavan@samsung.com> wrote: > Exynos7 has a special clock required for the functional operation > of the TMU that is not present in earlier SoCs. Add support for > this optional clock and update the binding documentation. > Latest Exynos SoC needs the special clocks. It is different part from previous Exynos SoC. Exynos3250 must need the special clock for ADC IP like this patch. So, I sent the smiliar patch[1] to support SCLK as following and merged it. [1] https://lkml.org/lkml/2014/7/21/734 So, I suggest you that Exynos would use the similar method to support special clock on all of IPs for Exynos SoC. > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > --- > This patch was earlier part of the series adding TMU support for > Exynos7 [1]. It has been split out as it does not impact the on-going > consolidation in the exynos tmu driver and can be considered > independently. > > .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ > drivers/thermal/samsung/exynos_tmu.c | 27 ++++++++++++++++---- > 2 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > index ae738f5..2393eac 100644 > --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > @@ -32,10 +32,13 @@ > - clocks : The main clocks for TMU device > -- 1. operational clock for TMU channel > -- 2. optional clock to access the shared registers of TMU channel > + -- 3. optional special clock for functional operation > - clock-names : Thermal system clock name > -- "tmu_apbif" operational clock for current TMU channel > -- "tmu_triminfo_apbif" clock to access the shared triminfo register > for current TMU channel > + -- "tmu_sclk" clock for functional operation of the current TMU > + channel > - vtmu-supply: This entry is optional and provides the regulator node supplying > voltage to TMU. If needed this entry can be placed inside > board/platform specific dts file. > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index d44d91d..6627937 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -128,6 +128,7 @@ > * @lock: lock to implement synchronization. > * @clk: pointer to the clock structure. > * @clk_sec: pointer to the clock structure for accessing the base_second. > + * @sclk: pointer to the clock structure for accessing the tmu special clk. > * @temp_error1: fused value of the first point trim. > * @temp_error2: fused value of the second point trim. > * @regulator: pointer to the TMU regulator structure. > @@ -147,7 +148,7 @@ struct exynos_tmu_data { > enum soc_type soc; > struct work_struct irq_work; > struct mutex lock; > - struct clk *clk, *clk_sec; > + struct clk *clk, *clk_sec, *sclk; > u8 temp_error1, temp_error2; > struct regulator *regulator; > struct thermal_sensor_conf *reg_conf; > @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct platform_device *pdev) > goto err_clk_sec; > } > > + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); > + if (IS_ERR(data->sclk)) { > + dev_err(&pdev->dev, "Failed to get optional special clock\n"); Exynos4, Exynos3250 etc may show error message always. I think It is not proper. I recommend that you use additional 'needs_sclk' field. If 'needs_sclk' is true, tmu driver will get the special clock of tmu without error message. Also, How about just 'sclk' instead of 'tmu_sclk'? I discussed the name of 'sclk' with Arnd Bergmann on following patch[2] [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html > + } else { > + ret = clk_prepare_enable(data->sclk); > + if (ret) { > + dev_err(&pdev->dev, "Failed to enable special clock\n"); > + goto err_clk; > + } > + } > + > ret = exynos_tmu_initialize(pdev); > if (ret) { > dev_err(&pdev->dev, "Failed to initialize TMU\n"); > - goto err_clk; > + goto err_sclk; > } > > exynos_tmu_control(pdev, true); > @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) > sizeof(struct thermal_sensor_conf), GFP_KERNEL); > if (!sensor_conf) { > ret = -ENOMEM; > - goto err_clk; > + goto err_sclk; > } > sprintf(sensor_conf->name, "therm_zone%d", data->id); > sensor_conf->read_temperature = (int (*)(void *))exynos_tmu_read; > @@ -928,7 +940,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) > ret = exynos_register_thermal(sensor_conf); > if (ret) { > dev_err(&pdev->dev, "Failed to register thermal interface\n"); > - goto err_clk; > + goto err_sclk; > } > data->reg_conf = sensor_conf; > > @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct platform_device *pdev) > IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data); > if (ret) { > dev_err(&pdev->dev, "Failed to request irq: %d\n", data->irq); > - goto err_clk; > + goto err_sclk; > } > > return 0; > +err_sclk: > + if (!IS_ERR(data->sclk)) > + clk_disable_unprepare(data->sclk); I think IS_ERROR don't be necessary because clk_disalbe_unprepare check the NULL pointer of clock pointer. > err_clk: > clk_unprepare(data->clk); > err_clk_sec: > @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct platform_device *pdev) > > exynos_tmu_control(pdev, false); > > + if (!IS_ERR(data->sclk)) > + clk_disable_unprepare(data->sclk); ditto. Best Regards, Chanwoo Choi > clk_unprepare(data->clk); > if (!IS_ERR(data->clk_sec)) > clk_unprepare(data->clk_sec); > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-23 4:51 ` Chanwoo Choi @ 2014-11-23 5:32 ` Abhilash Kesavan 2014-11-24 10:48 ` Lukasz Majewski 0 siblings, 1 reply; 9+ messages in thread From: Abhilash Kesavan @ 2014-11-23 5:32 UTC (permalink / raw) To: linux-arm-kernel Hi Chanwoo, On Sun, Nov 23, 2014 at 10:21 AM, Chanwoo Choi <cwchoi00@gmail.com> wrote: > Hi Abhilash, > > On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan <a.kesavan@samsung.com> wrote: >> Exynos7 has a special clock required for the functional operation >> of the TMU that is not present in earlier SoCs. Add support for >> this optional clock and update the binding documentation. >> > > Latest Exynos SoC needs the special clocks. It is different part from > previous Exynos SoC. > Exynos3250 must need the special clock for ADC IP like this patch. > So, I sent the smiliar patch[1] to support SCLK as following and merged it. > > [1] https://lkml.org/lkml/2014/7/21/734 Thanks for the link. > > So, I suggest you that Exynos would use the similar method to support > special clock > on all of IPs for Exynos SoC. > >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> --- >> This patch was earlier part of the series adding TMU support for >> Exynos7 [1]. It has been split out as it does not impact the on-going >> consolidation in the exynos tmu driver and can be considered >> independently. >> >> .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ >> drivers/thermal/samsung/exynos_tmu.c | 27 ++++++++++++++++---- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> index ae738f5..2393eac 100644 >> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> @@ -32,10 +32,13 @@ >> - clocks : The main clocks for TMU device >> -- 1. operational clock for TMU channel >> -- 2. optional clock to access the shared registers of TMU channel >> + -- 3. optional special clock for functional operation >> - clock-names : Thermal system clock name >> -- "tmu_apbif" operational clock for current TMU channel >> -- "tmu_triminfo_apbif" clock to access the shared triminfo register >> for current TMU channel >> + -- "tmu_sclk" clock for functional operation of the current TMU >> + channel >> - vtmu-supply: This entry is optional and provides the regulator node supplying >> voltage to TMU. If needed this entry can be placed inside >> board/platform specific dts file. >> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c >> index d44d91d..6627937 100644 >> --- a/drivers/thermal/samsung/exynos_tmu.c >> +++ b/drivers/thermal/samsung/exynos_tmu.c >> @@ -128,6 +128,7 @@ >> * @lock: lock to implement synchronization. >> * @clk: pointer to the clock structure. >> * @clk_sec: pointer to the clock structure for accessing the base_second. >> + * @sclk: pointer to the clock structure for accessing the tmu special clk. >> * @temp_error1: fused value of the first point trim. >> * @temp_error2: fused value of the second point trim. >> * @regulator: pointer to the TMU regulator structure. >> @@ -147,7 +148,7 @@ struct exynos_tmu_data { >> enum soc_type soc; >> struct work_struct irq_work; >> struct mutex lock; >> - struct clk *clk, *clk_sec; >> + struct clk *clk, *clk_sec, *sclk; >> u8 temp_error1, temp_error2; >> struct regulator *regulator; >> struct thermal_sensor_conf *reg_conf; >> @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct platform_device *pdev) >> goto err_clk_sec; >> } >> >> + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); >> + if (IS_ERR(data->sclk)) { >> + dev_err(&pdev->dev, "Failed to get optional special clock\n"); > > Exynos4, Exynos3250 etc may show error message always. I think It is not proper. > I recommend that you use additional 'needs_sclk' field. If 'needs_sclk' is true, > tmu driver will get the special clock of tmu without error message. OK, I'll add a per-soc field to check for sclk availability. > > Also, How about just 'sclk' instead of 'tmu_sclk'? > I discussed the name of 'sclk' with Arnd Bergmann on following patch[2] Sure, I'll use 'sclk' instead of 'tmu_sclk'. > > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html > >> + } else { >> + ret = clk_prepare_enable(data->sclk); >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to enable special clock\n"); >> + goto err_clk; >> + } >> + } >> + >> ret = exynos_tmu_initialize(pdev); >> if (ret) { >> dev_err(&pdev->dev, "Failed to initialize TMU\n"); >> - goto err_clk; >> + goto err_sclk; >> } >> >> exynos_tmu_control(pdev, true); >> @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) >> sizeof(struct thermal_sensor_conf), GFP_KERNEL); >> if (!sensor_conf) { >> ret = -ENOMEM; >> - goto err_clk; >> + goto err_sclk; >> } >> sprintf(sensor_conf->name, "therm_zone%d", data->id); >> sensor_conf->read_temperature = (int (*)(void *))exynos_tmu_read; >> @@ -928,7 +940,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) >> ret = exynos_register_thermal(sensor_conf); >> if (ret) { >> dev_err(&pdev->dev, "Failed to register thermal interface\n"); >> - goto err_clk; >> + goto err_sclk; >> } >> data->reg_conf = sensor_conf; >> >> @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct platform_device *pdev) >> IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data); >> if (ret) { >> dev_err(&pdev->dev, "Failed to request irq: %d\n", data->irq); >> - goto err_clk; >> + goto err_sclk; >> } >> >> return 0; >> +err_sclk: >> + if (!IS_ERR(data->sclk)) >> + clk_disable_unprepare(data->sclk); > > I think IS_ERROR don't be necessary because > clk_disalbe_unprepare check the NULL pointer of clock pointer. > >> err_clk: >> clk_unprepare(data->clk); >> err_clk_sec: >> @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct platform_device *pdev) >> >> exynos_tmu_control(pdev, false); >> >> + if (!IS_ERR(data->sclk)) >> + clk_disable_unprepare(data->sclk); > > ditto. Will fix. Thanks for the review. Regards, Abhilash > > Best Regards, > Chanwoo Choi > >> clk_unprepare(data->clk); >> if (!IS_ERR(data->clk_sec)) >> clk_unprepare(data->clk_sec); >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-23 5:32 ` Abhilash Kesavan @ 2014-11-24 10:48 ` Lukasz Majewski 2014-11-24 16:10 ` Abhilash Kesavan 0 siblings, 1 reply; 9+ messages in thread From: Lukasz Majewski @ 2014-11-24 10:48 UTC (permalink / raw) To: linux-arm-kernel Hi Abhilash, > Hi Chanwoo, > > On Sun, Nov 23, 2014 at 10:21 AM, Chanwoo Choi <cwchoi00@gmail.com> > wrote: > > Hi Abhilash, > > > > On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan > > <a.kesavan@samsung.com> wrote: > >> Exynos7 has a special clock required for the functional operation > >> of the TMU that is not present in earlier SoCs. Add support for > >> this optional clock and update the binding documentation. > >> > > > > Latest Exynos SoC needs the special clocks. It is different part > > from previous Exynos SoC. > > Exynos3250 must need the special clock for ADC IP like this patch. > > So, I sent the smiliar patch[1] to support SCLK as following and > > merged it. > > > > [1] https://lkml.org/lkml/2014/7/21/734 > > Thanks for the link. > > > > > So, I suggest you that Exynos would use the similar method to > > support special clock > > on all of IPs for Exynos SoC. > > > >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > >> --- > >> This patch was earlier part of the series adding TMU support for > >> Exynos7 [1]. It has been split out as it does not impact the > >> on-going consolidation in the exynos tmu driver and can be > >> considered independently. > >> > >> .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ > >> drivers/thermal/samsung/exynos_tmu.c | 27 > >> ++++++++++++++++---- 2 files changed, 25 insertions(+), 5 > >> deletions(-) > >> > >> diff --git > >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > >> index ae738f5..2393eac 100644 --- > >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt +++ > >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt @@ > >> -32,10 +32,13 @@ > >> - clocks : The main clocks for TMU device > >> -- 1. operational clock for TMU channel > >> -- 2. optional clock to access the shared registers of TMU > >> channel > >> + -- 3. optional special clock for functional operation > >> - clock-names : Thermal system clock name > >> -- "tmu_apbif" operational clock for current TMU channel > >> -- "tmu_triminfo_apbif" clock to access the shared > >> triminfo register for current TMU channel > >> + -- "tmu_sclk" clock for functional operation of the > >> current TMU > >> + channel > >> - vtmu-supply: This entry is optional and provides the regulator > >> node supplying voltage to TMU. If needed this entry can be placed > >> inside board/platform specific dts file. > >> diff --git a/drivers/thermal/samsung/exynos_tmu.c > >> b/drivers/thermal/samsung/exynos_tmu.c index d44d91d..6627937 > >> 100644 --- a/drivers/thermal/samsung/exynos_tmu.c > >> +++ b/drivers/thermal/samsung/exynos_tmu.c > >> @@ -128,6 +128,7 @@ > >> * @lock: lock to implement synchronization. > >> * @clk: pointer to the clock structure. > >> * @clk_sec: pointer to the clock structure for accessing the > >> base_second. > >> + * @sclk: pointer to the clock structure for accessing the tmu > >> special clk. > >> * @temp_error1: fused value of the first point trim. > >> * @temp_error2: fused value of the second point trim. > >> * @regulator: pointer to the TMU regulator structure. > >> @@ -147,7 +148,7 @@ struct exynos_tmu_data { > >> enum soc_type soc; > >> struct work_struct irq_work; > >> struct mutex lock; > >> - struct clk *clk, *clk_sec; > >> + struct clk *clk, *clk_sec, *sclk; > >> u8 temp_error1, temp_error2; > >> struct regulator *regulator; > >> struct thermal_sensor_conf *reg_conf; > >> @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct > >> platform_device *pdev) goto err_clk_sec; > >> } > >> > >> + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); > >> + if (IS_ERR(data->sclk)) { > >> + dev_err(&pdev->dev, "Failed to get optional > >> special clock\n"); > > > > Exynos4, Exynos3250 etc may show error message always. I think It > > is not proper. I recommend that you use additional 'needs_sclk' > > field. If 'needs_sclk' is true, tmu driver will get the special > > clock of tmu without error message. > > OK, I'll add a per-soc field to check for sclk availability. I think that it would be enough to change dev_err() -> dev_dbg(). > > > > > Also, How about just 'sclk' instead of 'tmu_sclk'? > > I discussed the name of 'sclk' with Arnd Bergmann on following > > patch[2] > > Sure, I'll use 'sclk' instead of 'tmu_sclk'. > > > > > [2] > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html > > > >> + } else { > >> + ret = clk_prepare_enable(data->sclk); > >> + if (ret) { > >> + dev_err(&pdev->dev, "Failed to enable > >> special clock\n"); > >> + goto err_clk; > >> + } > >> + } > >> + > >> ret = exynos_tmu_initialize(pdev); > >> if (ret) { > >> dev_err(&pdev->dev, "Failed to initialize TMU\n"); > >> - goto err_clk; > >> + goto err_sclk; > >> } > >> > >> exynos_tmu_control(pdev, true); > >> @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct > >> platform_device *pdev) sizeof(struct thermal_sensor_conf), > >> GFP_KERNEL); if (!sensor_conf) { > >> ret = -ENOMEM; > >> - goto err_clk; > >> + goto err_sclk; > >> } > >> sprintf(sensor_conf->name, "therm_zone%d", data->id); > >> sensor_conf->read_temperature = (int (*)(void > >> *))exynos_tmu_read; @@ -928,7 +940,7 @@ static int > >> exynos_tmu_probe(struct platform_device *pdev) ret = > >> exynos_register_thermal(sensor_conf); if (ret) { > >> dev_err(&pdev->dev, "Failed to register thermal > >> interface\n"); > >> - goto err_clk; > >> + goto err_sclk; > >> } > >> data->reg_conf = sensor_conf; > >> > >> @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct > >> platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_SHARED, > >> dev_name(&pdev->dev), data); if (ret) { > >> dev_err(&pdev->dev, "Failed to request irq: %d\n", > >> data->irq); > >> - goto err_clk; > >> + goto err_sclk; > >> } > >> > >> return 0; > >> +err_sclk: > >> + if (!IS_ERR(data->sclk)) > >> + clk_disable_unprepare(data->sclk); > > > > I think IS_ERROR don't be necessary because > > clk_disalbe_unprepare check the NULL pointer of clock pointer. > > > >> err_clk: > >> clk_unprepare(data->clk); > >> err_clk_sec: > >> @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct > >> platform_device *pdev) > >> > >> exynos_tmu_control(pdev, false); > >> > >> + if (!IS_ERR(data->sclk)) > >> + clk_disable_unprepare(data->sclk); > > > > ditto. > > Will fix. > > Thanks for the review. > > Regards, > Abhilash > > > > Best Regards, > > Chanwoo Choi > > > >> clk_unprepare(data->clk); > >> if (!IS_ERR(data->clk_sec)) > >> clk_unprepare(data->clk_sec); > >> -- > >> 1.7.9.5 > >> > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel at lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-24 10:48 ` Lukasz Majewski @ 2014-11-24 16:10 ` Abhilash Kesavan 2014-11-24 17:52 ` Eduardo Valentin 0 siblings, 1 reply; 9+ messages in thread From: Abhilash Kesavan @ 2014-11-24 16:10 UTC (permalink / raw) To: linux-arm-kernel Hi Lukasz, On Mon, Nov 24, 2014 at 4:18 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Abhilash, > >> Hi Chanwoo, >> >> On Sun, Nov 23, 2014 at 10:21 AM, Chanwoo Choi <cwchoi00@gmail.com> >> wrote: >> > Hi Abhilash, >> > >> > On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan >> > <a.kesavan@samsung.com> wrote: >> >> Exynos7 has a special clock required for the functional operation >> >> of the TMU that is not present in earlier SoCs. Add support for >> >> this optional clock and update the binding documentation. >> >> >> > >> > Latest Exynos SoC needs the special clocks. It is different part >> > from previous Exynos SoC. >> > Exynos3250 must need the special clock for ADC IP like this patch. >> > So, I sent the smiliar patch[1] to support SCLK as following and >> > merged it. >> > >> > [1] https://lkml.org/lkml/2014/7/21/734 >> >> Thanks for the link. >> >> > >> > So, I suggest you that Exynos would use the similar method to >> > support special clock >> > on all of IPs for Exynos SoC. >> > >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> >> --- >> >> This patch was earlier part of the series adding TMU support for >> >> Exynos7 [1]. It has been split out as it does not impact the >> >> on-going consolidation in the exynos tmu driver and can be >> >> considered independently. >> >> >> >> .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ >> >> drivers/thermal/samsung/exynos_tmu.c | 27 >> >> ++++++++++++++++---- 2 files changed, 25 insertions(+), 5 >> >> deletions(-) >> >> >> >> diff --git >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> >> index ae738f5..2393eac 100644 --- >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt +++ >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt @@ >> >> -32,10 +32,13 @@ >> >> - clocks : The main clocks for TMU device >> >> -- 1. operational clock for TMU channel >> >> -- 2. optional clock to access the shared registers of TMU >> >> channel >> >> + -- 3. optional special clock for functional operation >> >> - clock-names : Thermal system clock name >> >> -- "tmu_apbif" operational clock for current TMU channel >> >> -- "tmu_triminfo_apbif" clock to access the shared >> >> triminfo register for current TMU channel >> >> + -- "tmu_sclk" clock for functional operation of the >> >> current TMU >> >> + channel >> >> - vtmu-supply: This entry is optional and provides the regulator >> >> node supplying voltage to TMU. If needed this entry can be placed >> >> inside board/platform specific dts file. >> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c >> >> b/drivers/thermal/samsung/exynos_tmu.c index d44d91d..6627937 >> >> 100644 --- a/drivers/thermal/samsung/exynos_tmu.c >> >> +++ b/drivers/thermal/samsung/exynos_tmu.c >> >> @@ -128,6 +128,7 @@ >> >> * @lock: lock to implement synchronization. >> >> * @clk: pointer to the clock structure. >> >> * @clk_sec: pointer to the clock structure for accessing the >> >> base_second. >> >> + * @sclk: pointer to the clock structure for accessing the tmu >> >> special clk. >> >> * @temp_error1: fused value of the first point trim. >> >> * @temp_error2: fused value of the second point trim. >> >> * @regulator: pointer to the TMU regulator structure. >> >> @@ -147,7 +148,7 @@ struct exynos_tmu_data { >> >> enum soc_type soc; >> >> struct work_struct irq_work; >> >> struct mutex lock; >> >> - struct clk *clk, *clk_sec; >> >> + struct clk *clk, *clk_sec, *sclk; >> >> u8 temp_error1, temp_error2; >> >> struct regulator *regulator; >> >> struct thermal_sensor_conf *reg_conf; >> >> @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct >> >> platform_device *pdev) goto err_clk_sec; >> >> } >> >> >> >> + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); >> >> + if (IS_ERR(data->sclk)) { >> >> + dev_err(&pdev->dev, "Failed to get optional >> >> special clock\n"); >> > >> > Exynos4, Exynos3250 etc may show error message always. I think It >> > is not proper. I recommend that you use additional 'needs_sclk' >> > field. If 'needs_sclk' is true, tmu driver will get the special >> > clock of tmu without error message. >> >> OK, I'll add a per-soc field to check for sclk availability. > > I think that it would be enough to change dev_err() -> dev_dbg(). Lukasz, thanks for the review. Chanwoo, are you OK with this ? If so, I will post a v2 with this and the other changes you suggested. Abhilash > >> >> > >> > Also, How about just 'sclk' instead of 'tmu_sclk'? >> > I discussed the name of 'sclk' with Arnd Bergmann on following >> > patch[2] >> >> Sure, I'll use 'sclk' instead of 'tmu_sclk'. >> >> > >> > [2] >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html >> > >> >> + } else { >> >> + ret = clk_prepare_enable(data->sclk); >> >> + if (ret) { >> >> + dev_err(&pdev->dev, "Failed to enable >> >> special clock\n"); >> >> + goto err_clk; >> >> + } >> >> + } >> >> + >> >> ret = exynos_tmu_initialize(pdev); >> >> if (ret) { >> >> dev_err(&pdev->dev, "Failed to initialize TMU\n"); >> >> - goto err_clk; >> >> + goto err_sclk; >> >> } >> >> >> >> exynos_tmu_control(pdev, true); >> >> @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct >> >> platform_device *pdev) sizeof(struct thermal_sensor_conf), >> >> GFP_KERNEL); if (!sensor_conf) { >> >> ret = -ENOMEM; >> >> - goto err_clk; >> >> + goto err_sclk; >> >> } >> >> sprintf(sensor_conf->name, "therm_zone%d", data->id); >> >> sensor_conf->read_temperature = (int (*)(void >> >> *))exynos_tmu_read; @@ -928,7 +940,7 @@ static int >> >> exynos_tmu_probe(struct platform_device *pdev) ret = >> >> exynos_register_thermal(sensor_conf); if (ret) { >> >> dev_err(&pdev->dev, "Failed to register thermal >> >> interface\n"); >> >> - goto err_clk; >> >> + goto err_sclk; >> >> } >> >> data->reg_conf = sensor_conf; >> >> >> >> @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct >> >> platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_SHARED, >> >> dev_name(&pdev->dev), data); if (ret) { >> >> dev_err(&pdev->dev, "Failed to request irq: %d\n", >> >> data->irq); >> >> - goto err_clk; >> >> + goto err_sclk; >> >> } >> >> >> >> return 0; >> >> +err_sclk: >> >> + if (!IS_ERR(data->sclk)) >> >> + clk_disable_unprepare(data->sclk); >> > >> > I think IS_ERROR don't be necessary because >> > clk_disalbe_unprepare check the NULL pointer of clock pointer. >> > >> >> err_clk: >> >> clk_unprepare(data->clk); >> >> err_clk_sec: >> >> @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct >> >> platform_device *pdev) >> >> >> >> exynos_tmu_control(pdev, false); >> >> >> >> + if (!IS_ERR(data->sclk)) >> >> + clk_disable_unprepare(data->sclk); >> > >> > ditto. >> >> Will fix. >> >> Thanks for the review. >> >> Regards, >> Abhilash >> > >> > Best Regards, >> > Chanwoo Choi >> > >> >> clk_unprepare(data->clk); >> >> if (!IS_ERR(data->clk_sec)) >> >> clk_unprepare(data->clk_sec); >> >> -- >> >> 1.7.9.5 >> >> >> >> >> >> _______________________________________________ >> >> linux-arm-kernel mailing list >> >> linux-arm-kernel at lists.infradead.org >> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-24 16:10 ` Abhilash Kesavan @ 2014-11-24 17:52 ` Eduardo Valentin 2014-11-25 2:30 ` Abhilash Kesavan 0 siblings, 1 reply; 9+ messages in thread From: Eduardo Valentin @ 2014-11-24 17:52 UTC (permalink / raw) To: linux-arm-kernel Hello Folks, On Mon, Nov 24, 2014 at 09:40:38PM +0530, Abhilash Kesavan wrote: > Hi Lukasz, > > On Mon, Nov 24, 2014 at 4:18 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Hi Abhilash, > > > >> Hi Chanwoo, > >> > >> On Sun, Nov 23, 2014 at 10:21 AM, Chanwoo Choi <cwchoi00@gmail.com> > >> wrote: > >> > Hi Abhilash, > >> > > >> > On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan > >> > <a.kesavan@samsung.com> wrote: > >> >> Exynos7 has a special clock required for the functional operation > >> >> of the TMU that is not present in earlier SoCs. Add support for > >> >> this optional clock and update the binding documentation. > >> >> > >> > > >> > Latest Exynos SoC needs the special clocks. It is different part > >> > from previous Exynos SoC. > >> > Exynos3250 must need the special clock for ADC IP like this patch. > >> > So, I sent the smiliar patch[1] to support SCLK as following and > >> > merged it. > >> > > >> > [1] https://lkml.org/lkml/2014/7/21/734 > >> > >> Thanks for the link. > >> > >> > > >> > So, I suggest you that Exynos would use the similar method to > >> > support special clock > >> > on all of IPs for Exynos SoC. > >> > > >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > >> >> --- > >> >> This patch was earlier part of the series adding TMU support for > >> >> Exynos7 [1]. It has been split out as it does not impact the > >> >> on-going consolidation in the exynos tmu driver and can be > >> >> considered independently. > >> >> > >> >> .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ > >> >> drivers/thermal/samsung/exynos_tmu.c | 27 > >> >> ++++++++++++++++---- 2 files changed, 25 insertions(+), 5 > >> >> deletions(-) > >> >> > >> >> diff --git > >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt > >> >> index ae738f5..2393eac 100644 --- > >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt +++ > >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt @@ > >> >> -32,10 +32,13 @@ > >> >> - clocks : The main clocks for TMU device > >> >> -- 1. operational clock for TMU channel > >> >> -- 2. optional clock to access the shared registers of TMU > >> >> channel > >> >> + -- 3. optional special clock for functional operation > >> >> - clock-names : Thermal system clock name > >> >> -- "tmu_apbif" operational clock for current TMU channel > >> >> -- "tmu_triminfo_apbif" clock to access the shared > >> >> triminfo register for current TMU channel > >> >> + -- "tmu_sclk" clock for functional operation of the > >> >> current TMU > >> >> + channel > >> >> - vtmu-supply: This entry is optional and provides the regulator > >> >> node supplying voltage to TMU. If needed this entry can be placed > >> >> inside board/platform specific dts file. > >> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c > >> >> b/drivers/thermal/samsung/exynos_tmu.c index d44d91d..6627937 > >> >> 100644 --- a/drivers/thermal/samsung/exynos_tmu.c > >> >> +++ b/drivers/thermal/samsung/exynos_tmu.c > >> >> @@ -128,6 +128,7 @@ > >> >> * @lock: lock to implement synchronization. > >> >> * @clk: pointer to the clock structure. > >> >> * @clk_sec: pointer to the clock structure for accessing the > >> >> base_second. > >> >> + * @sclk: pointer to the clock structure for accessing the tmu > >> >> special clk. > >> >> * @temp_error1: fused value of the first point trim. > >> >> * @temp_error2: fused value of the second point trim. > >> >> * @regulator: pointer to the TMU regulator structure. > >> >> @@ -147,7 +148,7 @@ struct exynos_tmu_data { > >> >> enum soc_type soc; > >> >> struct work_struct irq_work; > >> >> struct mutex lock; > >> >> - struct clk *clk, *clk_sec; > >> >> + struct clk *clk, *clk_sec, *sclk; > >> >> u8 temp_error1, temp_error2; > >> >> struct regulator *regulator; > >> >> struct thermal_sensor_conf *reg_conf; > >> >> @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct > >> >> platform_device *pdev) goto err_clk_sec; > >> >> } > >> >> > >> >> + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); > >> >> + if (IS_ERR(data->sclk)) { > >> >> + dev_err(&pdev->dev, "Failed to get optional > >> >> special clock\n"); > >> > > >> > Exynos4, Exynos3250 etc may show error message always. I think It > >> > is not proper. I recommend that you use additional 'needs_sclk' > >> > field. If 'needs_sclk' is true, tmu driver will get the special > >> > clock of tmu without error message. > >> > >> OK, I'll add a per-soc field to check for sclk availability. > > > > I think that it would be enough to change dev_err() -> dev_dbg(). > > Lukasz, thanks for the review. > > Chanwoo, are you OK with this ? If so, I will post a v2 with this and > the other changes you suggested. Is this a mandatory or a optional clk? What happens if the chip has the clock, but you fail to get it in this code? Think the rule of thumb is to at least warn the user that you are bailing to the defaults, because you failed to get the clock, when you were supposed to. Besides, warning / logging (even if it is in debugging mode) the user that you failed to get a clock, when the board / platform does not support it, sounds at least bogus for them. my two cents, > > Abhilash > > > >> > >> > > >> > Also, How about just 'sclk' instead of 'tmu_sclk'? > >> > I discussed the name of 'sclk' with Arnd Bergmann on following > >> > patch[2] > >> > >> Sure, I'll use 'sclk' instead of 'tmu_sclk'. > >> > >> > > >> > [2] > >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html > >> > > >> >> + } else { > >> >> + ret = clk_prepare_enable(data->sclk); > >> >> + if (ret) { > >> >> + dev_err(&pdev->dev, "Failed to enable > >> >> special clock\n"); > >> >> + goto err_clk; > >> >> + } > >> >> + } > >> >> + > >> >> ret = exynos_tmu_initialize(pdev); > >> >> if (ret) { > >> >> dev_err(&pdev->dev, "Failed to initialize TMU\n"); > >> >> - goto err_clk; > >> >> + goto err_sclk; > >> >> } > >> >> > >> >> exynos_tmu_control(pdev, true); > >> >> @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct > >> >> platform_device *pdev) sizeof(struct thermal_sensor_conf), > >> >> GFP_KERNEL); if (!sensor_conf) { > >> >> ret = -ENOMEM; > >> >> - goto err_clk; > >> >> + goto err_sclk; > >> >> } > >> >> sprintf(sensor_conf->name, "therm_zone%d", data->id); > >> >> sensor_conf->read_temperature = (int (*)(void > >> >> *))exynos_tmu_read; @@ -928,7 +940,7 @@ static int > >> >> exynos_tmu_probe(struct platform_device *pdev) ret = > >> >> exynos_register_thermal(sensor_conf); if (ret) { > >> >> dev_err(&pdev->dev, "Failed to register thermal > >> >> interface\n"); > >> >> - goto err_clk; > >> >> + goto err_sclk; > >> >> } > >> >> data->reg_conf = sensor_conf; > >> >> > >> >> @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct > >> >> platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_SHARED, > >> >> dev_name(&pdev->dev), data); if (ret) { > >> >> dev_err(&pdev->dev, "Failed to request irq: %d\n", > >> >> data->irq); > >> >> - goto err_clk; > >> >> + goto err_sclk; > >> >> } > >> >> > >> >> return 0; > >> >> +err_sclk: > >> >> + if (!IS_ERR(data->sclk)) > >> >> + clk_disable_unprepare(data->sclk); > >> > > >> > I think IS_ERROR don't be necessary because > >> > clk_disalbe_unprepare check the NULL pointer of clock pointer. > >> > > >> >> err_clk: > >> >> clk_unprepare(data->clk); > >> >> err_clk_sec: > >> >> @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct > >> >> platform_device *pdev) > >> >> > >> >> exynos_tmu_control(pdev, false); > >> >> > >> >> + if (!IS_ERR(data->sclk)) > >> >> + clk_disable_unprepare(data->sclk); > >> > > >> > ditto. > >> > >> Will fix. > >> > >> Thanks for the review. > >> > >> Regards, > >> Abhilash > >> > > >> > Best Regards, > >> > Chanwoo Choi > >> > > >> >> clk_unprepare(data->clk); > >> >> if (!IS_ERR(data->clk_sec)) > >> >> clk_unprepare(data->clk_sec); > >> >> -- > >> >> 1.7.9.5 > >> >> > >> >> > >> >> _______________________________________________ > >> >> linux-arm-kernel mailing list > >> >> linux-arm-kernel at lists.infradead.org > >> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > > -- > > Best regards, > > > > Lukasz Majewski > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141124/14448735/attachment.sig> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-24 17:52 ` Eduardo Valentin @ 2014-11-25 2:30 ` Abhilash Kesavan 2014-11-25 5:18 ` Eduardo Valentin 0 siblings, 1 reply; 9+ messages in thread From: Abhilash Kesavan @ 2014-11-25 2:30 UTC (permalink / raw) To: linux-arm-kernel Hi Eduardo, On Mon, Nov 24, 2014 at 11:22 PM, Eduardo Valentin <edubezval@gmail.com> wrote: > Hello Folks, > > On Mon, Nov 24, 2014 at 09:40:38PM +0530, Abhilash Kesavan wrote: >> Hi Lukasz, >> >> On Mon, Nov 24, 2014 at 4:18 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: >> > Hi Abhilash, >> > >> >> Hi Chanwoo, >> >> >> >> On Sun, Nov 23, 2014 at 10:21 AM, Chanwoo Choi <cwchoi00@gmail.com> >> >> wrote: >> >> > Hi Abhilash, >> >> > >> >> > On Sat, Nov 22, 2014 at 4:45 PM, Abhilash Kesavan >> >> > <a.kesavan@samsung.com> wrote: >> >> >> Exynos7 has a special clock required for the functional operation >> >> >> of the TMU that is not present in earlier SoCs. Add support for >> >> >> this optional clock and update the binding documentation. >> >> >> >> >> > >> >> > Latest Exynos SoC needs the special clocks. It is different part >> >> > from previous Exynos SoC. >> >> > Exynos3250 must need the special clock for ADC IP like this patch. >> >> > So, I sent the smiliar patch[1] to support SCLK as following and >> >> > merged it. >> >> > >> >> > [1] https://lkml.org/lkml/2014/7/21/734 >> >> >> >> Thanks for the link. >> >> >> >> > >> >> > So, I suggest you that Exynos would use the similar method to >> >> > support special clock >> >> > on all of IPs for Exynos SoC. >> >> > >> >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> >> >> --- >> >> >> This patch was earlier part of the series adding TMU support for >> >> >> Exynos7 [1]. It has been split out as it does not impact the >> >> >> on-going consolidation in the exynos tmu driver and can be >> >> >> considered independently. >> >> >> >> >> >> .../devicetree/bindings/thermal/exynos-thermal.txt | 3 +++ >> >> >> drivers/thermal/samsung/exynos_tmu.c | 27 >> >> >> ++++++++++++++++---- 2 files changed, 25 insertions(+), 5 >> >> >> deletions(-) >> >> >> >> >> >> diff --git >> >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt >> >> >> index ae738f5..2393eac 100644 --- >> >> >> a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt +++ >> >> >> b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt @@ >> >> >> -32,10 +32,13 @@ >> >> >> - clocks : The main clocks for TMU device >> >> >> -- 1. operational clock for TMU channel >> >> >> -- 2. optional clock to access the shared registers of TMU >> >> >> channel >> >> >> + -- 3. optional special clock for functional operation >> >> >> - clock-names : Thermal system clock name >> >> >> -- "tmu_apbif" operational clock for current TMU channel >> >> >> -- "tmu_triminfo_apbif" clock to access the shared >> >> >> triminfo register for current TMU channel >> >> >> + -- "tmu_sclk" clock for functional operation of the >> >> >> current TMU >> >> >> + channel >> >> >> - vtmu-supply: This entry is optional and provides the regulator >> >> >> node supplying voltage to TMU. If needed this entry can be placed >> >> >> inside board/platform specific dts file. >> >> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c >> >> >> b/drivers/thermal/samsung/exynos_tmu.c index d44d91d..6627937 >> >> >> 100644 --- a/drivers/thermal/samsung/exynos_tmu.c >> >> >> +++ b/drivers/thermal/samsung/exynos_tmu.c >> >> >> @@ -128,6 +128,7 @@ >> >> >> * @lock: lock to implement synchronization. >> >> >> * @clk: pointer to the clock structure. >> >> >> * @clk_sec: pointer to the clock structure for accessing the >> >> >> base_second. >> >> >> + * @sclk: pointer to the clock structure for accessing the tmu >> >> >> special clk. >> >> >> * @temp_error1: fused value of the first point trim. >> >> >> * @temp_error2: fused value of the second point trim. >> >> >> * @regulator: pointer to the TMU regulator structure. >> >> >> @@ -147,7 +148,7 @@ struct exynos_tmu_data { >> >> >> enum soc_type soc; >> >> >> struct work_struct irq_work; >> >> >> struct mutex lock; >> >> >> - struct clk *clk, *clk_sec; >> >> >> + struct clk *clk, *clk_sec, *sclk; >> >> >> u8 temp_error1, temp_error2; >> >> >> struct regulator *regulator; >> >> >> struct thermal_sensor_conf *reg_conf; >> >> >> @@ -883,10 +884,21 @@ static int exynos_tmu_probe(struct >> >> >> platform_device *pdev) goto err_clk_sec; >> >> >> } >> >> >> >> >> >> + data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk"); >> >> >> + if (IS_ERR(data->sclk)) { >> >> >> + dev_err(&pdev->dev, "Failed to get optional >> >> >> special clock\n"); >> >> > >> >> > Exynos4, Exynos3250 etc may show error message always. I think It >> >> > is not proper. I recommend that you use additional 'needs_sclk' >> >> > field. If 'needs_sclk' is true, tmu driver will get the special >> >> > clock of tmu without error message. >> >> >> >> OK, I'll add a per-soc field to check for sclk availability. >> > >> > I think that it would be enough to change dev_err() -> dev_dbg(). >> >> Lukasz, thanks for the review. >> >> Chanwoo, are you OK with this ? If so, I will post a v2 with this and >> the other changes you suggested. > > Is this a mandatory or a optional clk? What happens if the chip has the > clock, but you fail to get it in this code? This is mandatory on Exynos7 but not on the older SoCs. On Exynos7 if this clock is disabled then the TMU will not function. > > Think the rule of thumb is to at least warn the user that you are > bailing to the defaults, because you failed to get the clock, when you > were supposed to. > > Besides, warning / logging (even if it is in debugging mode) the user > that you failed to get a clock, when the board / platform does not > support it, sounds at least bogus for them. So, you would rather that I go with Chanwoo's approach of adding a per-soc flag indicating the presence of this clock ? Regards, Abhilash > > > my two cents, > >> >> Abhilash >> > >> >> >> >> > >> >> > Also, How about just 'sclk' instead of 'tmu_sclk'? >> >> > I discussed the name of 'sclk' with Arnd Bergmann on following >> >> > patch[2] >> >> >> >> Sure, I'll use 'sclk' instead of 'tmu_sclk'. >> >> >> >> > >> >> > [2] >> >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273557.html >> >> > >> >> >> + } else { >> >> >> + ret = clk_prepare_enable(data->sclk); >> >> >> + if (ret) { >> >> >> + dev_err(&pdev->dev, "Failed to enable >> >> >> special clock\n"); >> >> >> + goto err_clk; >> >> >> + } >> >> >> + } >> >> >> + >> >> >> ret = exynos_tmu_initialize(pdev); >> >> >> if (ret) { >> >> >> dev_err(&pdev->dev, "Failed to initialize TMU\n"); >> >> >> - goto err_clk; >> >> >> + goto err_sclk; >> >> >> } >> >> >> >> >> >> exynos_tmu_control(pdev, true); >> >> >> @@ -896,7 +908,7 @@ static int exynos_tmu_probe(struct >> >> >> platform_device *pdev) sizeof(struct thermal_sensor_conf), >> >> >> GFP_KERNEL); if (!sensor_conf) { >> >> >> ret = -ENOMEM; >> >> >> - goto err_clk; >> >> >> + goto err_sclk; >> >> >> } >> >> >> sprintf(sensor_conf->name, "therm_zone%d", data->id); >> >> >> sensor_conf->read_temperature = (int (*)(void >> >> >> *))exynos_tmu_read; @@ -928,7 +940,7 @@ static int >> >> >> exynos_tmu_probe(struct platform_device *pdev) ret = >> >> >> exynos_register_thermal(sensor_conf); if (ret) { >> >> >> dev_err(&pdev->dev, "Failed to register thermal >> >> >> interface\n"); >> >> >> - goto err_clk; >> >> >> + goto err_sclk; >> >> >> } >> >> >> data->reg_conf = sensor_conf; >> >> >> >> >> >> @@ -936,10 +948,13 @@ static int exynos_tmu_probe(struct >> >> >> platform_device *pdev) IRQF_TRIGGER_RISING | IRQF_SHARED, >> >> >> dev_name(&pdev->dev), data); if (ret) { >> >> >> dev_err(&pdev->dev, "Failed to request irq: %d\n", >> >> >> data->irq); >> >> >> - goto err_clk; >> >> >> + goto err_sclk; >> >> >> } >> >> >> >> >> >> return 0; >> >> >> +err_sclk: >> >> >> + if (!IS_ERR(data->sclk)) >> >> >> + clk_disable_unprepare(data->sclk); >> >> > >> >> > I think IS_ERROR don't be necessary because >> >> > clk_disalbe_unprepare check the NULL pointer of clock pointer. >> >> > >> >> >> err_clk: >> >> >> clk_unprepare(data->clk); >> >> >> err_clk_sec: >> >> >> @@ -956,6 +971,8 @@ static int exynos_tmu_remove(struct >> >> >> platform_device *pdev) >> >> >> >> >> >> exynos_tmu_control(pdev, false); >> >> >> >> >> >> + if (!IS_ERR(data->sclk)) >> >> >> + clk_disable_unprepare(data->sclk); >> >> > >> >> > ditto. >> >> >> >> Will fix. >> >> >> >> Thanks for the review. >> >> >> >> Regards, >> >> Abhilash >> >> > >> >> > Best Regards, >> >> > Chanwoo Choi >> >> > >> >> >> clk_unprepare(data->clk); >> >> >> if (!IS_ERR(data->clk_sec)) >> >> >> clk_unprepare(data->clk_sec); >> >> >> -- >> >> >> 1.7.9.5 >> >> >> >> >> >> >> >> >> _______________________________________________ >> >> >> linux-arm-kernel mailing list >> >> >> linux-arm-kernel at lists.infradead.org >> >> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > >> > >> > >> > -- >> > Best regards, >> > >> > Lukasz Majewski >> > >> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-25 2:30 ` Abhilash Kesavan @ 2014-11-25 5:18 ` Eduardo Valentin 2014-11-25 17:23 ` Abhilash Kesavan 0 siblings, 1 reply; 9+ messages in thread From: Eduardo Valentin @ 2014-11-25 5:18 UTC (permalink / raw) To: linux-arm-kernel Abhilash, Hi, On Tue, Nov 25, 2014 at 08:00:50AM +0530, Abhilash Kesavan wrote: <big cut> > >> > >> Lukasz, thanks for the review. > >> > >> Chanwoo, are you OK with this ? If so, I will post a v2 with this and > >> the other changes you suggested. > > > > Is this a mandatory or a optional clk? What happens if the chip has the > > clock, but you fail to get it in this code? > > This is mandatory on Exynos7 but not on the older SoCs. On Exynos7 if > this clock is disabled then the TMU will not function. > It means, that only on Exynos7, outputing a dev_err is the right thing to do. On other SoCs versions, it does not make sense at all requesting this clock. > > > > Think the rule of thumb is to at least warn the user that you are > > bailing to the defaults, because you failed to get the clock, when you > > were supposed to. > > > > Besides, warning / logging (even if it is in debugging mode) the user > > that you failed to get a clock, when the board / platform does not > > support it, sounds at least bogus for them. > > So, you would rather that I go with Chanwoo's approach of adding a > per-soc flag indicating the presence of this clock ? > Yes. BR, Eduardo Valentin > Regards, > Abhilash -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141125/975c460d/attachment.sig> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] thermal: exynos: add optional sclk support 2014-11-25 5:18 ` Eduardo Valentin @ 2014-11-25 17:23 ` Abhilash Kesavan 0 siblings, 0 replies; 9+ messages in thread From: Abhilash Kesavan @ 2014-11-25 17:23 UTC (permalink / raw) To: linux-arm-kernel Hi Eduardo, On Tue, Nov 25, 2014 at 10:48 AM, Eduardo Valentin <edubezval@gmail.com> wrote: > Abhilash, > > Hi, > > On Tue, Nov 25, 2014 at 08:00:50AM +0530, Abhilash Kesavan wrote: > <big cut> > >> >> >> >> Lukasz, thanks for the review. >> >> >> >> Chanwoo, are you OK with this ? If so, I will post a v2 with this and >> >> the other changes you suggested. >> > >> > Is this a mandatory or a optional clk? What happens if the chip has the >> > clock, but you fail to get it in this code? >> >> This is mandatory on Exynos7 but not on the older SoCs. On Exynos7 if >> this clock is disabled then the TMU will not function. >> > > > It means, that only on Exynos7, outputing a dev_err is the right thing > to do. On other SoCs versions, it does not make sense at all requesting > this clock. OK > >> > >> > Think the rule of thumb is to at least warn the user that you are >> > bailing to the defaults, because you failed to get the clock, when you >> > were supposed to. >> > >> > Besides, warning / logging (even if it is in debugging mode) the user >> > that you failed to get a clock, when the board / platform does not >> > support it, sounds at least bogus for them. >> >> So, you would rather that I go with Chanwoo's approach of adding a >> per-soc flag indicating the presence of this clock ? >> > > Yes. Thanks for the clarification, will post v2 tomorrow. Abhilash > > BR, > > Eduardo Valentin > >> Regards, >> Abhilash ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-11-25 17:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-22 7:45 [PATCH] thermal: exynos: add optional sclk support Abhilash Kesavan 2014-11-23 4:51 ` Chanwoo Choi 2014-11-23 5:32 ` Abhilash Kesavan 2014-11-24 10:48 ` Lukasz Majewski 2014-11-24 16:10 ` Abhilash Kesavan 2014-11-24 17:52 ` Eduardo Valentin 2014-11-25 2:30 ` Abhilash Kesavan 2014-11-25 5:18 ` Eduardo Valentin 2014-11-25 17:23 ` Abhilash Kesavan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).