Re: [PATCH 02/11] arm64: dts: qcom: sc8280xp-arcata: Enable backlight

["Jérôme de Bretagne" <jerome.debretagne@gmail.com>]

On 5/20/26 18:57, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The eDP backlight regulator (`vreg_edp_bl`) is mapped to the wrong PMIC for its enable GPIO, directly contradicting the hardware findings documented in the commit message.
> --
> 
> commit 5bb21a2e354ddc1fbe2cb4a72c610f631fc3961f
> Author: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> 
> arm64: dts: qcom: sc8280xp-arcata: Enable backlight
> 
> This commit adds backlight nodes and enables the backlight so that it can be
> controlled with the buttons on Surface Pro Type Cover keyboards. The nodes
> are similar to the ThinkPad X13s but use a different enable GPIO for the
> vreg_edp_bl node.
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
>> index f2b4470d4407f..14287092fb07e 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
> [ ... ]
>> @@ -127,6 +137,22 @@ pmic_glink_con1_sbu: endpoint {
>>   		};
>>   	};
>>   
>> +	vreg_edp_bl: regulator-edp-bl {
>> +		compatible = "regulator-fixed";
>> +
>> +		regulator-name = "VREG_EDP_BL";
>> +		regulator-min-microvolt = <3600000>;
>> +		regulator-max-microvolt = <3600000>;
>> +
>> +		gpio = <&pmc8280c_gpios 6 GPIO_ACTIVE_HIGH>;
> 
> [Severity: High]
> Does this map to the correct PMIC for the enable GPIO?
> 
> The commit message explicitly states that the hardware match for the backlight
> enable GPIO was found at platform/c440000.spmi:pmic@3:gpio@8800 (which is
> gpio6).
> 
> According to sc8280xp-pmics.dtsi, pmic@3 corresponds to the pmc8280_2 PMIC and
> its pmc8280_2_gpios controller.
> 
> However, the patch configures the regulator and pinctrl to use pmc8280c_gpios,
> which is physically mapped to pmic@2.
> 
> Will this result in toggling the wrong physical pin on pmic@2 instead of
> enabling the eDP backlight?
> 
>> +		enable-active-high;
>> +
>> +		pinctrl-0 = <&edp_bl_reg_en>;
>> +		pinctrl-names = "default";
>> +
>> +		regulator-boot-on;
>> +	};
> 

I was skeptical at first but upon review I've tried the following patch:


diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts 
b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
index f938e68c69ea..82e0a01276a8 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
@@ -198,7 +198,7 @@ vreg_edp_bl: regulator-edp-bl {
                 regulator-min-microvolt = <3600000>;
                 regulator-max-microvolt = <3600000>;

-               gpio = <&pmc8280c_gpios 6 GPIO_ACTIVE_HIGH>;
+               gpio = <&pmc8280_2_gpios 6 GPIO_ACTIVE_HIGH>;
                 enable-active-high;

                 pinctrl-0 = <&edp_bl_reg_en>;
@@ -1050,6 +1050,11 @@ vol_up_n: vol-up-n-state {
  };

  &pmc8280_2_gpios {
+       edp_bl_reg_en: edp-bl-reg-en-state {
+               pins = "gpio6";
+               function = "normal";
+       };
+
         wwan_sw_en: wwan-sw-en-state {
                 pins = "gpio1";
                 function = "normal";
@@ -1061,11 +1066,6 @@ edp_bl_pwm: edp-bl-pwm-state {
                 pins = "gpio8";
                 function = "func1";
         };
-
-       edp_bl_reg_en: edp-bl-reg-en-state {
-               pins = "gpio6";
-               function = "normal";
-       };
  };

  &pmr735a_gpios {



and ... backlight still works! It can also be enabled/disabled with:
    /sys/class/backlight/backlight/bl_power
as expected.

I have no clue why both gpio6 on pmc8280_2 and gpio6 on pmc8280c can 
enable and disable the backlight. Is it a common scenario, Johan?

Anyway, if I follow my own investigation fully based on the X13s 
reference and the properties of its backlight enable GPIO, I should 
switch to pmc8280_2 to use the better match.

I will update this patch in v2.

Jérôme