tdo_panel: bcm27xx-gpu: Add dsi panel driver;#7318
tdo_panel: bcm27xx-gpu: Add dsi panel driver;#7318flash-fea wants to merge 3 commits intoraspberrypi:rpi-6.18.yfrom
Conversation
flash-fea
commented
Apr 16, 2026
7a2e2f8 to
840f6f9
Compare
|
whether the addition was successful? |
|
The partitioning of the code looks good, and checkpatch is happy. This isn't my field of expertise so there is limited input I can give regarding the driver itself - perhaps @6by9 has some comments - but there are a few things I have spotted:
|
|
Sorry I had all my review comments pending on the 6.12 version. Now posted. Largely the same as pelwell has commented with regard pr_err debugging and authorship, but also a couple of other points. |
|
yes,some informations need to change,about two dev_err() only use in debug to find by text easyier,but forget to recover. |
| @@ -0,0 +1,69 @@ | |||
| /* | |||
| * Device Tree overlay for Waveshare DSI Touchscreens | |||
| compatible = "tdo,4.0-dsi-tl040wvs17"; | ||
| status = "okay"; | ||
| reg = <0>; | ||
| reset-gpios = <&gpio 47 1>; // Dummy GPIO , Unused or change |
There was a problem hiding this comment.
delete all? reset-gpios maybe use in future,i see these were added also in other dts.
There was a problem hiding this comment.
You can add it at a later stage via an override if it is needed.
As it stands the driver will claim a totally unrelated GPIO for no particular reason. GPIO 47 isn't necessarily unused or even present on all devices, so could actually cause issues if left in.
There was a problem hiding this comment.
some my device's driver need it, as how i write to overlay in use
There was a problem hiding this comment.
For those devices that do require it you can add an additional fragment to configure it which can be enabled by the relevant override.
A similar case is https://github.com/raspberrypi/linux/blob/rpi-6.12.y/arch/arm/boot/dts/overlays/edt-ft5406.dtsi#L11-L16 where we want to add the touchscreen-inverted-x; property on demand, triggered by the invx override
With the driver calling devm_gpiod_get_optional, the driver doesn't need it. It's permitted to call any of the gpiod_ functions with the returned (NULL) handle.
There was a problem hiding this comment.
like this?
reset_pin = <&panel>,"reset-gpios:47";
and modify driver for devm_gpiod_get_optional.
There was a problem hiding this comment.
Close:
reset_pin = <&panel>,"reset-gpios:0=",<&gpio>,
<&panel>,"reset-gpios:4=47",
<&panel>,"reset-gpios:8=1";
will create the reset-gpios property if you use the reset_pin parameter, but 6by9 was suggesting that you would build that into a product declaration:
new_panel_needing_reset = <&panel>, "compatible=tdo,new-panel-needing-reset",
<&panel>,"reset-gpios:0=",<&gpio>,
<&panel>,"reset-gpios:4=47",
<&panel>,"reset-gpios:8=1";
But it might be cleaner to have a dormant fragment that gets enabled as needed.
There was a problem hiding this comment.
The driver is already using
ctx->reset = devm_gpiod_get_optional(&dsi->dev, "reset", GPIOD_OUT_LOW);
That means that ctx->reset will not return an error if reset-gpios is missing.
Add an additional fragment along the lines of:
fragment@10 {
target = <&panel>;
reset_gpio_frag: __dormant__ {
reset-gpios = <&gpio 47 1>;
};
};
__overrides__ {
reset_pin = <0>, "+10",
<&reset_gpio_frag>, "reset-gpios:4";
};
You can then have dtoverlay=vc4-kms-dsi-tdo-panel,reset_pin=47 to enable the reset gpio and set it to 47 (or whatever other GPIO number you wish to use).
Just looking through the various Pi variants, gpio 47 is used for:
- Pi1A/B: SD card detect
- Pi1A+, B+, 2B, 3B, Zero, ZeroW: Activity LED
- Pi 3B+/A+, CM3, CM4, 02W, CM0 : I2C to SMPS
- CM1: EMMC_ENABLE_N
This is why I say it can't be claimed generically by the overlay.
(Pi5 appears to use it for 2712_WAKE)
| struct drm_display_mode *mode; | ||
|
|
||
| mode = drm_mode_duplicate(connector->dev, ctx->desc->mode); | ||
| dev_info(&ctx->dsi->dev, "%ux,%ux,%ux\n", ctx->desc->mode->hdisplay, |
There was a problem hiding this comment.
No user needs to see this - dev_dbg or delete.
| struct tdo_panel *ctx; | ||
| int ret; | ||
|
|
||
| dev_info(&dsi->dev, "dsi panel: %s\n", |
There was a problem hiding this comment.
It is preferred for successful probing to generate minimal output. Perhaps defer this line to after the number of lanes is available and combine the two?
| struct tdo_panel *ctx = mipi_dsi_get_drvdata(dsi); | ||
|
|
||
| if (ctx->reset) { | ||
| dev_info(&dsi->dev, "shutdown\n"); |
There was a problem hiding this comment.
You really don't need this.
There was a problem hiding this comment.
struct tdo_panel *ctx = mipi_dsi_get_drvdata(dsi);
gpiod_set_value_cansleep(ctx->reset, 0);
like this,is it ok?
There was a problem hiding this comment.
I was referring to the dev_info()
panel-tdo-dsi-v1.c as module to driver tdo panel. Signed-off-by: kun_liu <kun_liu@shtdo.com>
__overrides__ parameter can be add in config.txt for customers. README describe the dts config info. Signed-off-by: kun_liu <kun_liu@shtdo.com>
Share panel drivers to bcmxxx_defconfig as module to use in all. Signed-off-by: kun_liu <kun_liu@shtdo.com>