| Message ID | 20220520124834.1.I7c51c6255bb53086a82c5b3f4fafffcc5ccbc4ae@changeid (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | arm64: dts: qcom: sc7280: Enable keyboard backlight for villager | expand |
Hi, On Fri, May 20, 2022 at 12:48 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > Villager has a backlit keyboard, enable support for the backlight. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts | 8 ++++++++ > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > index d3d6ffad4eff..b6a6a1454883 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > @@ -58,6 +58,10 @@ &ap_sar_sensor1 { > status = "okay"; > }; > > +&keyboard_backlight { > + status = "okay"; > +}; Instead of doing this, can you just get rid of the status = "disabled" in herobrine.dtsi? I don't think there's any benefit to having two levels of "disabled" in the herobrine device tree.
On Fri, May 20, 2022 at 12:57:50PM -0700, Doug Anderson wrote: > Hi, > > On Fri, May 20, 2022 at 12:48 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Villager has a backlit keyboard, enable support for the backlight. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts | 8 ++++++++ > > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +- > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > index d3d6ffad4eff..b6a6a1454883 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > @@ -58,6 +58,10 @@ &ap_sar_sensor1 { > > status = "okay"; > > }; > > > > +&keyboard_backlight { > > + status = "okay"; > > +}; > > Instead of doing this, can you just get rid of the status = "disabled" > in herobrine.dtsi? I don't think there's any benefit to having two > levels of "disabled" in the herobrine device tree. Sure. I guess the 'disabled' status was put as a micro-optimization to avoid probing the 'pwm-leds' driver on boards that don't have any such LEDs. In practical terms it shouldn't really make a difference in terms of memory or CPU.
Hi, On Fri, May 20, 2022 at 1:29 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Fri, May 20, 2022 at 12:57:50PM -0700, Doug Anderson wrote: > > Hi, > > > > On Fri, May 20, 2022 at 12:48 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > Villager has a backlit keyboard, enable support for the backlight. > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts | 8 ++++++++ > > > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +- > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > > index d3d6ffad4eff..b6a6a1454883 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts > > > @@ -58,6 +58,10 @@ &ap_sar_sensor1 { > > > status = "okay"; > > > }; > > > > > > +&keyboard_backlight { > > > + status = "okay"; > > > +}; > > > > Instead of doing this, can you just get rid of the status = "disabled" > > in herobrine.dtsi? I don't think there's any benefit to having two > > levels of "disabled" in the herobrine device tree. > > Sure. > > I guess the 'disabled' status was put as a micro-optimization to avoid > probing the 'pwm-leds' driver on boards that don't have any such LEDs. In > practical terms it shouldn't really make a difference in terms of memory > or CPU. Just to be clear, I was suggesting leaving the "disabled" at the pwm-leds level but removing the one at the "keyboard_backlight" level. I don't expect us to have any pwm_leds on herobrine devices that aren't for the keyboard backlight. If/when we do we can always add the two-levels of disabled back in... -Doug
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts index d3d6ffad4eff..b6a6a1454883 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts @@ -58,6 +58,10 @@ &ap_sar_sensor1 { status = "okay"; }; +&keyboard_backlight { + status = "okay"; +}; + &mdss_edp { status = "okay"; }; @@ -76,6 +80,10 @@ &pcie1_phy { status = "okay"; }; +&pwmleds { + status = "okay"; +}; + /* For eMMC */ &sdhc_1 { status = "okay"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi index 9cb1bc8ed6b5..4b8bdcd6d9f4 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi @@ -295,7 +295,7 @@ pp1200_wf_cam: pp1200-wf-cam-regulator { /* BOARD-SPECIFIC TOP LEVEL NODES */ - pwmleds { + pwmleds: pwmleds { compatible = "pwm-leds"; status = "disabled"; keyboard_backlight: keyboard-backlight {
Villager has a backlit keyboard, enable support for the backlight. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts | 8 ++++++++ arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-)