Hi,

On Tue, Dec 30, 2025 at 9:21 AM Robin Murphy <[email protected]> wrote:
>
> +&{/} {
> +       pwm_bl: backlight {

nit: up to you, but I happened to notice that other rk3399 boards just
use the label "backlight:" instead of "pwm_bl:"


> +               compatible = "pwm-backlight";
> +               pwms = <&pwm0 0 25000 0>;

40kHz is awfully fast for a PWM backlight. Are you sure you need it that fast?


> +               enable-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_HIGH>;

Not that I'm doing a thorough review here, but I happened to notice
that you're referring to a GPIO without adding a pinctrl entry. I
think the usual convention is to add one.


> +               brightness-levels = <0 255>;
> +               default-brightness-level = <200>;
> +               num-interpolated-steps = <255>;
> +       };
> +};
> +
> +&edp {
> +       force-hpd;
> +       status = "okay";
> +
> +       aux-bus {
> +               edp-panel {
> +                       compatible = "friendlyarm,hd702e";

As per my response in your driver patch, any chance this can just be
"edp-panel"?


> +                       backlight = <&pwm_bl>;
> +                       no-hpd;
> +                       power-supply = <&vcc12v0_sys>;

While not strictly required, it seems highly likely that you want
"hpd-absent-delay-ms". It's highly unlikely that you would have
"no-hpd" plus a "power-supply" but no hardcoded delay to wait here. I
haven't seen panels that turn on and respond instantly.


> +
> +                       port {
> +                               panel_in_edp: endpoint {
> +                                       remote-endpoint = <&edp_out_panel>;
> +                               };
> +                       };
> +               };
> +       };
> +};
> +
> +&edp_out {
> +       edp_out_panel: endpoint {
> +               remote-endpoint = <&panel_in_edp>;
> +       };
> +};
> +
> +&i2c4 {
> +       #address-cells = <1>;
> +       #size-cells = <0>;

The base dts already specifies #address-cells and #size-cells, right?


> +       touchscreen@5d {
> +               compatible = "goodix,gt9271";
> +               reg = <0x5d>;
> +               interrupt-parent = <&gpio1>;
> +               interrupts = <RK_PC4 IRQ_TYPE_EDGE_FALLING>;
> +               irq-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_HIGH>;
> +               reset-gpios = <&gpio1 RK_PB5 GPIO_ACTIVE_HIGH>;

There's no power supply here, so I'll assume it follows the power
supply of the panel? You probably want to be a "panel-follower" then,
right? AKA you'd want to add a "panel = " node here to refer to your
edp-panel.

Oh, except that the goodix driver you're using doesn't support
panel-follower. That's annoying. I guess you'd have to add support for
that (see history around "is_panel_follower" in "i2c-hid-core.c")?
Without it then I assume you'll just be lucky that things work? ...or
you'd need to mark the power supply as always-on?

I guess I can also note the same comment that there is no pinctrl for
your GPIOs here.

-Doug

Reply via email to