Add Waveshare 6.25-DSI-TOUCH-A display support#7478
Conversation
Add initialization sequences and display modes for the Waveshare 6.25-DSI-TOUCH-A display in both 2-lane and 4-lane MIPI DSI configurations. The panel uses a 720x1560 resolution at 93MHz pixel clock with MIPI DSI RGB888 format. Signed-off-by: Waveshare_Team <support@waveshare.com>
Add a new I2C touchscreen controller driver for the Sitronix ST7123 chip, organized as a multi-file module under drivers/input/touchscreen/sitronix_st7123/. The driver consists of: - sitronix_ts.c: core touchscreen handling and initialization - sitronix_ts_i2c.c: I2C bus interface layer - sitronix_ts_mt.c: multi-touch reporting - sitronix_ts_upgrade.c: firmware upgrade support - sitronix_ts_utility.c: configuration and mode control Supported features include multi-touch reporting, firmware upgrade, and various operating modes (glove, charge suppression, palm rejection, grip suppression). The driver is built as a module (sitronix-st7123) and depends on I2C and the input subsystem. Signed-off-by: Waveshare_Team <support@waveshare.com>
Add device tree overlay entries for the Waveshare 6.25-DSI-TOUCH-A display in both 2-lane and 4-lane configurations. The 6.25-DSI-TOUCH-A uses the Sitronix ST7123 touchscreen controller at I2C address 0x55, replacing the default Goodix touch configuration. Rename the touch node from "goodix@5d" to "touch@5d" to reflect that different panels may use different touch controllers. Enable CONFIG_TOUCHSCREEN_ST7123=m in bcm2712_defconfig. Signed-off-by: Waveshare_Team <support@waveshare.com>
| CONFIG_TOUCHSCREEN_USB_COMPOSITE=m | ||
| CONFIG_TOUCHSCREEN_TSC2007=m | ||
| CONFIG_TOUCHSCREEN_TSC2007_IIO=y | ||
| CONFIG_TOUCHSCREEN_ST7123=m |
There was a problem hiding this comment.
Please move the config changes into a separate commit once you've responded to other feedback (i.e. you don't need to do it yet).
There was a problem hiding this comment.
And include the 32-bit defconfigs, unless there is a reason they won't be compatible.
There was a problem hiding this comment.
Thanks, I’ll split the defconfig updates into a separate commit and add CONFIG_TOUCHSCREEN_ST7123=m to the 32-bit Raspberry Pi defconfigs as well.
Our hardware testing so far has been on the 64-bit kernel. The driver only depends on I2C and is built as a module,so I don’t see an obvious 32-bit-specific compatibility issue.
6by9
left a comment
There was a problem hiding this comment.
Review of the touch driver alone.
Does this touch controller require the firmware to be uploaded every boot? If not, then updating the firmware would be far better implemented as a userspace application rather than a huge block of code that has to be loaded as part of the module on every system.
If not required, and we remove the I2C transfer abstraction, the whole thing likely makes sense as 1 source file rather than multiple.
This looks very much like the type of driver produced by the chip vendor to cover a bundle of kernel trees (eg vendor specific Android) with various modifications. None of those vendor specific bits are relevant to what should be a mainline touch controller driver, and therefore they should all be removed.
I've commented in a couple of places, global variables are bad news. Please amend to use the device state structure.
I'm amazed that the checkpatch CI flow hasn't thrown any errors. Admittedly I normally run with --strict, but things like the missing dt-bindings should throw up an undocumented compatible, and camel case is almost universally frowned upon within the kernel.
With --strict I get 3 warnings (strscpy instead of strcpy, compatible string docs, and MAINTAINERS), and 217 checks which are largely for u8 vs uint8_t, camel case, if (foo == NULL) instead of if (!foo), externs in header files, and whitespace.
| #include "sitronix_st7123.h" | ||
|
|
||
| struct sitronix_ts_data *gts; | ||
| //unsigned char wbuf[SITRONIX_RW_BUF_LEN+8] = {0}; |
There was a problem hiding this comment.
Commented out code. There are commented out blocks throughout this file, all of which should be cleaned up.
There was a problem hiding this comment.
Thanks for the detailed review.
I’ll go through the comments as a set and update the series accordingly. In particular, I’ll clean up the touchscreen driver style issues, remove commented-out code.
I’ll also confirm the driver-related details with the touchscreen IC vendor before sending the next revision.
| //unsigned char wbuf[SITRONIX_RW_BUF_LEN+8] = {0}; | ||
| //unsigned char rbuf[SITRONIX_RW_BUF_LEN+8] = {0}; | ||
| unsigned char *wbuf; | ||
| unsigned char *rbuf; |
There was a problem hiding this comment.
Global variables fail if you try connecting 2 (or more) touch screens to the same device.
| uint8_t read_len = (ts_data->ts_dev_info.max_touches * 7) + 5; | ||
| uint8_t touch_count = 0; | ||
| uint8_t *p; | ||
| uint16_t coordCkhsum = 0x5A; //since v01.12.01 |
There was a problem hiding this comment.
So what happens on versions with versions before v1.12.01? If this is a restriction, then check the version in the probe and enforce it to be greater.
| static void sitronix_ts_resume_work(struct work_struct *work); | ||
| static int sitronix_ts_suspend(struct device *dev); | ||
| static int sitronix_ts_resume(struct device *dev); | ||
| #ifdef _MSM_DRM_NOTIFY_H_ |
There was a problem hiding this comment.
This is a Qualcomm kernel tree specific define which is not applicable to a mainline kernel.
| } | ||
|
|
||
| /* Timer poll function */ | ||
| static void sitronix_ts_irq_poll_timer(struct timer_list *t) |
There was a problem hiding this comment.
You can set up polling by the input subsystem just by calling input_set_poll_interval(tsdata->input, POLL_INTERVAL);, avoiding having to create your own work thread and timer.
See 409fe0c for an example with the goodix controller.
| buf[0] = (addr >> 8) & 0xFF; | ||
| buf[1] = (addr) & 0xFF; | ||
|
|
||
| for (retry = 1; retry <= I2C_RETRY_COUNT; retry++) { |
There was a problem hiding this comment.
How unreliable is the I2C comms on this touch screen that it warrants having a retry mechanism?
|
|
||
| static struct sitronix_ts_i2c_data g_i2c_data; | ||
|
|
||
| static struct sitronix_ts_host_interface i2c_host_if = { |
There was a problem hiding this comment.
You only have I2C support, so no need for the abstraction of these functions.
| return -1; | ||
| } | ||
|
|
||
| stmsg("Display status = 0x%X\n", rbuf[off]); |
There was a problem hiding this comment.
If the read was done at line 52, why wait until here to use the result?
| int ret = 0; | ||
| int off = 0; | ||
|
|
||
| cmd[1] = 0x53; |
There was a problem hiding this comment.
Condense all these structs into const uint8_t arrays as that is more generally efficient than having code to update an array on the stack each time,
| #include "sitronix_st7123.h" | ||
| #include "sitronix_ts_upgrade.h" | ||
|
|
||
| #define st_u8 u8 |
There was a problem hiding this comment.
Remove all these and use the standard names.
6by9
left a comment
There was a problem hiding this comment.
Panel driver and overlay review
| @@ -2444,11 +2480,12 @@ static const struct of_device_id ws_panel_of_match[] = { | |||
| { .compatible = "waveshare,7.0-dsi-touch-a", &ws_panel_7_inch_a_desc }, | |||
| { .compatible = "waveshare,7.0-dsi-touch-b", &ws_panel_7_inch_a_desc }, | |||
| { .compatible = "waveshare,7.0-dsi-touch-c", &ws_panel_7_inch_c_desc }, | |||
| { .compatible = "waveshare,6.25-dsi-touch-a,4lane", | |||
There was a problem hiding this comment.
AIUI Additional commas are not allowed in compatible strings.
| @@ -2444,11 +2480,12 @@ static const struct of_device_id ws_panel_of_match[] = { | |||
| { .compatible = "waveshare,7.0-dsi-touch-a", &ws_panel_7_inch_a_desc }, | |||
| { .compatible = "waveshare,7.0-dsi-touch-b", &ws_panel_7_inch_a_desc }, | |||
| { .compatible = "waveshare,7.0-dsi-touch-c", &ws_panel_7_inch_c_desc }, | |||
| { .compatible = "waveshare,6.25-dsi-touch-a,4lane", | |||
| &ws_panel_6_25_inch_a_4lane_desc }, | |||
| { .compatible = "waveshare,6.25-dsi-touch-a", &ws_panel_6_25_inch_a_desc }, | |||
There was a problem hiding this comment.
If the 4 lane version is waveshare,6.25-dsi-touch-a,4lane, why is this one not waveshare,6.25-dsi-touch-a,2lane? Some consistency would be nice.
| @@ -1393,6 +1393,139 @@ static const struct panel_init_cmd ws_panel_7_c_init[] = { | |||
| {}, | |||
| }; | |||
|
|
|||
| static const struct panel_init_cmd ws_panel_6_25_a_4lane_init[] = { | |||
There was a problem hiding this comment.
Commit text
The panel uses a 720x1560 resolution at 93MHz pixel clock with
MIPI DSI RGB888 format.
It's only 93MHz in 4 lane mode.
| .vtotal = 1560 + 200 + 8 + 10, | ||
| .width_mm = 62, | ||
| .height_mm = 110, | ||
| }; |
There was a problem hiding this comment.
As it works over 2 lanes at 60Hz, what is the gain in having the 4 lane mode?
Add Waveshare 6.25-DSI-TOUCH-A display support
Summary
This series adds support for the Waveshare 6.25-DSI-TOUCH-A display to the Raspberry Pi kernel tree.
The panel is a 720x1560 MIPI DSI display and is supported in both 2-lane and 4-lane configurations. The series also adds the Sitronix ST7123 I2C touchscreen driver used by this panel, wires the driver into the Raspberry Pi defconfigs, and exposes the panel through the existing
vc4-kms-dsi-waveshare-panel-v2overlay.Changes
Add Waveshare 6.25-DSI-TOUCH-A support to
panel-waveshare-dsi-v2.waveshare,6.25-dsi-touch-awaveshare,6.25-dsi-touch-a,4laneAdd the Sitronix ST7123 touchscreen driver.
CONFIG_TOUCHSCREEN_ST7123.sitronix-st7123module.Update the Waveshare v2 DSI overlay.
6_25_inch_aand6_25_inch_a_4laneoverlay parameters.0x55for this panel.goodix@5dtotouch@5d, since the overlay now supports panels using different touchscreen controllers.Enable the ST7123 touchscreen module in Raspberry Pi arm64 defconfigs.
bcm2711_defconfigbcm2711_rt_defconfigbcm2712_defconfigHardware
0x550x45Overlay Usage
2-lane configuration:
dtoverlay=vc4-kms-dsi-waveshare-panel-v2,6_25_inch_a4-lane configuration:
dtoverlay=vc4-kms-dsi-waveshare-panel-v2,6_25_inch_a_4lane