On Fri, Jun 5, 2020 at 10:10 AM Vinod Koul <vkoul@xxxxxxxxxx> 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
