Re: [PATCH v2 4/4] arm64: dts: qcom: sdm845: mtp: Add vadc channels and thermal zones
From: Thara Gopinath
Date: Tue Oct 05 2021 - 18:21:09 EST
On 10/5/21 5:52 PM, Bjorn Andersson wrote:
On Tue 05 Oct 13:30 PDT 2021, Thara Gopinath wrote:
On 10/4/21 4:56 PM, Dmitry Baryshkov wrote:
On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
On Thu, Sep 23, 2021 at 02:23:11PM -0700, Bjorn Andersson wrote:
Downstream defines four ADC channels related to thermal sensors external
to the PM8998 and two channels for internal voltage measurements.
Add these to the upstream SDM845 MTP, describe the thermal monitor
channels and add thermal_zones for these.
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
---
In addition to the iio channels exposed by v1, Daniel wanted thermal_zones...
Changes since v1:
- Enable the pm8998_adc_tm and describe the ADC channels
- Add thermal-zones for the new channels
arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 128 ++++++++++++++++++++++++
1 file changed, 128 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
[..]
+&pm8998_adc {
+ adc-chan@4c {
+ reg = <ADC5_XO_THERM_100K_PU>;
+ label = "xo_therm";
+ };
+
+ adc-chan@4d {
+ reg = <ADC5_AMUX_THM1_100K_PU>;
+ label = "msm_therm";
+ };
+
+ adc-chan@4f {
+ reg = <ADC5_AMUX_THM3_100K_PU>;
+ label = "pa_therm1";
+ };
+
+ adc-chan@51 {
+ reg = <ADC5_AMUX_THM5_100K_PU>;
+ label = "quiet_therm";
+ };
+
+ adc-chan@83 {
+ reg = <ADC5_VPH_PWR>;
+ label = "vph_pwr";
+ };
+
+ adc-chan@85 {
+ reg = <ADC5_VCOIN>;
+ label = "vcoin";
+ };
+};
+
+&pm8998_adc_tm {
+ status = "okay";
+
+ xo-thermistor@1 {
+ reg = <1>;
+ io-channels = <&pm8998_adc ADC5_XO_THERM_100K_PU>;
+ qcom,ratiometric;
+ qcom,hw-settle-time-us = <200>;
+ };
+
+ msm-thermistor@2 {
+ reg = <2>;
+ io-channels = <&pm8998_adc ADC5_AMUX_THM1_100K_PU>;
+ qcom,ratiometric;
+ qcom,hw-settle-time-us = <200>;
+ };
+
+ pa-thermistor@3 {
+ reg = <3>;
+ io-channels = <&pm8998_adc ADC5_AMUX_THM3_100K_PU>;
+ qcom,ratiometric;
+ qcom,hw-settle-time-us = <200>;
+ };
+
+ quiet-thermistor@4 {
+ reg = <4>;
+ io-channels = <&pm8998_adc ADC5_AMUX_THM5_100K_PU>;
+ qcom,ratiometric;
+ qcom,hw-settle-time-us = <200>;
+ };
+};
+
The example in the 'qcom,spmi-adc-tm5' binding specifies 'qcom,ratiometric'
and 'qcom,hw-settle-time-us' for both the ADC and the thermal monitor, so do
several board files (e.g. sm8250-mtp.dts and qrb5165-rb5.dts). This apparent
redundancy bothered me earlier, it's not really clear to me whether it's
needed/recommended or not. Do you happen to have any insights on this?
Hmm, you're right and I missed this in defining my channels. I've not
looked at this detail, just got reasonable readings from my thermal
zones and was happy about that.
Dmitry, do you have any further insights why these properties are
supposed to be duplicated between the adc channel and the thermal zones?
Because both ADC channel and thermal zone registers should be
programmed accordingly.
One not-so-perfect approach would be to use io-channels property to
locate the adc's adc-chan node and to parse it. However this way
thermal driver would have to know the exact structure of adc's device
tree nodes.
Another (even worse) solution would be to introduce qcom-specific API
to query these properties from the IIO channel.
Selecting between these two options I decided to follow the downstream
path and just to duplicate these few properties.
I know I am joining late to this party, since v3 is out. But before I send
my acked-by, wanted to check/state a few things.
To me, the not-so-perfect approach that you mention here is actually not so
bad, considering the thermal sensor driver already is aware of internals of
the adc device. The duplicating of node is kind of confusing.
Also did you explore using one of _channel_write apis in iio framework to
move all the writes into the adc device instead of thermal sensor driver ?
To me the TM hardware block seems to just duplicate some parts of the
ADC block. Without understanding why the two would need to be configured
in sync, I don't think we should tie them together further.
You are right. It is duplicating most of the ADC parts except for
temperature read for which it calls into the adc device. And I don't see
any interface from the ADC driver that can be used to do the
configuration TM needs. Hence the question, have we ever explored this
path ?
In particular, I get the same readings with or without configuring the
ADC channels, does this perhaps imply that the TM configuration is used
to configure the ADC during it's reads (i.e. we don't need to define the
ADC channels after all), or is this just a coincidence?
Maybe because of what we do in adc_tm5_init ?
If needed, we should be able to duplicate the read in the TM as well and
get rid of ADC channels , right ? I don't like this half configuration
in TM and half in ADC device unless there is a reason behind this.
There are other things in the PMIC, using the ADC seemingly without you
having to explicitly configure the ADC and there's definitely things
pointing to the ADC being "timeshared" between various users.
Regards,
Bjorn
--
Warm Regards
Thara (She/Her/Hers)