RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported sensors

Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
sensors

On 15/06/2023 06:04, Peng Fan wrote:

quoted

On 6/15/2023 10:53 AM, Sebastian Krzyszkowiak wrote:

quoted

Caution: This is an external email. Please take care when clicking
links or opening attachments. When in doubt, report the message using
the 'Report this email' button


On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:

quoted

On 6/8/2023 3:10 AM, Daniel Lezcano wrote:

quoted

[...]

Ok, I misunderstood. I thought that was for failing registered
thermal zone.

Would enabling the site in ops->change_mode do the trick ?

No. ops->change_mode not able to do the trick.

devm_thermal_of_zone_register->thermal_zone_device_enable
->thermal_zone_device_set_mode-

__thermal_zone_device_update.part.0

quoted

quoted

->__thermal_zone_get_temp

The thermal_zone_device_set_mode will call change_mode, if return
fail here, the thermal zone will fail to be registered.

Thanks,
Peng.

I think the idea is not to return a failure in ops->change_mode, but
to move enabling the site in REGS_TMR/REGS_V2_TMSR register from
qoriq_tmu_register_tmu_zone to ops->change_mode.

But qoriq_tmu_register_tmu_zone will finally call ops->change_mode.

And it is per zone, so we not able to enable TMR_ME here.

This way the site will be

quoted

enabled only for actually existing thermal zones, since those not
described in the device tree won't reach thermal_zone_device_enable.

No. The TMR_ME is the gate for all sites.

What about the following change on top of your series:

diff --git a/drivers/thermal/qoriq_thermal.c

b/drivers/thermal/qoriq_thermal.c index c710449b0c50..ecf88bf13762
100644

--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c

@@ -107,8 +107,6 @@ static int tmu_get_temp(struct thermal_zone_device

*tz, int *temp)
  	 */

  	regmap_read(qdata->regmap, REGS_TMR, &val);
-	if (!(val & TMR_ME))
-		return -EAGAIN;

  	if (regmap_read_poll_timeout(qdata->regmap,
  				     REGS_TRITSR(qsensor->id),

@@ -131,14 +129,40 @@ static int tmu_get_temp(struct

thermal_zone_device *tz, int *temp)
  	return 0;
  }

+static int qoriq_tmu_change_mode(struct thermal_zone_device *tz,
+				 enum thermal_device_mode mode)
+{
+	struct qoriq_sensor *qsensor = thermal_zone_device_priv(tz);
+	struct qoriq_tmu_data *qdata = qoriq_sensor_to_data(qsensor);
+	unsigned int site;
+	unsigned int value;
+	unsigned int mask;
+
+	if (qdata->ver == TMU_VER1) {
+		site = BIT(15 - qsensor->id);
+		mask = TMR_ME | TMR_ALPF | site;
+		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask
& ~site;
+		regmap_update_bits(qdata->regmap, REGS_TMR, mask,
value);
+	} else {
+		site = BIT(qsensor->id);
+		mask = TMR_ME | TMR_ALPF_V2 | site;
+		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask
& ~site;
+		regmap_update_bits(qdata->regmap, REGS_V2_TMSR,
mask, value);
+		regmap_write(qdata->regmap, REGS_TMR, TMR_ME |
TMR_ALPF_V2);

+	}
+
+	return 0;
+}
+
  static const struct thermal_zone_device_ops tmu_tz_ops = {
  	.get_temp = tmu_get_temp,
+	.change_mode = qoriq_tmu_change_mode,
  };

  static int qoriq_tmu_register_tmu_zone(struct device *dev,
  				       struct qoriq_tmu_data *qdata)
  {
-	int id, sites = 0;
+	int id;

  	for (id = 0; id < SITES_MAX; id++) {
  		struct thermal_zone_device *tzd;

@@ -158,25 +182,11 @@ static int qoriq_tmu_register_tmu_zone(struct

device *dev,
  			return ret;
  		}

-		if (qdata->ver == TMU_VER1)
-			sites |= 0x1 << (15 - id);
-		else
-			sites |= 0x1 << id;
-
  		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
  			dev_warn(dev,
  				 "Failed to add hwmon sysfs attributes\n");
  	}

-	if (sites) {
-		if (qdata->ver == TMU_VER1) {
-			regmap_write(qdata->regmap, REGS_TMR,
TMR_ME | TMR_ALPF | sites);
-		} else {
-			regmap_write(qdata->regmap, REGS_V2_TMSR,
sites);
-			regmap_write(qdata->regmap, REGS_TMR,
TMR_ME | TMR_ALPF_V2);
-		}
-	}
-
  	return 0;
  }


--
<https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
w.linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C5c60b05b18b
9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
C0%7C638224265339069492%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
7C%7C%7C&sdata=StV3sHe7fjGIMNjEYaDwiuTm8GpB6IDmoRhKoRpmUQY
%3D&reserved=0> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:
<https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
w.facebook.com%2Fpages%2FLinaro&data=05%7C01%7Cpeng.fan%40nxp.c
om%7C5c60b05b18b9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd
99c5c301635%7C0%7C0%7C638224265339069492%7CUnknown%7CTWFpb
GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
6Mn0%3D%7C3000%7C%7C%7C&sdata=AjK1yxq5PT60YfQRlxOBzlM2YXnhl6
9NWCQFRIUwVm0%3D&reserved=0> Facebook |
<https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
er.com%2F%23!%2Flinaroorg&data=05%7C01%7Cpeng.fan%40nxp.com%7C
5c60b05b18b9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c30
1635%7C0%7C0%7C638224265339069492%7CUnknown%7CTWFpbGZsb3d8
eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
D%7C3000%7C%7C%7C&sdata=ViShZhpql%2FJ6xUGJ2M9acJtmKuG%2BFCyZ
Ivy99cPpfxA%3D&reserved=0> Twitter |
<https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
w.linaro.org%2Flinaro-
blog%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C5c60b05b18b944222
3cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
38224265339069492%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
7C&sdata=PCNIqUJPZve9x1sgsdkPtRjIKQXnr514I%2BUx%2FbLcGC0%3D&res
erved=0> Blog