Re: [PATCH v3 11/12] arm64: dts: qcom: sm6350-lena: Add PM6150L thermals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: wrong io-channels specifier on rear_cam_sensor
- [Low] arm64: dts: qcom: misplaced xo-thermal, unused includes, and style issues
--

commit ac86db420f5582034afb81e70777f975abaf47c9
Author: erebion <erebion@erebion.eu>

arm64: dts: qcom: sm6350-lena: Add PM6150L thermals

This commit adds ADC channels, ADC_TM channels, generic-adc-thermal sensors,
and thermal zones for the thermistors connected to PM6150L on the Sony Xperia
10 III (lena).

> diff --git a/arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts b/arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts
> index 4e9707712a651..487ef75f2f1de 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts
> +++ b/arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts
> @@ -12,7 +12,9 @@
>  #define PMK8350_SID 6
>  
>  #include <dt-bindings/firmware/qcom,scm.h>
> +#include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
> +#include <dt-bindings/input/input.h>
>  #include <dt-bindings/leds/common.h>
>  #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> @@ -70,6 +72,20 @@ lcdb_dummy: regulator-lcdb-dummy {
>  		regulator-boot-on;
>  	};
>  
> +	msm_therm_sensor: thermal-sensor-msm {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&pm6150l_adc ADC5_AMUX_THM2_100K_PU>;
> +		io-channel-names = "sensor-channel";
> +	};
> +
> +	rear_cam_sensor: thermal-sensor-rear-cam {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&pm6150l_adc ADC5_GPIO6_100K_PU>;
> +		io-channel-names = "sensor-channel";
> +	};

[Severity: Medium]
Should rear_cam_sensor reference ADC5_GPIO2_100K_PU instead of
ADC5_GPIO6_100K_PU here?

On PM6150L, physical pin gpio6 is routed to ADC5_GPIO2_100K_PU (0x53), and
under &pm6150l_adc below the rear camera thermistor channel is defined as:

	channel@53 {
		reg = <ADC5_GPIO2_100K_PU>;
		label = "rear_cam_therm";

When gadc_thermal_probe() calls devm_iio_channel_get() for rear_cam_sensor,
adc5_fwnode_xlate() in drivers/iio/adc/qcom-spmi-adc5.c searches the channels
registered under &pm6150l_adc for a matching channel number:

	for (i = 0; i < adc->nchannels; i++)
		if (adc->chan_props[i].channel == iiospec->args[0])
			return i;

	return -EINVAL;

Because no channel is registered for ADC5_GPIO6_100K_PU (and adc5_chans_pmic[]
only defines 100K pull-up GPIO channels up to ADC5_GPIO4_100K_PU), probing
rear_cam_sensor fails with -EINVAL and leaves rear-cam-thermal non-functional.

[ ... ]

> @@ -115,6 +131,99 @@ active-config0 {

[ ... ]

> +		xo-thermal {
> +			thermal-sensors = <&pmk8350_adc_tm 0>;
> +
> +			trips {
> +				active-config0 {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "passive";
> +				};
> +			};
> +		};
>  	};
>  };
>  
> @@ -392,6 +501,99 @@ &mpss {
>  	status = "okay";
>  };
>  
> +&pm6150l_gpios {
> +	pm6150l_adc_default: adc-default-state {
> +		pins = "gpio6", "gpio7", "gpio10";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +		bias-high-impedance;
> +	};
> +};
> +
> +&pm6150l_adc {

[ ... ]

> +&pm6150l_adc_tm {
> +	status = "okay";
> +
> +	pa-therm1@0 {
> +		reg = <0>;
> +		io-channels = <&pm6150l_adc ADC5_AMUX_THM1_100K_PU>;
> +		qcom,hw-settle-time-us = <200>;
> +		qcom,ratiometric;
> +	};
> +
> +	pa-therm0@1 {
> +		reg = <1>;
> +		io-channels = <&pm6150l_adc ADC5_AMUX_THM3_100K_PU>;
> +		qcom,hw-settle-time-us = <200>;
> +		qcom,ratiometric;
> +	};
> +
> +	rear-cam-flash-therm@2 {
> +		reg = <2>;
> +		io-channels = <&pm6150l_adc ADC5_GPIO3_100K_PU>;
> +		qcom,hw-settle-time-us = <200>;
> +		qcom,ratiometric;
> +		};

[Severity: Low]
This isn't a runtime bug, but there are a few patch split and style issues
across this change:

1. Earlier in the series, commit ff24120a08d6 ("arm64: dts: qcom: sm6350-lena:
Add PMK8350 thermals") added xo-thermal, and the next commit f17bc49f693d
("arm64: dts: qcom: sm6350-lena: Add PM7250B thermals") accidentally removed
it. Re-adding xo-thermal here puts a PMK8350 thermal zone into the PM6150L
thermal patch. Could xo-thermal be kept in the PMK8350 patch instead?

2. The newly added dt-bindings/gpio/gpio.h and dt-bindings/input/input.h
includes at the top of the file do not appear to be used by this patch.

3. Label references at the bottom of the file are sorted alphabetically, so
should &pm6150l_gpios be placed after &pm6150l_flash rather than before
&pm6150l_adc?

4. The closing brace of rear-cam-flash-therm@2 has an extra tab of
indentation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-pdx213-upstream-3-v3-0-cdc3be96d257@erebion.eu?part=11