| Message ID | 20170808132955.GB32732@leoy-ThinkPad-T440 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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, > > > > > &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. > 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(&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
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 >
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