linux-kernel - Re: [PATCH v4 2/5] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Feb 2022 22:03:03 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Rob Clark <robdclark@...il.com>,
        Sean Paul <seanpaul@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>, quic_kalyant@...cinc.com,
        quic_abhinavk@...cinc.com, quic_khsieh@...cinc.com,
        quic_mkrishn@...cinc.com, quic_vproddut@...cinc.com
Subject: Re: [PATCH v4 2/5] arm64: dts: qcom: sc7280: Add support for eDP
 panel on CRD

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@...cinc.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