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

Hi

On Thu, Oct 7, 2021 at 11:15 AM Philip Chen [off-list ref] wrote:

Hi,

On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson [off-list ref] wrote:

quoted

Hi,

On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd [off-list ref] wrote:

quoted

quoted

+       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.

Thanks for catching this.
I'll fix it in v3.

quoted

quoted

quoted

+
+               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?

Yes, I think so.
Plus, ti-sn65 dts doesn't define data-lanes for input either.

Sorry, I was wrong.
ti-sn65 dts actually defines data-lanes for input.
However, since ps8640 driver doesn't parse input data-lanes for now,
it's not useful to add data-lanes here anyway.

quoted

quoted

quoted

+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.

I will make it "bridge@8" to match ti-sn65 dts.
Meanwhile, can we keep "ps8640_bridge" label to match ti-sn65 dts?

quoted

quoted

quoted

+               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.

Sure, I will remove "aux_bus" label in v3.

quoted

-Doug