arm64: dts: qcom: sc8280xp: Add camera MCLK pinctrl
PATCH 1/2 arm64: dts: qcom: sc8280xp: Add camera MCLK pinctrl
Define pinctrl definitions to enable camera master clocks on sc8280xp.
--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +56 -0
@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>; + cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk1_default: cam-mclk1-default-state { + pins = "gpio120"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk2_default: cam-mclk2-default-state { + pins = "gpio16"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk3_default: cam-mclk3-default-state { + pins = "gpio17"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk4_default: cam-mclk4-default-state { + pins = "gpio6"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk5_default: cam-mclk5-default-state { + pins = "gpio7"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk6_default: cam-mclk6-default-state { + pins = "gpio33"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + + cam_mclk7_default: cam-mclk7-default-state { + pins = "gpio34"; + function = "cam_mclk"; + drive-strength = <6>; + bias-disable; + }; + cci0_default: cci0-default-state { cci0_i2c0_default: cci0-i2c0-default-pins { /* cci_i2c_sda0, cci_i2c_scl0 */
Signature · 1 line
2.54.0
PATCH 2/2 arm64: dts: qcom: sc8280xp-x13s: Use predefined MCLK pinctrl
Now that the predefined MCLK pinctrl configuration is available in the sc8280xp SoC DTSI, switch the ThinkPad X13s to use it. Drop the local, duplicate MCLK pin definition from the camera pinctrl node.
--- .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -8
@@ -674,8 +674,9 @@ camera@10 { reg = <0x10>; reset-gpios = <&tlmm 15 GPIO_ACTIVE_LOW>; + + pinctrl-0 = <&cam_rgb_default>, <&cam_mclk3_default>; pinctrl-names = "default"; - pinctrl-0 = <&cam_rgb_default>; leds = <&privacy_led>; led-names = "privacy";
@@ -1552,13 +1553,6 @@ cam_indicator_en: cam-indicator-en-state { }; cam_rgb_default: cam-rgb-default-state { - mclk-pins { - pins = "gpio17"; - function = "cam_mclk"; - drive-strength = <16>; - bias-disable; - }; - sc-rgb-xshut-n-pins { pins = "gpio15"; function = "gpio";
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Use predefined MCLK pinctrl
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Silent reduction of camera MCLK drive strength from 16 mA to 6 mA. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Silent reduction of camera MCLK drive strength from 16 mA to 6 mA. --
commit 68374459d8e6941a056f52b1b8fa71811fd7137a
Author: Pengyu Luo <[email protected]>
arm64: dts: qcom: sc8280xp-x13s: Use predefined MCLK pinctrl
This commit switches the ThinkPad X13s to use the predefined MCLK pinctrl configuration from the sc8280xp SoC DTSI. It drops the local MCLK pin definition from the camera pinctrl node.
Quoted 15 lines
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2
-1@@ -674,8 +674,9 @@ camera@10 { reg = <0x10>; reset-gpios = <&tlmm 15 GPIO_ACTIVE_LOW>; + + pinctrl-0 = <&cam_rgb_default>, <&cam_mclk3_default>; pinctrl-names = "default"; - pinctrl-0 = <&cam_rgb_default>; leds = <&privacy_led>; led-names = "privacy";
[ ... ]
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts cam_indicator_en: cam-indicator-en-state { · line 1561
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +0
-7@@ -1552,13 +1553,6 @@ cam_indicator_en: cam-indicator-en-state { }; cam_rgb_default: cam-rgb-default-state { - mclk-pins { - pins = "gpio17"; - function = "cam_mclk"; - drive-strength = <16>; - bias-disable; - }; -
[Severity: High]
Does this unintentionally reduce the camera MCLK drive strength?
The commit message notes this is dropping a duplicate MCLK pin definition, but the generic cam_mclk3_default definition in sc8280xp.dtsi now used by the camera node defaults to a lower drive strength of 6 mA:
arch/arm64/boot/dts/qcom/sc8280xp.dtsi:
cam_mclk3_default: cam-mclk3-default-state {
pins = "gpio17";
function = "cam_mclk";
drive-strength = <6>;
bias-disable;
};Reducing the drive capability from 16 mA to 6 mA for a high-frequency master clock signal routed over long traces might degrade signal integrity.
Could this lead to intermittent or complete camera initialization failures on this hardware?
Quoted 3 lines
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +0
-0sc-rgb-xshut-n-pins { pins = "gpio15"; function = "gpio";
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Use predefined MCLK pinctrl
In reply to Pengyu LuoQuoted 25 lines · Pengyu Luo
Now that the predefined MCLK pinctrl configuration is available in the sc8280xp SoC DTSI, switch the ThinkPad X13s to use it. Drop the local, duplicate MCLK pin definition from the camera pinctrl node.
Signed-off-by Pengyu Luo <[email protected]>--- .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2
-1@@ -674,8 +674,9 @@ camera@10 { reg = <0x10>; reset-gpios = <&tlmm 15 GPIO_ACTIVE_LOW>; + + pinctrl-0 = <&cam_rgb_default>, <&cam_mclk3_default>; pinctrl-names = "default"; - pinctrl-0 = <&cam_rgb_default>; leds = <&privacy_led>; led-names = "privacy";
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts cam_indicator_en: cam-indicator-en-state { · line 1558
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +0
-4@@ -1552,13 +1553,6 @@ cam_indicator_en: cam-indicator-en-state { }; cam_rgb_default: cam-rgb-default-state { - mclk-pins { - pins = "gpio17"; - function = "cam_mclk"; - drive-strength = <16>;
This changes the value of drive-strength (which is probably way too high, but it would be good to know what Lenovo set originally..)
Konrad
Quoted 13 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>; + cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;
Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
Konrad
Quoted 18 lines · Konrad Dybcio
Quoted 16 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>;
Quoted 9 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0+ cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
It is 6mA.
Let us get ctl_reg first on Windows
lkd> !dd f111000 L8
# f111000 00000284 00000002 000000e2 00000000 # f111010 00000001 00000801 00000000 00000000
ctl_reg => 0x284
in msm_gpio_dbg_show_one()
...
drive = (ctl_reg >> g->drv_bit) & 7; // (0x284 >> 6) & 7 == 2
...seq_printf(s, " %dmA", msm_regval_to_drive(drive)); // (drive + 1) * 2 == 6;
...
x13s should be the same as gaokun3 in this part.
--
Best wishes,
Pengyu
Konrad Dybcio
Reviewed-byQuoted 40 lines · Pengyu Luo
Quoted 18 lines · Konrad Dybcio
Quoted 16 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>;Quoted 9 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0+ cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
It is 6mA.
Let us get
ctl_regfirst on Windowslkd> !dd f111000 L8
# f111000 00000284 00000002 000000e2 00000000 # f111010 00000001 00000801 00000000 00000000
ctl_reg=>0x284
Quoted 8 lines
in
msm_gpio_dbg_show_one()
...
drive = (ctl_reg>>g->drv_bit) & 7; // (0x284>> 6) & 7 == 2
...seq_printf(s, " %dmA", msm_regval_to_drive(drive)); // (drive + 1) * 2 == 6;
...x13s should be the same as gaokun3 in this part.
I confirmed as much and I'm willing to believe this is a default for all 8280 devices
for the second patch, please mention in the commit message that the value will now match windows and please add a fixes tag
Konrad
Quoted 50 lines · Konrad Dybcio
Quoted 40 lines · Pengyu Luo
Quoted 18 lines · Konrad Dybcio
Quoted 16 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>;Quoted 9 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0+ cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
It is 6mA.
Let us get
ctl_regfirst on Windowslkd> !dd f111000 L8
# f111000 00000284 00000002 000000e2 00000000 # f111010 00000001 00000801 00000000 00000000
ctl_reg=>0x284Quoted 8 lines
in
msm_gpio_dbg_show_one()
...
drive = (ctl_reg>>g->drv_bit) & 7; // (0x284>> 6) & 7 == 2
...seq_printf(s, " %dmA", msm_regval_to_drive(drive)); // (drive + 1) * 2 == 6;
...x13s should be the same as gaokun3 in this part.
Quoted 8 lines
I confirmed as much and I'm willing to believe this is a default for all 8280 devices
Reviewed-by Konrad Dybcio <[email protected]>for the second patch, please mention in the commit message that the value will now match windows and please add a fixes tag
I believe the second change cannot be tagged as Fixes in sense that it strictly depends on a not going to be backported non-fix commit, and thus backporting of just 2/2 change as is will break the matter. Reordering of the commits placing the fix commit as the first one should be fine though.
Signature · 2 lines
Best wishes, Vladimir
Quoted 56 lines · Vladimir Zapolskiy
Quoted 55 lines · Konrad Dybcio
Quoted 40 lines · Pengyu Luo
Quoted 18 lines · Konrad Dybcio
Quoted 16 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi| 56 ++++++++++++++++++++++++++1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>;Quoted 9 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0+ cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
It is 6mA.
Let us get
ctl_regfirst on Windowslkd> !dd f111000 L8
# f111000 00000284 00000002 000000e2 00000000 # f111010 00000001 00000801 00000000 00000000
ctl_reg=>0x284Quoted 8 lines
in
msm_gpio_dbg_show_one()
...
drive = (ctl_reg>>g->drv_bit) & 7; // (0x284>> 6) & 7 == 2
...seq_printf(s, " %dmA", msm_regval_to_drive(drive)); // (drive + 1) * 2 == 6;
...x13s should be the same as gaokun3 in this part.
I confirmed as much and I'm willing to believe this is a default for all 8280 devices
Reviewed-by Konrad Dybcio <[email protected]>
Quoted 8 lines
Quoted 3 lines
for the second patch, please mention in the commit message that the value will now match windows and please add a fixes tag
I believe the second change cannot be tagged as Fixes in sense that it strictly depends on a not going to be backported non-fix commit, and thus backporting of just 2/2 change as is will break the matter. Reordering of the commits placing the fix commit as the first one should be fine though.
The Fixes tag makes the patch eligible for backporting through AUTOSEL but is itself not the same as "please backport"
Konrad
Quoted 61 lines · Konrad Dybcio
Quoted 60 lines · Vladimir Zapolskiy
Quoted 58 lines · Konrad Dybcio
Quoted 40 lines · Pengyu Luo
Quoted 18 lines · Konrad Dybcio
Quoted 16 lines · Pengyu Luo
Define pinctrl definitions to enable camera master clocks on sc8280xp.
Suggested-by Vladimir Zapolskiy <[email protected]>Signed-off-by Pengyu Luo <[email protected]>--- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0
-0@@ -5484,6 +5484,62 @@ tlmm: pinctrl@f100000 { gpio-ranges = <&tlmm 0 0 230>; wakeup-parent = <&pdc>;Quoted 9 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi tlmm: pinctrl@f100000 { · line 5490
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc8280xp.dtsi +4
-0+ cam_mclk0_default: cam-mclk0-default-state { + pins = "gpio119"; + function = "cam_mclk"; + drive-strength = <6>;Other platforms set this to 2 by default.
What's the value set on Windows when the camera is in use?
It is 6mA.
Let us get
ctl_regfirst on Windowslkd> !dd f111000 L8
# f111000 00000284 00000002 000000e2 00000000 # f111010 00000001 00000801 00000000 00000000
ctl_reg=>0x284Quoted 8 lines
in
msm_gpio_dbg_show_one()
...
drive = (ctl_reg>>g->drv_bit) & 7; // (0x284>> 6) & 7 == 2
...seq_printf(s, " %dmA", msm_regval_to_drive(drive)); // (drive + 1) * 2 == 6;
...x13s should be the same as gaokun3 in this part.
I confirmed as much and I'm willing to believe this is a default for all 8280 devices
Reviewed-by Konrad Dybcio <[email protected]>for the second patch, please mention in the commit message that the value will now match windows and please add a fixes tag
Quoted 8 lines
Quoted 4 lines
I believe the second change cannot be tagged as Fixes in sense that it strictly depends on a not going to be backported non-fix commit, and thus backporting of just 2/2 change as is will break the matter. Reordering of the commits placing the fix commit as the first one should be fine though.
The Fixes tag makes the patch eligible for backporting through AUTOSEL but is itself not the same as "please backport"
That's correct, and due Documentation/process/stable-kernel-rules.rst it would make sense to add Cc: <[email protected]> to the next version of the change to help stable tree maintainers, since it is known in advance that the unmodified and Fixes tagged 2/2 change shall not be considered as a candidate change to the stable tree. Or is it excessive?
IMHO here it might be better to properly arrange the changes and backport the fix.
Signature · 2 lines
Best wishes, Vladimir