Hi, On Tue, Jul 17, 2018 at 2:08 PM, Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote: > When the temperature reaches stage 2 the PMIC performs by default a > 'partial shutdown', unless software override is enabled. It is not well > defined which peripherals are affected by a 'partial shutdown'. Drivers > might be unhappy when their devices suddenly disappear and prevent an > orderly shutdown. > > Add an optional device tree property that allows to disable stage 2 > shutdown. > > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx> > --- > Changes in v4: > - patch added to the series > --- > .../bindings/thermal/qcom-spmi-temp-alarm.txt | 3 ++ > drivers/thermal/qcom-spmi-temp-alarm.c | 28 +++++++++++++------ > 2 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt > index 290ec06fa33a..377c94fa1821 100644 > --- a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt > +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt > @@ -15,6 +15,8 @@ Optional properties: > - io-channels: Should contain IIO channel specifier for the ADC channel, > which report chip die temperature. > - io-channel-names: Should contain "thermal". > +- stage2-shutdown-disabled: boolean to disable a partial shutdown of the PMIC > + when the temperature reaches stage 2 With my understanding of device tree I believe that this should be "qcom,stage2-shutdown-disabled". If something isn't a generic property affecting a whole group of bindings I believe it's supposed to have a company prefix. >From the device tree specification (wow, this exists now? ...and is public?) https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2 I find: > Nonstandard property names should specify a unique string prefix, such > as a stock ticker symbol, identifying the name of the company or > organization that defined the property. Examples: fsl,channel-fifo-len ibm,ppc-interrupt-server#s linux,network-index ===== I would also perhaps add a bit more verbose justification to your description of this property. Specifically there should be some indication in the bindings doc of when you'd want this property. From what David Collins said, I'd maybe write something like: --- You want to disable the partial "stage 2" shutdown any time you've properly defined thermal zones in such a way that the OS will try to shut down at that stage anyway. Said another way, there are three thermal stages defined in the PMIC: stage 1: warning stage 2: system should shut down stage 3: emergency shut down By default the PMIC assumes that the OS isn't doing anything and thus at stage 2 it does the partial PMIC shutdown and at stage 3 it kills all power. If we have thermal zones defined such that the OS will initiate the shutdown at stage 2 then we want to disable the PMIC's automatic mode for stage 2. That still leaves us with the emergency at stage 3. === Actually, after writing all the above I wonder if perhaps there's a way to do this all automatically. AKA: if someone has specified a "critical" level can we automatically disable the partial PMIC shutdown and we don't need the extra property? Bonus points if we can check to see if the thermal zones defined actually match reality and even more bonus points if you can automatically adjust the settings in the PMIC based on the thermal zones... I did find that in "struct thermal_zone_of_device_ops" there appears to be a "set_trip_temp" function you can fill in. When I tried that quickly I saw that I got called when I echoed into "/sys/class/thermal/thermal_zone0/trip_point_1_temp" but not at bootup, but I didn't debug more than that... > Example: > > @@ -23,6 +25,7 @@ Example: > reg = <0x2400 0x100>; > interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>; > #thermal-sensor-cells = <0>; > + stage2-shutdown-disabled; > > io-channels = <&pm8941_vadc VADC_DIE_TEMP>; > io-channel-names = "thermal"; BTW: can you update the "example" in this file? Based on the PMIC docs I saw it's likely that it should be: stage1 { temperature = <1050000>; hysteresis = <2000>; type = "passive"; }; stage2 { temperature = <125000>; hysteresis = <2000>; type = "critical"; }; ...and I guess we don't specify the final one because by that time Linux is kaput. > diff --git a/drivers/thermal/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom-spmi-temp-alarm.c > index ad4f3a8d6560..acbb0dbec79e 100644 > --- a/drivers/thermal/qcom-spmi-temp-alarm.c > +++ b/drivers/thermal/qcom-spmi-temp-alarm.c Sometimes people like bindings and code change to be separate patches. Maintainer can specify what's desired in this case, but I find it generally pleases more people to make them separate. ...though I guess if we can figure out how to do this automatically we will have no bindings change... -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
