2019-11-21 07:03:37

by Hsin-Yi Wang

Subject: Re: [PATCH v2 7/8] thermal: mediatek: add another get_temp ops for thermal sensors

On Fri, May 10, 2019 at 9:27 PM michael.kao <[email protected]> wrote:

> - tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, mt,
> - &mtk_thermal_ops);
> - if (IS_ERR(tzdev)) {
> - ret = PTR_ERR(tzdev);
> - goto err_disable_clk_peri_therm;
> + for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> + tz = kmalloc(sizeof(*tz), GFP_KERNEL);
> + if (!tz)
> + return -ENOMEM;
> +
> + tz->mt = mt;
> + tz->id = i;
> +
> + tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, i,
> + tz, (i == 0) ?
> + &mtk_thermal_ops : &mtk_thermal_sensor_ops);
> +
> + if (IS_ERR(tzdev)) {
> + if (IS_ERR(tzdev) != -EACCES) {
PTR_ERR(tzdev)

> + ret = PTR_ERR(tzdev);
> + goto err_disable_clk_peri_therm;
> + }
> + }

This for loop adding thermal zone sensors will not work for mt8173. It
assumes that thermal-zones in dts have subnodes (eg. cpu_thermal,
tzts..) amount equal to num_sensors+1. Otherwise tzdev would be
-ENODEV and thermal failed to be probed.
In mt8183 this is fine, since each thermal zone only has one sensor,
but in mt8173, some sensor appears in multiple thermal zones.

In order to let the change also works for 8173, I think if the error
is -ENODEV, and the id is not 0 (0 is cpu_thermal), prompt a warning
instead of failing. Eg.

if (IS_ERR(tzdev)) {
+ if (i > 0 && PTR_ERR(tzdev) == -ENODEV) {
+ dev_warn(&pdev->dev, "can't find
thermal sensor %d\n", i);
+ continue;
+ }
if (PTR_ERR(tzdev) != -EACCES) {

2019-12-06 09:36:16

by Michael Kao

Subject: Re: [PATCH v2 7/8] thermal: mediatek: add another get_temp ops for thermal sensors

On Thu, 2019-11-21 at 15:00 +0800, Hsin-Yi Wang wrote:
> On Fri, May 10, 2019 at 9:27 PM michael.kao <[email protected]> wrote:
>
> > - tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, mt,
> > - &mtk_thermal_ops);
> > - if (IS_ERR(tzdev)) {
> > - ret = PTR_ERR(tzdev);
> > - goto err_disable_clk_peri_therm;
> > + for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> > + tz = kmalloc(sizeof(*tz), GFP_KERNEL);
> > + if (!tz)
> > + return -ENOMEM;
> > +
> > + tz->mt = mt;
> > + tz->id = i;
> > +
> > + tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, i,
> > + tz, (i == 0) ?
> > + &mtk_thermal_ops : &mtk_thermal_sensor_ops);
> > +
> > + if (IS_ERR(tzdev)) {
> > + if (IS_ERR(tzdev) != -EACCES) {
> PTR_ERR(tzdev)
>
> > + ret = PTR_ERR(tzdev);
> > + goto err_disable_clk_peri_therm;
> > + }
> > + }
>
> This for loop adding thermal zone sensors will not work for mt8173. It
> assumes that thermal-zones in dts have subnodes (eg. cpu_thermal,
> tzts..) amount equal to num_sensors+1. Otherwise tzdev would be
> -ENODEV and thermal failed to be probed.
> In mt8183 this is fine, since each thermal zone only has one sensor,
> but in mt8173, some sensor appears in multiple thermal zones.
>
> In order to let the change also works for 8173, I think if the error
> is -ENODEV, and the id is not 0 (0 is cpu_thermal), prompt a warning
> instead of failing. Eg.
>
> if (IS_ERR(tzdev)) {
> + if (i > 0 && PTR_ERR(tzdev) == -ENODEV) {
> + dev_warn(&pdev->dev, "can't find
> thermal sensor %d\n", i);
> + continue;
> + }
> if (PTR_ERR(tzdev) != -EACCES) {

OK, I will update this patch.