| Message ID | 1416642356-8694-1-git-send-email-a.kesavan@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/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@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/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@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
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@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
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@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
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
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
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);
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(-)