Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Doug Anderson]

Hi,

On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > +               compatible = "regulator-fixed";
> > +               status = "okay";
> > +               regulator-name = "pp3300_brij_ps8640";
> > +
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +
> > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
>
> Doesn't this need
>
>                 enable-active-high;

Looks like it. Without that it looks like it assumes active low.


> > +
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > +
> > +               vin-supply = <&pp3300_a>;
> > +       };
> > +};
> > +
> > +&dsi0_out {
> > +       remote-endpoint = <&ps8640_in>;
>
> Should this also have data-lanes to be "complete"?

That's still back in the main trogdor.dtsi, isn't it?


> > +edp_brij_i2c: &i2c2 {
> > +       status = "okay";
> > +       clock-frequency = <400000>;
> > +
> > +       ps8640_bridge: edp-bridge@8 {
>
> Just bridge@8 to match ti bridge? Also, is the label used? If not
> please drop it.

I agree with Stephen about it being "bridge@8". Personally I don't
mind labels like this even if they're not used since they don't hurt
and it creates less churn to add them now, but I won't fight hard to
keep them.


> > +               aux_bus: aux-bus {
>
> Is this label used either?

Yeah, I'd get rid of this one since there you didn't add it in the
sn65dsi86 dtsi file and it seems exceedingly unlikely we'd need it for
any reason.

-Doug