On Fri, Jun 5, 2020 at 10:10 AM Vinod Koul vkoul@kernel.org wrote:
Sorry missed ccing Amit, done now.
On 04-06-20, 18:03, Dmitry Baryshkov wrote:
On 04/06/2020 13:47, Vinod Koul wrote:
On 04-06-20, 03:43, Dmitry Baryshkov wrote:
pm8150_adc: adc@3100 { compatible = "qcom,spmi-adc5"; reg = <0x3100>;@@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { #io-channel-cells = <1>; interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
status = "disabled";This should not be removed, rather than this please add enabled in you board dts file
Is the default disabled for a reason?
I'd expect the default to be enabled and then board-specific dts to disable a specific adc if needed.
...
+&thermal_zones {
- pm8150_temp {
polling-delay-passive = <0>;polling-delay = <0>;thermal-sensors = <&pm8150_temp>;trips {trip0 {temperature = <95000>;hysteresis = <0>;type = "passive";};trip1 {temperature = <115000>;hysteresis = <0>;type = "passive";
Since there is not cooling map associated with these two trips i.e. no mitigation action, this trip is informational. So make it of type "hot".
Is there really a need for two passive trip points? Just one at 115 should be enough?
};trip2 {temperature = <145000>;
Are you sure about this? That is a very toasty temperature. :-)
hysteresis = <0>;type = "passive";
The last trip should typically be of type "critical". That is the temperature at which the system will initiate a shutdown.
};};- };
Not sure about this, Amit..? Should this also not be in board dts?
Similar comments on similar ones for rest of the patch as well..
I'm not so sure. This part of the configuration seems generic to me. Unlike adc-tm config, which definitely goes to the board file.
I think the temperature values may be board specific, Amit can confirm that. If that is the case then this belongs to board dts, otherwise here :)
While the temp values can be board-specific e.g. if the same SoC is used in a mobile phone and a laptop, the thresholds rarely change, in my experience.
I think they can stay in the pmic dtsi file and any specific board can override if necessary.
I can split this into a separate pm8150-temp.dtsi file. Does that sound better?
That might make it worse, we don't do splitting.
Right, let's not split it.
-- ~Vinod