Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1716264 > unrolled thread
| Started by | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| First post | 2017-08-21 12:10 +0200 |
| Last post | 2017-08-23 08:20 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] thermal/drivers/hisi: Remove confusing error message Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-08-21 12:10 +0200
Re: [PATCH] thermal/drivers/hisi: Remove confusing error message Leo Yan <leo.yan@linaro.org> - 2017-08-22 10:10 +0200
Re: [PATCH] thermal/drivers/hisi: Remove confusing error message Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-08-22 10:30 +0200
Re: [PATCH] thermal/drivers/hisi: Remove confusing error message Leo Yan <leo.yan@linaro.org> - 2017-08-23 08:20 +0200
| From | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| Date | 2017-08-21 12:10 +0200 |
| Subject | Re: [PATCH] thermal/drivers/hisi: Remove confusing error message |
| Message-ID | <ugROG-339-9@gated-at.bofh.it> |
On 08/08/2017 15:29, Leo Yan wrote:
> On Tue, Aug 08, 2017 at 08:48:51PM +0800, Zhang Rui wrote:
>
> [...]
>
>>>>> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
>>>>> platform_device *pdev)
>>>>> ret = hisi_thermal_register_sensor(pdev, data,
>>>>> &data-
>>>>>>
>>>>>> sensors[i], i);
>>>>> if (ret)
>>>>> - dev_err(&pdev->dev,
>>>>> - "failed to register thermal
>>>>> sensor:
>>>>> %d\n", ret);
>>>>> - else
>>>>> - hisi_thermal_toggle_sensor(&data-
>>>>>>
>>>>>> sensors[i], true);
>>>>> + continue;
>>>>> +
>>>>> + hisi_thermal_toggle_sensor(&data->sensors[i],
>>>>> true);
>>>>> }
>>>>>
>>>>> return 0;
>>>> With these removed, is there any other information in dmesg that
>>>> suggests this failure?
>>> The problem is there are always failures showed in dmesg. The init
>>> function is based on the assumption there is HISI_MAX_SENSORS sensors
>>> which is not true for the hi6220 and that raises at boot time errors.
>>>
>>> Why HISI_MAX_SENSORS(=4) while there is only one on hi6220 AFAIK? and
>>> this driver is only used for hi6220 (now).
>>>
>> right, I think we should remove one error log, and then change the
>> HISI_MAX_SENSORS to reflect the reality instead.
>>
>> XinWei and Leo,
>> can you please help check this?
>
> Sure.
>
> Here I am a bit confusion and I think this is a common question for
> SoC thermal driver.
>
> Hi6220 does has 4 thermal sensors, but we now only use one sensor of
> them (thermal sensor id 2) to bind with thermal zone and other three
> sensors are not bound to any thermal zone. So this is the reason the
> booting reports the failure.
>
> I think changing HISI_MAX_SENSORS value cannot resolve this issue, due
> we are using thermal id 2. How about below change? We change to use
> warning for sensors without binding, and remove redundant log.
Hi Leo,
a cleanest solution would be either:
- add the 3 missing thermal sensors in the DT and default to the id 2
or
- remove all the code assuming 4 sensors and deal with the one unique
sensor
No ?
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 9c3ce34..6d34980 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -260,8 +260,6 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
> if (IS_ERR(sensor->tzd)) {
> ret = PTR_ERR(sensor->tzd);
> sensor->tzd = NULL;
> - dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
> - sensor->id, ret);
> return ret;
> }
>
> @@ -351,7 +349,10 @@ static int hisi_thermal_probe(struct platform_device *pdev)
> for (i = 0; i < HISI_MAX_SENSORS; ++i) {
> ret = hisi_thermal_register_sensor(pdev, data,
> &data->sensors[i], i);
> - if (ret)
> + if (ret == -ENODEV)
> + dev_warn(&pdev->dev,
> + "thermal sensor %d has not bound\n", i);
> + else if (ret)
> dev_err(&pdev->dev,
> "failed to register thermal sensor: %d\n", ret);
> else
>
> Thanks,
> Leo Yan
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
[toc] | [next] | [standalone]
| From | Leo Yan <leo.yan@linaro.org> |
|---|---|
| Date | 2017-08-22 10:10 +0200 |
| Message-ID | <uhcq5-83s-1@gated-at.bofh.it> |
| In reply to | #1716264 |
Hi Daniel,
On Mon, Aug 21, 2017 at 12:06:17PM +0200, Daniel Lezcano wrote:
[...]
> Hi Leo,
>
> a cleanest solution would be either:
>
> - add the 3 missing thermal sensors in the DT and default to the id 2
Yeah, so do you think below change works for you?
---8<---
ARM64: dts: hisilicon: add missed thermal sensors for Hi6220
The thermal driver tries to register four sensors but the DT only binds
one sensor (sensor ID 2) with thermal zone, as result the thermal driver
reports failure for missed thermal sensor binding.
This patch adds missed thermal sensor for Hi6220, so can dismiss the
booting failure log.
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index eacbe0d..44c2bc7 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -833,6 +833,18 @@
thermal-zones {
+ local: local {
+ polling-delay = <0>;
+ polling-delay-passive = <0>;
+ thermal-sensors = <&tsensor 0>;
+ };
+
+ cls1: cls1 {
+ polling-delay = <0>;
+ polling-delay-passive = <0>;
+ thermal-sensors = <&tsensor 1>;
+ };
+
cls0: cls0 {
polling-delay = <1000>;
polling-delay-passive = <100>;
@@ -862,6 +874,12 @@
};
};
};
+
+ gpu: gpu {
+ polling-delay = <0>;
+ polling-delay-passive = <0>;
+ thermal-sensors = <&tsensor 3>;
+ };
};
> or
>
> - remove all the code assuming 4 sensors and deal with the one unique
> sensor
I personally prefer to avoid doing this, if only register one unique
sensor this will let us have no flexiblity for trying multiple sensors
on this platform.
[...]
Thanks,
Leo Yan
[toc] | [prev] | [next] | [standalone]
| From | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| Date | 2017-08-22 10:30 +0200 |
| Message-ID | <uhcJs-8b9-9@gated-at.bofh.it> |
| In reply to | #1717141 |
On 22/08/2017 10:04, Leo Yan wrote:
> Hi Daniel,
>
> On Mon, Aug 21, 2017 at 12:06:17PM +0200, Daniel Lezcano wrote:
>
> [...]
>
>> Hi Leo,
>>
>> a cleanest solution would be either:
>>
>> - add the 3 missing thermal sensors in the DT and default to the id 2
>
> Yeah, so do you think below change works for you?
Isn't it possible to set the delay also ? so we don't have to send
another patch if we want to use one of those instead of 2.
> ---8<---
>
> ARM64: dts: hisilicon: add missed thermal sensors for Hi6220
>
> The thermal driver tries to register four sensors but the DT only binds
> one sensor (sensor ID 2) with thermal zone, as result the thermal driver
> reports failure for missed thermal sensor binding.
>
> This patch adds missed thermal sensor for Hi6220, so can dismiss the
> booting failure log.
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index eacbe0d..44c2bc7 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -833,6 +833,18 @@
>
> thermal-zones {
>
> + local: local {
> + polling-delay = <0>;
> + polling-delay-passive = <0>;
> + thermal-sensors = <&tsensor 0>;
> + };
> +
> + cls1: cls1 {
> + polling-delay = <0>;
> + polling-delay-passive = <0>;
> + thermal-sensors = <&tsensor 1>;
> + };
> +
> cls0: cls0 {
> polling-delay = <1000>;
> polling-delay-passive = <100>;
> @@ -862,6 +874,12 @@
> };
> };
> };
> +
> + gpu: gpu {
> + polling-delay = <0>;
> + polling-delay-passive = <0>;
> + thermal-sensors = <&tsensor 3>;
> + };
> };
>
>
>> or
>>
>> - remove all the code assuming 4 sensors and deal with the one unique
>> sensor
>
> I personally prefer to avoid doing this, if only register one unique
> sensor this will let us have no flexiblity for trying multiple sensors
> on this platform.
Ok, I will on the other side give a cleanup in the driver to optimize
the sensors lookup.
Thanks
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
[toc] | [prev] | [next] | [standalone]
| From | Leo Yan <leo.yan@linaro.org> |
|---|---|
| Date | 2017-08-23 08:20 +0200 |
| Message-ID | <uhxbc-5ng-13@gated-at.bofh.it> |
| In reply to | #1717180 |
On Tue, Aug 22, 2017 at 10:25:07AM +0200, Daniel Lezcano wrote: > On 22/08/2017 10:04, Leo Yan wrote: > > Hi Daniel, > > > > On Mon, Aug 21, 2017 at 12:06:17PM +0200, Daniel Lezcano wrote: > > > > [...] > > > >> Hi Leo, > >> > >> a cleanest solution would be either: > >> > >> - add the 3 missing thermal sensors in the DT and default to the id 2 > > > > Yeah, so do you think below change works for you? > > Isn't it possible to set the delay also ? so we don't have to send > another patch if we want to use one of those instead of 2. Yeah, this makes sense for me. Have shared updated DT binding patch with you, you could stack it with your changes. Thanks for the suggestion. [...] Thanks, Leo Yan
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web