Re: [PATCH v3 12/12] arm64: dts: qcom: sm6350-lena: enable remaining i2c busses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: sm6350-lena: unrelated and incomplete GPU enablement
- [Medium] arm64: dts: qcom: sm6350-lena: wrong ADC channel in rear_cam_sensor
- [Medium] arm64: dts: qcom: sm6350-lena: invalid led-max-microamp in pm6150l_flash
- [Low] arm64: dts: qcom: sm6350-lena: undocumented unused lcdb_dummy regulator
--

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

arm64: dts: qcom: sm6350-lena: enable remaining i2c busses

This series enables peripheral, PMIC, thermal, LED, regulator, and bus nodes in the device tree for the Sony Xperia 10 III (sm6350-sony-xperia-lena-pdx213).

> 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 6e2bbf4f060ac..d2b3aa6e61ab6 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
[ ... ]
> @@ -44,7 +63,30 @@ key-volume-down {
>  		};
>  	};
>  
> -	touch_en_vreg: touch-en-regulator {
> +	lcdb_dummy: regulator-lcdb-dummy {
> +		compatible = "regulator-fixed";
> +		regulator-name = "lcdb_dummy";
> +		regulator-min-microvolt = <5500000>;
> +		regulator-max-microvolt = <5500000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};

[Severity: Low]
This isn't a bug, but commit e7731bb19fd5 ("arm64: dts: qcom: sm6350-lena:
rename touch-en-regulator") states that it only renames touch-en-regulator
to regulator-touch-en for sorting, while also adding this lcdb_dummy
regulator node.

Nothing in sm6350-sony-xperia-lena-pdx213.dts or sm6350.dtsi references
&lcdb_dummy. Was this node added by mistake, or should it be dropped until a
consumer uses it?

> +
> +	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]
Does rear_cam_sensor fail to probe with -EINVAL here?

Under &pm6150l_adc below, the rear_cam_therm channel is defined on
channel@53 with reg = <ADC5_GPIO2_100K_PU> (since GPIO6 on PM6150L routes to
ADC5_GPIO2_100K_PU):

	channel@53 {
		reg = <ADC5_GPIO2_100K_PU>;
		label = "rear_cam_therm";
		qcom,hw-settle-time = <200>;
		qcom,pre-scaling = <1 1>;
		qcom,ratiometric;
	};

However, rear_cam_sensor requests io-channels = <&pm6150l_adc
ADC5_GPIO6_100K_PU>. When gadc_thermal_probe() requests the channel,
adc5_fwnode_xlate() in drivers/iio/adc/qcom-spmi-adc5.c searches the
configured channels on &pm6150l_adc:

	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 ADC5_GPIO1_100K_PU through
ADC5_GPIO4_100K_PU), adc5_fwnode_xlate() returns -EINVAL and leaves
rear-cam-thermal without a working sensor.

Should rear_cam_sensor use ADC5_GPIO2_100K_PU instead of ADC5_GPIO6_100K_PU?

[ ... ]
> @@ -285,6 +470,21 @@ &gpi_dma1 {
>  	status = "okay";
>  };
>  
> +&gpu {
> +	status = "okay";
> +};
> +
> +&gpu_zap_shader {
> +	firmware-name = "qcom/sm6350/sony/pdx213/a615_zap.mbn";
> +};

[Severity: Medium]
Was enabling &gpu and &gpu_zap_shader intended to be part of commit
dab0552e769e ("arm64: dts: qcom: sm6350-lena: enable remaining i2c busses")?

In addition to not matching the commit description, &mdss in sm6350.dtsi
remains disabled:

		mdss: display-subsystem@ae00000 {
			...
			status = "disabled";

			mdss_mdp: display-controller@ae01000 {
				compatible = "qcom,sm6350-dpu";

With the default drm/msm configuration (separate_gpu_kms = false),
adreno_probe() in drivers/gpu/drm/msm/adreno/adreno_device.c registers a
component and waits for the KMS master on mdss_mdp:

static int adreno_probe(struct platform_device *pdev)
{
	if (of_device_is_compatible(pdev->dev.of_node, "amd,imageon") ||
	    msm_gpu_no_components())
		return msm_gpu_probe(pdev, &a3xx_ops);

	return component_add(&pdev->dev, &a3xx_ops);
}

Because &mdss is disabled, the component master never probes and the GPU
component never binds (and if adreno_remove() runs while unbound,
platform_get_drvdata(pdev) is NULL and dereferencing priv->kms_init crashes).

Should the &gpu and &gpu_zap_shader changes be dropped from this I2C patch?

[ ... ]
> +&pm6150l_flash {
> +	status = "okay";
> +
> +	led-0 {
> +		function = LED_FUNCTION_FLASH;
> +		color = <LED_COLOR_ID_WHITE>;
> +		led-sources = <1>;
> +		led-max-microamp = <1500000>;
> +		flash-max-microamp = <1000000>;
> +		flash-max-timeout-us = <1280000>;
> +	};
> +};

[Severity: Medium]
Is led-max-microamp = <1500000> valid for a single flash channel here?

In Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml,
led-max-microamp (continuous torch current) is limited to 500000 uA for a
single channel or 1000000 uA for dual ganged channels, matching
TORCH_CURRENT_MAX_UA (500000) in drivers/leds/flash/leds-qcom-flash.c:

      led-max-microamp:
        anyOf:
          - minimum: 5000
            maximum: 500000
            multipleOf: 5000
          - minimum: 10000
            maximum: 1000000
            multipleOf: 10000

Setting led-max-microamp to 1500000 uA with led-sources = <1> fails
dtbs_check validation and sets the continuous torch current higher than both
the 500 mA hardware limit and the 1000000 uA flash-max-microamp strobe
current.

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