On Fri, Jul 27, 2018 at 03:45:06PM -0700, Eduardo Valentin wrote:

On Fri, Jul 27, 2018 at 03:40:52PM -0700, Eduardo Valentin wrote:

quoted

On Wed, Jul 25, 2018 at 06:12:28PM -0700, Matthias Kaehlcke wrote:

quoted

Hi Doug,

On Wed, Jul 25, 2018 at 04:19:56PM -0700, Doug Anderson wrote:

quoted

On Tue, Jul 24, 2018 at 4:46 PM, Matthias Kaehlcke [off-list ref] wrote:

quoted

+static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip,
+                                            int temp)
+{
+       u8 reg;
+       bool disable_s2_shutdown = false;
+       int ret;
+
+       WARN_ON(!mutex_is_locked(&chip->lock));
+
+       /*
+        * Default: S2 and S3 shutdown enabled, thresholds at
+        * 105C/125C/145C, monitoring at 25Hz
+        */
+       reg = SHUTDOWN_CTRL1_RATE_25HZ;
+
+       if ((temp == THERMAL_TEMP_INVALID) ||
+           (temp < STAGE2_THRESHOLD_MIN)) {
+               chip->thresh = THRESH_MIN;
+               goto skip;
+       }
+
+       if (temp <= STAGE2_THRESHOLD_MAX) {
+               chip->thresh = THRESH_MAX -
+                       ((STAGE2_THRESHOLD_MAX - temp) /
+                        TEMP_THRESH_STEP);
+               disable_s2_shutdown = true;
+       } else {
+               chip->thresh = THRESH_MAX;
+
+               if (!IS_ERR(chip->adc))
+                       disable_s2_shutdown = true;
+               else
+                       dev_warn(chip->dev,
+                                "No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140?C! Configuring stage 2 shutdown at 140?C.\n");

Putting a non-ASCII character (the degree symbol) in your commit
message is one thing, but are you sure it's wise to put it in the
kernel logs?

A few other drivers also do this
(drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c,
drivers/macintosh/windfarm_pm121.c), however that doesn't mean it's a
good idea. Will change to degC or C.

quoted

quoted

+       }
+
+skip:
+       reg |= chip->thresh;
+       if (disable_s2_shutdown)
+               reg |= SHUTDOWN_CTRL1_OVERRIDE_S2;
+
+       ret = qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
+       if (ret < 0)
+               return ret;
+
+       return ret;

Simplify the above lines to:

return qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);

Ouch, my code is indeed dumb ...

quoted

quoted

@@ -313,12 +441,7 @@ static int qpnp_tm_probe(struct platform_device *pdev)
        if (ret < 0)
                return ret;

-       chip->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, chip,
-                                                       &qpnp_tm_sensor_ops);
-       if (IS_ERR(chip->tz_dev)) {
-               dev_err(&pdev->dev, "failed to register sensor\n");
-               return PTR_ERR(chip->tz_dev);
-       }
+       chip->initialized = true;

Should we add "thermal_zone_device_update(chip->tz_dev,
THERMAL_EVENT_UNSPECIFIED);" here

Seems reasonable, will do.

quoted

...also: do we care about any type of locking for chip->initialized?
Technically we can be running on weakly ordered memory so if
qpnp_tm_update_temp_no_adc() is running on a different processor then
possibly it could still keep returning the default temperature for a
little while.  We could try to analyze whether there's some sort of
implicit barrier or we could add manual memory barriers, but generally
I try to avoid that and just do the simple locking...  What about just
setting chip-Initialized = true at the end of qpnp_tm_init() while the
mutex is still held?

Thanks for pointing that out. I agree that we should keep things
simple, chip->initialized to true at the end of qpnp_tm_init() sounds
good to me.

quoted

I'd also love to hear from someone with more thermal framework
experience to make sure it's legit to return a default value if
someone calls us while we're initting.  It seems sane to me but nice
to confirm it's OK.

An alternative could be to return THERMAL_TEMP_INVALID, however I
don't see this handled outside of thermal_core.c, not sure if it could
throw some other code off.

Comments from thermal folks on either approach (or alternatives) are
definitely welcome :)

quoted

Overall I like the idea of this patch so hopefully others do too.
Thanks for sending it out!

minor ask for next version


WARNING: line over 80 characters
#159: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:65:
+#define STAGE2_THRESHOLD_MIN		125000	/* Stage 2 Threshold
Min: 125 C */

WARNING: line over 80 characters
#160: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:66:
+#define STAGE2_THRESHOLD_MAX		140000	/* Stage 2 Threshold
Max: 140 C */

ERROR: trailing statements should be on next line
#201: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:186:
+	if (!chip->adc)) {

CHECK: Unnecessary parentheses around 'temp == THERMAL_TEMP_INVALID'
#227: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:220:
+	if ((temp == THERMAL_TEMP_INVALID) ||
+	    (temp < STAGE2_THRESHOLD_MIN)) {

CHECK: Unnecessary parentheses around 'temp < STAGE2_THRESHOLD_MIN'
#227: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:220:
+	if ((temp == THERMAL_TEMP_INVALID) ||
+	    (temp < STAGE2_THRESHOLD_MIN)) {

CHECK: Unnecessary parentheses around 'trips[i].type ==
THERMAL_TRIP_CRITICAL'
#305: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:302:
+		if (of_thermal_is_trip_valid(chip->tz_dev, i) &&
+		    (trips[i].type == THERMAL_TRIP_CRITICAL))

CHECK: Alignment should match open parenthesis
#386: FILE: drivers/thermal/qcom-spmi-temp-alarm.c:427:
+	chip->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev,
0, chip,
+
&qpnp_tm_sensor_ops);

And it would be great if you could combine these two in your a single
series, say when you fix this patch and send a new version of this
series, please include these too:
https://patchwork.kernel.org/patch/10543335/
https://patchwork.kernel.org/patch/10543333/

Ok, will do