Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

On 05/12/2017 02:52, Eduardo Valentin wrote:
> Hello,
> 
> Catching up on old patches.
> On Fri, Jul 07, 2017 at 05:03:52PM +0200, Daniel Lezcano wrote:
>> The sensor id is unknown at init time and we use all id in the authorized
>> MAX_SENSORS interval to register the sensor. On this SoC there is one
>> thermal-zone with one sensor on it. No need to spit on the console everytime 
>> we
>> failed to register thermal sensors, information which is deliberaly known as 
>> it
>> is part of the discovery process.
>>
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>
>> Remove the error messages
> 
> Is this still needed? I am assuming no.

Right, no longer needed.

  -- Daniel

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

On 05/12/2017 02:52, Eduardo Valentin wrote:
> Hello,
> 
> Catching up on old patches.
> On Fri, Jul 07, 2017 at 05:03:52PM +0200, Daniel Lezcano wrote:
>> The sensor id is unknown at init time and we use all id in the authorized
>> MAX_SENSORS interval to register the sensor. On this SoC there is one
>> thermal-zone with one sensor on it. No need to spit on the console everytime 
>> we
>> failed to register thermal sensors, information which is deliberaly known as 
>> it
>> is part of the discovery process.
>>
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>>
>> Remove the error messages
> 
> Is this still needed? I am assuming no.

Right, no longer needed.

  -- Daniel

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Eduardo Valentin]

Hello,

Catching up on old patches.
On Fri, Jul 07, 2017 at 05:03:52PM +0200, Daniel Lezcano wrote:
> The sensor id is unknown at init time and we use all id in the authorized
> MAX_SENSORS interval to register the sensor. On this SoC there is one
> thermal-zone with one sensor on it. No need to spit on the console everytime 
> we
> failed to register thermal sensors, information which is deliberaly known as 
> it
> is part of the discovery process.
> 
>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
> 
> Remove the error messages

Is this still needed? I am assuming no.
.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/hisi_thermal.c | 12 ++--

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Eduardo Valentin]

Hello,

Catching up on old patches.
On Fri, Jul 07, 2017 at 05:03:52PM +0200, Daniel Lezcano wrote:
> The sensor id is unknown at init time and we use all id in the authorized
> MAX_SENSORS interval to register the sensor. On this SoC there is one
> thermal-zone with one sensor on it. No need to spit on the console everytime 
> we
> failed to register thermal sensors, information which is deliberaly known as 
> it
> is part of the discovery process.
> 
>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
> 
> Remove the error messages

