| Message ID | 20210219181032.3.Ia4c1022191d09fe8c56a16486b77796b83ffcae4@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] arm64: dts: qcom: sc7180: Add lazor rev4 | expand |
Quoting Matthias Kaehlcke (2021-02-19 18:10:59) > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > the charger temperature which currently isn't supported by the > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > to avoid the use of bogus temperature values. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ > 3 files changed, 27 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > index 30e3e769d2b4..0974dbd424e1 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > @@ -14,6 +14,15 @@ / { > compatible = "google,lazor-rev0", "qcom,sc7180"; > }; > > +/* > + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently > + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC > + * channel to avoid the use of bogus temperature values. > + */ > +/delete-node/ &charger_thermal; > +/delete-node/ &pm6150_adc_charger_thm; > +/delete-node/ &pm6150_adc_tm_charger_thm; Can we disable pm6150_adc_tm instead on <= rev3 boards? It would be the same number of lines, but is simpler to reason about disabled nodes vs. deleted nodes usually. > + > &pp3300_hub { > /* pp3300_l7c is used to power the USB hub */ > /delete-property/regulator-always-on;
On Mon, Feb 22, 2021 at 12:20:04PM -0800, Stephen Boyd wrote: > Quoting Matthias Kaehlcke (2021-02-19 18:10:59) > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > > the charger temperature which currently isn't supported by the > > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > > to avoid the use of bogus temperature values. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ > > 3 files changed, 27 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > index 30e3e769d2b4..0974dbd424e1 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > @@ -14,6 +14,15 @@ / { > > compatible = "google,lazor-rev0", "qcom,sc7180"; > > }; > > > > +/* > > + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently > > + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC > > + * channel to avoid the use of bogus temperature values. > > + */ > > +/delete-node/ &charger_thermal; > > +/delete-node/ &pm6150_adc_charger_thm; > > +/delete-node/ &pm6150_adc_tm_charger_thm; > > Can we disable pm6150_adc_tm instead on <= rev3 boards? It would be the > same number of lines, but is simpler to reason about disabled nodes vs. > deleted nodes usually. For Lazor theoretically this could be done since it doesn't use other ADC channels, however it won't work for other trogdor devices that will be upstreamed eventually. Some of these boards have the same problem, however they have other thermistors connected to the ADC. One could argue that it's preferable to do things in a uniform way, but I'm open to do it either way for Lazor.
Quoting Matthias Kaehlcke (2021-02-22 12:38:46) > On Mon, Feb 22, 2021 at 12:20:04PM -0800, Stephen Boyd wrote: > > Quoting Matthias Kaehlcke (2021-02-19 18:10:59) > > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > > > the charger temperature which currently isn't supported by the > > > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > > > to avoid the use of bogus temperature values. > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ > > > 3 files changed, 27 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > index 30e3e769d2b4..0974dbd424e1 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > @@ -14,6 +14,15 @@ / { > > > compatible = "google,lazor-rev0", "qcom,sc7180"; > > > }; > > > > > > +/* > > > + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently > > > + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC > > > + * channel to avoid the use of bogus temperature values. > > > + */ > > > +/delete-node/ &charger_thermal; > > > +/delete-node/ &pm6150_adc_charger_thm; > > > +/delete-node/ &pm6150_adc_tm_charger_thm; > > > > Can we disable pm6150_adc_tm instead on <= rev3 boards? It would be the > > same number of lines, but is simpler to reason about disabled nodes vs. > > deleted nodes usually. > > For Lazor theoretically this could be done since it doesn't use other ADC > channels, however it won't work for other trogdor devices that will be > upstreamed eventually. Some of these boards have the same problem, however > they have other thermistors connected to the ADC. One could argue that it's > preferable to do things in a uniform way, but I'm open to do it either way > for Lazor. > I see. Can the thermal-zone be disabled then vs. deleting three nodes? I think the thermal driver uses for_each_available_child_of_node() so that would work?
Hi, On Mon, Feb 22, 2021 at 12:45 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Matthias Kaehlcke (2021-02-22 12:38:46) > > On Mon, Feb 22, 2021 at 12:20:04PM -0800, Stephen Boyd wrote: > > > Quoting Matthias Kaehlcke (2021-02-19 18:10:59) > > > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > > > > the charger temperature which currently isn't supported by the > > > > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > > > > to avoid the use of bogus temperature values. > > > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > --- > > > > > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ > > > > 3 files changed, 27 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > index 30e3e769d2b4..0974dbd424e1 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > @@ -14,6 +14,15 @@ / { > > > > compatible = "google,lazor-rev0", "qcom,sc7180"; > > > > }; > > > > > > > > +/* > > > > + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently > > > > + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC > > > > + * channel to avoid the use of bogus temperature values. > > > > + */ > > > > +/delete-node/ &charger_thermal; > > > > +/delete-node/ &pm6150_adc_charger_thm; > > > > +/delete-node/ &pm6150_adc_tm_charger_thm; > > > > > > Can we disable pm6150_adc_tm instead on <= rev3 boards? It would be the > > > same number of lines, but is simpler to reason about disabled nodes vs. > > > deleted nodes usually. > > > > For Lazor theoretically this could be done since it doesn't use other ADC > > channels, however it won't work for other trogdor devices that will be > > upstreamed eventually. Some of these boards have the same problem, however > > they have other thermistors connected to the ADC. One could argue that it's > > preferable to do things in a uniform way, but I'm open to do it either way > > for Lazor. > > > > I see. Can the thermal-zone be disabled then vs. deleting three nodes? I > think the thermal driver uses for_each_available_child_of_node() so that > would work? FWIW: +1 to what Stephen suggests assuming it works. -Doug
Hi, On Sat, 20 Feb 2021 at 05:13, Matthias Kaehlcke <mka@chromium.org> wrote: > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > the charger temperature which currently isn't supported by the > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > to avoid the use of bogus temperature values. Should we just expand the adc/adc-tm drivers with additional calibration tables?
On Tue, Feb 23, 2021 at 02:12:30PM +0300, Dmitry Baryshkov wrote: > Hi, > > On Sat, 20 Feb 2021 at 05:13, Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > > the charger temperature which currently isn't supported by the > > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > > to avoid the use of bogus temperature values. > > Should we just expand the adc/adc-tm drivers with additional calibration tables? Generally that seems desirable, I'm not sure about the process, I guess someone with access to a climate chamber would have to create these tables? I think it would also require an extension of the DT bindings, currently the ADC driver assumes that a 100k NTC is connected, something in the DT would have to indicate the thermistor type. We want to remove the bogus temperatures from the system now, if support for 47k NTCs is added at some point we can consider changing the DT again.
On Mon, Feb 22, 2021 at 12:45:23PM -0800, Stephen Boyd wrote: > Quoting Matthias Kaehlcke (2021-02-22 12:38:46) > > On Mon, Feb 22, 2021 at 12:20:04PM -0800, Stephen Boyd wrote: > > > Quoting Matthias Kaehlcke (2021-02-19 18:10:59) > > > > Lazor rev3 and older are stuffed with a 47k NTC as thermistor for > > > > the charger temperature which currently isn't supported by the > > > > PM6150 ADC driver. Delete the charger thermal zone and ADC channel > > > > to avoid the use of bogus temperature values. > > > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > --- > > > > > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ > > > > 3 files changed, 27 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > index 30e3e769d2b4..0974dbd424e1 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts > > > > @@ -14,6 +14,15 @@ / { > > > > compatible = "google,lazor-rev0", "qcom,sc7180"; > > > > }; > > > > > > > > +/* > > > > + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently > > > > + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC > > > > + * channel to avoid the use of bogus temperature values. > > > > + */ > > > > +/delete-node/ &charger_thermal; > > > > +/delete-node/ &pm6150_adc_charger_thm; > > > > +/delete-node/ &pm6150_adc_tm_charger_thm; > > > > > > Can we disable pm6150_adc_tm instead on <= rev3 boards? It would be the > > > same number of lines, but is simpler to reason about disabled nodes vs. > > > deleted nodes usually. > > > > For Lazor theoretically this could be done since it doesn't use other ADC > > channels, however it won't work for other trogdor devices that will be > > upstreamed eventually. Some of these boards have the same problem, however > > they have other thermistors connected to the ADC. One could argue that it's > > preferable to do things in a uniform way, but I'm open to do it either way > > for Lazor. > > > > I see. Can the thermal-zone be disabled then vs. deleting three nodes? I > think the thermal driver uses for_each_available_child_of_node() so that > would work? Yes, that would work. I also deleted the ADC/TM nodes to remove the bogus temperature completely from the system, but one could argue that it does no harm to keep it as long as it isn't used.
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts index 30e3e769d2b4..0974dbd424e1 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts @@ -14,6 +14,15 @@ / { compatible = "google,lazor-rev0", "qcom,sc7180"; }; +/* + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC + * channel to avoid the use of bogus temperature values. + */ +/delete-node/ &charger_thermal; +/delete-node/ &pm6150_adc_charger_thm; +/delete-node/ &pm6150_adc_tm_charger_thm; + &pp3300_hub { /* pp3300_l7c is used to power the USB hub */ /delete-property/regulator-always-on; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts index c2ef06367baf..0381ca85ae97 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts @@ -14,6 +14,15 @@ / { compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180"; }; +/* + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC + * channel to avoid the use of bogus temperature values. + */ +/delete-node/ &charger_thermal; +/delete-node/ &pm6150_adc_charger_thm; +/delete-node/ &pm6150_adc_tm_charger_thm; + &pp3300_hub { /* pp3300_l7c is used to power the USB hub */ /delete-property/regulator-always-on; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts index 240c3e067fac..b9473bba8f4a 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts @@ -13,3 +13,12 @@ / { model = "Google Lazor (rev3)"; compatible = "google,lazor-rev3", "qcom,sc7180"; }; + +/* + * rev <= 3 are stuffed with a 47k NTC as charger thermistor which is currently + * not supported by the PM6150 ADC driver. Delete the thermal zone and ADC + * channel to avoid the use of bogus temperature values. + */ +/delete-node/ &charger_thermal; +/delete-node/ &pm6150_adc_charger_thm; +/delete-node/ &pm6150_adc_tm_charger_thm;
Lazor rev3 and older are stuffed with a 47k NTC as thermistor for the charger temperature which currently isn't supported by the PM6150 ADC driver. Delete the charger thermal zone and ADC channel to avoid the use of bogus temperature values. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 +++++++++ arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 +++++++++ arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 +++++++++ 3 files changed, 27 insertions(+)