tdo_panel: bcm27xx-gpu: Add dsi panel driver;#7318
tdo_panel: bcm27xx-gpu: Add dsi panel driver;#73186by9 merged 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)
There was a problem hiding this comment.
this reset pin is not decided yet,can you give me a suggest? now dts add this->
+fragment@4 {
-
target = <&panel>; -
reset_gpio_frag: __dormant__ { -
reset-gpios = <&gpio 47 1>; -
}; - };
dsi0 = <&dsi_frag>, "target:0=",<&dsi0>,
<&i2c_frag>, "target:0=",<&i2c_csi_dsi0>;
- reset_pin = <0>, "+10",
-
<&reset_gpio_frag>, "reset-gpios:4";
There was a problem hiding this comment.
ctx->reset = devm_gpiod_get_optional(&dsi->dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(ctx->reset))
dev_warn(&dsi->dev, "Couldn't get the reset GPIO\n");
| 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()
|
my company need only one driver to use all our mipi panel,it seems to add some after this request finish. |
| rotation Display rotation {0,90,180,270} | ||
| dsi0 Use DSI0 and i2c_csi_dsi0 (rather than | ||
| the default DSI1 and i2c_csi_dsi). | ||
| reset_pin GPIO pin for RESET (default 25) |
There was a problem hiding this comment.
This is not true - there is no default value, because you can't enable the reset pin without assigning a value.
| rotation = <&panel>, "rotation:0"; | ||
| dsi0 = <&dsi_frag>, "target:0=",<&dsi0>, | ||
| <&i2c_frag>, "target:0=",<&i2c_csi_dsi0>; | ||
| reset_pin = <0>, "+10", |
There was a problem hiding this comment.
The number after the "+" is the number of the fragment to enable, which in this case should be 4.
You haven't actually answered my questions as to whether you are actually representing TDO as the panel manufacturer, and whether you are creating a product to use this with a Raspberry Pi. You may prefer having a single driver for all displays, but the mainline Linux folks may disagree and wish the panels to be clustered into similar families or by controller IC. |
|
Yes, I work at TDO. The company is currently expanding into the Raspberry Pi market, and in fact, some customers are already using it in a variety of scenarios. Merging into the branch will facilitate future work. |
7aa28c5 to
2677911
Compare
|
I've split the long README line |
|
Was the push successful? |
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>
|
I've had a look at the details of the products (or at least the two I can find), and at the driver, and I can't see anything Raspberry Pi-specific in them. As such, there's no incentive for us to host the driver - it would mean taking on the support burden of forward-porting to each new kernel version. |
|
There's been a discussion internally regarding the maintenance burden of additional drivers such as this, and how we handle this in future. Overlays are always specific to our kernel tree, so those aren't an issue. Those patches all get squashed together, and device tree is considered an ABI that can't be changed once published. Likewise defconfigs get rebuilt with each kernel release. Accepting drivers to our downstream creates a support burden on us to as the in-kernel APIs do change from time to time. If drivers are upstream then the person making the changes to the in-kernel APIs is responsible for updating all drivers that use those APIs. However we appreciate that you and other companies can be in the position of wanting to launch products, whilst upstreaming drivers takes a fair amount of time due to the kernel development cycle. If you need assistance in upstreaming drivers, then there are a number of consulting companies that are able to offer services (eg Collabora, Pengutronix, and Linaro). We're not in a position to offer assistance. We appreciate that this may not have been the answer you wanted, but we need to look at the bigger picture. If upstreaming isn't feasible, then you may want to look at a DKMS (Dynamic Kernel Module Support) package for the driver so it will get automatically rebuilt on the user's system whenever the kernel gets updated via apt. |
6by9
left a comment
There was a problem hiding this comment.
Nothing glaringly obviously wrong.
The driver Kconfig does mention supporting backlight control via the backlight class, but no backlight is configured in the overlay. Do the displays you're intending to sell have backlight control?
I suspect there is a question regarding touch overlays as well. TL034WVS03 for one appears to come with the option of a FT6336U touch overlay (at least according to panelook). That can be added later if desired as it's only a dtoverlay change.