Is this still needed? I am assuming no.
.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/hisi_thermal.c | 12 ++--

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

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 = < 0>;
> +   };
> +
> +   cls1: cls1 {
> +   polling-delay = <0>;
> +   polling-delay-passive = <0>;
> +   thermal-sensors = < 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 = < 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


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

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 = < 0>;
> +   };
> +
> +   cls1: cls1 {
> +   polling-delay = <0>;
> +   polling-delay-passive = <0>;
> +   thermal-sensors = < 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 = < 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


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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 = < 0>;
+   };
+
+   cls1: cls1 {
+   polling-delay = <0>;
+   polling-delay-passive = <0>;
+   thermal-sensors = < 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 = < 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

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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 = < 0>;
+   };
+
+   cls1: cls1 {
+   polling-delay = <0>;
+   polling-delay-passive = <0>;
+   thermal-sensors = < 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 = < 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

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

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,
>  
>>
>> sensors[i], i);
>   if (ret)
> - dev_err(>dev,
> - "failed to register thermal
> sensor:
> %d\n", ret);
> - else
> - hisi_thermal_toggle_sensor(
>>
>> sensors[i], true);
> + continue;
> +
> + hisi_thermal_toggle_sensor(>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(>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,
>>sensors[i], i);
> -   if (ret)
> +   if (ret == -ENODEV)
> +   dev_warn(>dev,
> +"thermal sensor %d has not bound\n", i);
> +   else if (ret)
> dev_err(>dev,
> "failed to register thermal sensor: %d\n", 
> ret);
> else
> 
> Thanks,
> Leo Yan
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

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,
>  
>>
>> sensors[i], i);
>   if (ret)
> - dev_err(>dev,
> - "failed to register thermal
> sensor:
> %d\n", ret);
> - else
> - hisi_thermal_toggle_sensor(
>>
>> sensors[i], true);
> + continue;
> +
> + hisi_thermal_toggle_sensor(>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(>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,
>>sensors[i], i);
> -   if (ret)
> +   if (ret == -ENODEV)
> +   dev_warn(>dev,
> +"thermal sensor %d has not bound\n", i);
> +   else if (ret)
> dev_err(>dev,
> "failed to register thermal sensor: %d\n", 
> ret);
> else
> 
> Thanks,
> Leo Yan
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Tue, 2017-08-08 at 21:29 +0800, 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,
> > > > >      
> > > > > > 
> > > > > > 
> > > > > > sensors[i], i);
> > > > >   if (ret)
> > > > > - dev_err(>dev,
> > > > > - "failed to register thermal
> > > > > sensor:
> > > > > %d\n", ret);
> > > > > - else
> > > > > - hisi_thermal_toggle_sensor(
> > > > > > 
> > > > > > 
> > > > > > sensors[i], true);
> > > > > + continue;
> > > > > +
> > > > > + hisi_thermal_toggle_sensor(
> > > > > >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.
> 
Now we will get three "thermal sensor %d has not bound" messages for
every normal probe, and an extra "failed to register thermal sensor:"
for a real failure probe?

If that's the case, as we are not using the sensors on purpose, why not
keep silence for -ENODEV?

thanks,
rui

> 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(>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,
>    >sensors[i], 
> i);
> -   if (ret)
> +   if (ret == -ENODEV)
> +   dev_warn(>dev,
> +"thermal sensor %d has not bound\n",
> i);
> +   else if (ret)
> dev_err(>dev,
> "failed to register thermal sensor:
> %d\n", ret);
> else
> 
> Thanks,
> Leo Yan

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Tue, 2017-08-08 at 21:29 +0800, 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,
> > > > >      
> > > > > > 
> > > > > > 
> > > > > > sensors[i], i);
> > > > >   if (ret)
> > > > > - dev_err(>dev,
> > > > > - "failed to register thermal
> > > > > sensor:
> > > > > %d\n", ret);
> > > > > - else
> > > > > - hisi_thermal_toggle_sensor(
> > > > > > 
> > > > > > 
> > > > > > sensors[i], true);
> > > > > + continue;
> > > > > +
> > > > > + hisi_thermal_toggle_sensor(
> > > > > >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.
> 
Now we will get three "thermal sensor %d has not bound" messages for
every normal probe, and an extra "failed to register thermal sensor:"
for a real failure probe?

If that's the case, as we are not using the sensors on purpose, why not
keep silence for -ENODEV?

thanks,
rui

> 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(>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,
>    >sensors[i], 
> i);
> -   if (ret)
> +   if (ret == -ENODEV)
> +   dev_warn(>dev,
> +"thermal sensor %d has not bound\n",
> i);
> +   else if (ret)
> dev_err(>dev,
> "failed to register thermal sensor:
> %d\n", ret);
> else
> 
> Thanks,
> Leo Yan

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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,
> > > >        
> > > > > 
> > > > > sensors[i], i);
> > > >     if (ret)
> > > > -   dev_err(>dev,
> > > > -   "failed to register thermal
> > > > sensor:
> > > > %d\n", ret);
> > > > -   else
> > > > -   hisi_thermal_toggle_sensor(
> > > > > 
> > > > > sensors[i], true);
> > > > +   continue;
> > > > +
> > > > +   hisi_thermal_toggle_sensor(>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.

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(>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,
   >sensors[i], i);
-   if (ret)
+   if (ret == -ENODEV)
+   dev_warn(>dev,
+"thermal sensor %d has not bound\n", i);
+   else if (ret)
dev_err(>dev,
"failed to register thermal sensor: %d\n", ret);
else

Thanks,
Leo Yan

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Leo Yan]

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,
> > > >        
> > > > > 
> > > > > sensors[i], i);
> > > >     if (ret)
> > > > -   dev_err(>dev,
> > > > -   "failed to register thermal
> > > > sensor:
> > > > %d\n", ret);
> > > > -   else
> > > > -   hisi_thermal_toggle_sensor(
> > > > > 
> > > > > sensors[i], true);
> > > > +   continue;
> > > > +
> > > > +   hisi_thermal_toggle_sensor(>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.

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(>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,
   >sensors[i], i);
-   if (ret)
+   if (ret == -ENODEV)
+   dev_warn(>dev,
+"thermal sensor %d has not bound\n", i);
+   else if (ret)
dev_err(>dev,
"failed to register thermal sensor: %d\n", ret);
else

Thanks,
Leo Yan

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Tue, 2017-08-08 at 12:15 +0200, Daniel Lezcano wrote:
> On 08/08/2017 09:55, Zhang Rui wrote:
> > 
> > On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
> > > 
> > > The sensor id is unknown at init time and we use all id in the
> > > authorized
> > > MAX_SENSORS interval to register the sensor. On this SoC there is
> > > one
> > > thermal-zone with one sensor on it. No need to spit on the
> > > console
> > > everytime we
> > > failed to register thermal sensors, information which is
> > > deliberaly
> > > known as it
> > > is part of the discovery process.
> > > 
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 0:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 1:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 3:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > > 
> > > Remove the error messages.
> > > 
> > > Signed-off-by: Daniel Lezcano 
> > > ---
> > >  drivers/thermal/hisi_thermal.c | 12 ++--
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/hisi_thermal.c
> > > b/drivers/thermal/hisi_thermal.c
> > > index f642966..2cc98c6 100644
> > > --- a/drivers/thermal/hisi_thermal.c
> > > +++ b/drivers/thermal/hisi_thermal.c
> > > @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void
> > > *_sensor,
> > > int *temp)
> > >  
> > >   dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
> > > thres=%d\n",
> > >   sensor->id, data->irq_enabled, *temp, sensor-
> > > > 
> > > > thres_temp);
> > > +
> > > + printk("id=%d, irq=%d, temp=%d, thres=%d\n",
> > > + sensor->id, data->irq_enabled, *temp, sensor-
> > > > 
> > > > thres_temp);
> > what's this printk for?
> Argh. It shouldn't be there.
> 
> > 
> > > 
> > >   /*
> > >    * Bind irq to sensor for two cases:
> > >    *   Reenable alarm IRQ if temperature below threshold;
> > > @@ -260,8 +263,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(>dev, "failed to register sensor
> > > id
> > > %d: %d\n",
> > > - sensor->id, ret);
> > >   return ret;
> > >   }
> > >  
> > > @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
> > > platform_device *pdev)
> > >   ret = hisi_thermal_register_sensor(pdev, data,
> > >      
> > > > 
> > > > sensors[i], i);
> > >   if (ret)
> > > - dev_err(>dev,
> > > - "failed to register thermal
> > > sensor:
> > > %d\n", ret);
> > > - else
> > > - hisi_thermal_toggle_sensor(
> > > > 
> > > > sensors[i], true);
> > > + continue;
> > > +
> > > + hisi_thermal_toggle_sensor(>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?

thanks,
rui
> That ends up with 3 errors in dmesg for nothing.
> 
> 
> 
> 
> 

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Tue, 2017-08-08 at 12:15 +0200, Daniel Lezcano wrote:
> On 08/08/2017 09:55, Zhang Rui wrote:
> > 
> > On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
> > > 
> > > The sensor id is unknown at init time and we use all id in the
> > > authorized
> > > MAX_SENSORS interval to register the sensor. On this SoC there is
> > > one
> > > thermal-zone with one sensor on it. No need to spit on the
> > > console
> > > everytime we
> > > failed to register thermal sensors, information which is
> > > deliberaly
> > > known as it
> > > is part of the discovery process.
> > > 
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 0:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 1:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register sensor id 3:
> > > -19
> > >  hisi_thermal f7030700.tsensor: failed to register thermal
> > > sensor:
> > > -19
> > > 
> > > Remove the error messages.
> > > 
> > > Signed-off-by: Daniel Lezcano 
> > > ---
> > >  drivers/thermal/hisi_thermal.c | 12 ++--
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/hisi_thermal.c
> > > b/drivers/thermal/hisi_thermal.c
> > > index f642966..2cc98c6 100644
> > > --- a/drivers/thermal/hisi_thermal.c
> > > +++ b/drivers/thermal/hisi_thermal.c
> > > @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void
> > > *_sensor,
> > > int *temp)
> > >  
> > >   dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
> > > thres=%d\n",
> > >   sensor->id, data->irq_enabled, *temp, sensor-
> > > > 
> > > > thres_temp);
> > > +
> > > + printk("id=%d, irq=%d, temp=%d, thres=%d\n",
> > > + sensor->id, data->irq_enabled, *temp, sensor-
> > > > 
> > > > thres_temp);
> > what's this printk for?
> Argh. It shouldn't be there.
> 
> > 
> > > 
> > >   /*
> > >    * Bind irq to sensor for two cases:
> > >    *   Reenable alarm IRQ if temperature below threshold;
> > > @@ -260,8 +263,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(>dev, "failed to register sensor
> > > id
> > > %d: %d\n",
> > > - sensor->id, ret);
> > >   return ret;
> > >   }
> > >  
> > > @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
> > > platform_device *pdev)
> > >   ret = hisi_thermal_register_sensor(pdev, data,
> > >      
> > > > 
> > > > sensors[i], i);
> > >   if (ret)
> > > - dev_err(>dev,
> > > - "failed to register thermal
> > > sensor:
> > > %d\n", ret);
> > > - else
> > > - hisi_thermal_toggle_sensor(
> > > > 
> > > > sensors[i], true);
> > > + continue;
> > > +
> > > + hisi_thermal_toggle_sensor(>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?

thanks,
rui
> That ends up with 3 errors in dmesg for nothing.
> 
> 
> 
> 
> 

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

On 08/08/2017 09:55, Zhang Rui wrote:
> On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
>> The sensor id is unknown at init time and we use all id in the
>> authorized
>> MAX_SENSORS interval to register the sensor. On this SoC there is one
>> thermal-zone with one sensor on it. No need to spit on the console
>> everytime we
>> failed to register thermal sensors, information which is deliberaly
>> known as it
>> is part of the discovery process.
>>
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>
>> Remove the error messages.
>>
>> Signed-off-by: Daniel Lezcano 
>> ---
>>  drivers/thermal/hisi_thermal.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c
>> b/drivers/thermal/hisi_thermal.c
>> index f642966..2cc98c6 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor,
>> int *temp)
>>  
>>  dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
>> thres=%d\n",
>>  sensor->id, data->irq_enabled, *temp, sensor-
>>> thres_temp);
>> +
>> +printk("id=%d, irq=%d, temp=%d, thres=%d\n",
>> +sensor->id, data->irq_enabled, *temp, sensor-
>>> thres_temp);
> 
> what's this printk for?

Argh. It shouldn't be there.

>>  /*
>>   * Bind irq to sensor for two cases:
>>   *   Reenable alarm IRQ if temperature below threshold;
>> @@ -260,8 +263,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(>dev, "failed to register sensor id
>> %d: %d\n",
>> -sensor->id, ret);
>>  return ret;
>>  }
>>  
>> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
>> platform_device *pdev)
>>  ret = hisi_thermal_register_sensor(pdev, data,
>> 
>>> sensors[i], i);
>>  if (ret)
>> -dev_err(>dev,
>> -"failed to register thermal sensor:
>> %d\n", ret);
>> -else
>> -hisi_thermal_toggle_sensor(
>>> sensors[i], true);
>> +continue;
>> +
>> +hisi_thermal_toggle_sensor(>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).

That ends up with 3 errors in dmesg for nothing.





-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

On 08/08/2017 09:55, Zhang Rui wrote:
> On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
>> The sensor id is unknown at init time and we use all id in the
>> authorized
>> MAX_SENSORS interval to register the sensor. On this SoC there is one
>> thermal-zone with one sensor on it. No need to spit on the console
>> everytime we
>> failed to register thermal sensors, information which is deliberaly
>> known as it
>> is part of the discovery process.
>>
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
>> -19
>>
>> Remove the error messages.
>>
>> Signed-off-by: Daniel Lezcano 
>> ---
>>  drivers/thermal/hisi_thermal.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c
>> b/drivers/thermal/hisi_thermal.c
>> index f642966..2cc98c6 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor,
>> int *temp)
>>  
>>  dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
>> thres=%d\n",
>>  sensor->id, data->irq_enabled, *temp, sensor-
>>> thres_temp);
>> +
>> +printk("id=%d, irq=%d, temp=%d, thres=%d\n",
>> +sensor->id, data->irq_enabled, *temp, sensor-
>>> thres_temp);
> 
> what's this printk for?

Argh. It shouldn't be there.

>>  /*
>>   * Bind irq to sensor for two cases:
>>   *   Reenable alarm IRQ if temperature below threshold;
>> @@ -260,8 +263,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(>dev, "failed to register sensor id
>> %d: %d\n",
>> -sensor->id, ret);
>>  return ret;
>>  }
>>  
>> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
>> platform_device *pdev)
>>  ret = hisi_thermal_register_sensor(pdev, data,
>> 
>>> sensors[i], i);
>>  if (ret)
>> -dev_err(>dev,
>> -"failed to register thermal sensor:
>> %d\n", ret);
>> -else
>> -hisi_thermal_toggle_sensor(
>>> sensors[i], true);
>> +continue;
>> +
>> +hisi_thermal_toggle_sensor(>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).

That ends up with 3 errors in dmesg for nothing.





-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
> The sensor id is unknown at init time and we use all id in the
> authorized
> MAX_SENSORS interval to register the sensor. On this SoC there is one
> thermal-zone with one sensor on it. No need to spit on the console
> everytime we
> failed to register thermal sensors, information which is deliberaly
> known as it
> is part of the discovery process.
> 
>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
> 
> Remove the error messages.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/hisi_thermal.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c
> b/drivers/thermal/hisi_thermal.c
> index f642966..2cc98c6 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor,
> int *temp)
>  
>   dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
> thres=%d\n",
>   sensor->id, data->irq_enabled, *temp, sensor-
> >thres_temp);
> +
> + printk("id=%d, irq=%d, temp=%d, thres=%d\n",
> + sensor->id, data->irq_enabled, *temp, sensor-
> >thres_temp);

what's this printk for?

>   /*
>    * Bind irq to sensor for two cases:
>    *   Reenable alarm IRQ if temperature below threshold;
> @@ -260,8 +263,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(>dev, "failed to register sensor id
> %d: %d\n",
> - sensor->id, ret);
>   return ret;
>   }
>  
> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
> platform_device *pdev)
>   ret = hisi_thermal_register_sensor(pdev, data,
>      
> >sensors[i], i);
>   if (ret)
> - dev_err(>dev,
> - "failed to register thermal sensor:
> %d\n", ret);
> - else
> - hisi_thermal_toggle_sensor(
> >sensors[i], true);
> + continue;
> +
> + hisi_thermal_toggle_sensor(>sensors[i], true);
>   }
>  
>   return 0;

With these removed, is there any other information in dmesg that
suggests this failure?

thanks,
rui

Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

[Zhang Rui]

On Fri, 2017-07-07 at 17:03 +0200, Daniel Lezcano wrote:
> The sensor id is unknown at init time and we use all id in the
> authorized
> MAX_SENSORS interval to register the sensor. On this SoC there is one
> thermal-zone with one sensor on it. No need to spit on the console
> everytime we
> failed to register thermal sensors, information which is deliberaly
> known as it
> is part of the discovery process.
> 
>  hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
>  hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
>  hisi_thermal f7030700.tsensor: failed to register thermal sensor:
> -19
> 
> Remove the error messages.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/thermal/hisi_thermal.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c
> b/drivers/thermal/hisi_thermal.c
> index f642966..2cc98c6 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor,
> int *temp)
>  
>   dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d,
> thres=%d\n",
>   sensor->id, data->irq_enabled, *temp, sensor-
> >thres_temp);
> +
> + printk("id=%d, irq=%d, temp=%d, thres=%d\n",
> + sensor->id, data->irq_enabled, *temp, sensor-
> >thres_temp);

what's this printk for?

>   /*
>    * Bind irq to sensor for two cases:
>    *   Reenable alarm IRQ if temperature below threshold;
> @@ -260,8 +263,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(>dev, "failed to register sensor id
> %d: %d\n",
> - sensor->id, ret);
>   return ret;
>   }
>  
> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
> platform_device *pdev)
>   ret = hisi_thermal_register_sensor(pdev, data,
>      
> >sensors[i], i);
>   if (ret)
> - dev_err(>dev,
> - "failed to register thermal sensor:
> %d\n", ret);
> - else
> - hisi_thermal_toggle_sensor(
> >sensors[i], true);
> + continue;
> +
> + hisi_thermal_toggle_sensor(>sensors[i], true);
>   }
>  
>   return 0;

With these removed, is there any other information in dmesg that
suggests this failure?

thanks,
rui

[PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

The sensor id is unknown at init time and we use all id in the authorized
MAX_SENSORS interval to register the sensor. On this SoC there is one
thermal-zone with one sensor on it. No need to spit on the console everytime we
failed to register thermal sensors, information which is deliberaly known as it
is part of the discovery process.

 hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
 hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
 hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19

Remove the error messages.

Signed-off-by: Daniel Lezcano 
---
 drivers/thermal/hisi_thermal.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index f642966..2cc98c6 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
 
dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
+
+   printk("id=%d, irq=%d, temp=%d, thres=%d\n",
+   sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
/*
 * Bind irq to sensor for two cases:
 *   Reenable alarm IRQ if temperature below threshold;
@@ -260,8 +263,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(>dev, "failed to register sensor id %d: %d\n",
-   sensor->id, ret);
return ret;
}
 
@@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct platform_device *pdev)
ret = hisi_thermal_register_sensor(pdev, data,
   >sensors[i], i);
if (ret)
-   dev_err(>dev,
-   "failed to register thermal sensor: %d\n", ret);
-   else
-   hisi_thermal_toggle_sensor(>sensors[i], true);
+   continue;
+
+   hisi_thermal_toggle_sensor(>sensors[i], true);
}
 
return 0;
-- 
2.7.4

[PATCH] thermal/drivers/hisi: Remove confusing error message

[Daniel Lezcano]

The sensor id is unknown at init time and we use all id in the authorized
MAX_SENSORS interval to register the sensor. On this SoC there is one
thermal-zone with one sensor on it. No need to spit on the console everytime we
failed to register thermal sensors, information which is deliberaly known as it
is part of the discovery process.

 hisi_thermal f7030700.tsensor: failed to register sensor id 0: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
 hisi_thermal f7030700.tsensor: failed to register sensor id 1: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19
 hisi_thermal f7030700.tsensor: failed to register sensor id 3: -19
 hisi_thermal f7030700.tsensor: failed to register thermal sensor: -19

Remove the error messages.

Signed-off-by: Daniel Lezcano 
---
 drivers/thermal/hisi_thermal.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index f642966..2cc98c6 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -187,6 +187,9 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
 
dev_dbg(>pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
+
+   printk("id=%d, irq=%d, temp=%d, thres=%d\n",
+   sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
/*
 * Bind irq to sensor for two cases:
 *   Reenable alarm IRQ if temperature below threshold;
@@ -260,8 +263,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(>dev, "failed to register sensor id %d: %d\n",
-   sensor->id, ret);
return ret;
}
 
@@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct platform_device *pdev)
ret = hisi_thermal_register_sensor(pdev, data,
   >sensors[i], i);
if (ret)
-   dev_err(>dev,
-   "failed to register thermal sensor: %d\n", ret);
-   else
-   hisi_thermal_toggle_sensor(>sensors[i], true);
+   continue;
+
+   hisi_thermal_toggle_sensor(>sensors[i], true);
}
 
return 0;
-- 
2.7.4