On Thu 17 Feb 17:03 PST 2022, Doug Anderson wrote:
Hi,
On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
backlight_3v3_regulator: backlight-3v3-regulator {compatible = "regulator-fixed";regulator-name = "backlight_3v3_regulator";regulator-min-microvolt = <3300000>;regulator-max-microvolt = <3300000>;gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;enable-active-high;pinctrl-names = "default";pinctrl-0 = <&edp_bl_power>;};So I'm pretty sure that this is wrong and what you had on a previous patch was more correct. Specifically the PMIC's GPIO 7 truly _is_ an enable pin for the backlight. In the schematics I see it's named as "PMIC_EDP_BL_EN" and is essentially the same net as "EDP_BL_EN". This is distinct from the backlight _regulator_ that is named VREG_EDP_BP. I believe the VREG_EDP_BP is essentially sourced directly from PPVAR_SYS. That's how it works on herobrine and I believe that CRD is the same. You currently don't model ppvar_sys, but it's basically just a variable-voltage rail that could be provided somewhat directly from the battery or could be provided from Type C components. I believe that the panel backlight is designed to handle this fairly wide voltage range and it's done this way to get the best efficiency.
So personally I'd prefer if you do something like herobrine and model PPVAR_SYS. Then the backlight can use ppvar_sys as its regulator and you can go back to providing this as an "enable" pin for the backlight.
I know, technically it doesn't _really_ matter, but it's nice to model it more correctly.
While I've not seen your schematics, the proposal does look similar to what I have on sc8180x, where there's a power rail, the BL_EN and a pwm signal.
If that's the case I think representing BL_EN using the enable-gpios property directly in the pwm-backlight node seems more appropriate (with power-supply being the actual thing that powers the backlight).
If however gpio 7 is wired to something like the enable-pin on an actual LDO the proposal here seems reasonable, but it seems unlikely that the output of that would be named "backlight_3v3_regulator"?
Regards, Bjorn