Conversation
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
These comments are based on a fairly brief read through.
| * Init sequence (page select via 0xDE). Page0 0xCC = 0x31 (2-lane). | ||
| */ | ||
| static const struct jd9365tx_cmd jd9365tx_7kf82_init[] = { | ||
| #include "panel-viewe-jd9365tx-init.h" |
There was a problem hiding this comment.
Including a .h file that isn't a header isn't acceptable.
| { | ||
| struct jd9365tx_panel *ctx = to_jd9365tx(panel); | ||
| const struct drm_display_mode *src = | ||
| use_60hz ? &jd9365tx_7kf82_mode_60 : &jd9365tx_7kf82_mode_50; |
There was a problem hiding this comment.
Should be switched by the compatible string, not a module parameter.
| "Delay between DISPOFF/SLPIN retries"); | ||
|
|
||
| static struct mipi_dsi_device *g_reboot_dsi; | ||
| static struct notifier_block jd9365tx_reboot_nb; |
There was a problem hiding this comment.
Globals don't work if you have two of the devices connected to the system.
| static uint sleep_cmd_retry_delay_ms = 20; | ||
| module_param(sleep_cmd_retry_delay_ms, uint, 0644); | ||
| MODULE_PARM_DESC(sleep_cmd_retry_delay_ms, | ||
| "Delay between DISPOFF/SLPIN retries"); |
There was a problem hiding this comment.
None of these things should be module parameters. You're writing the driver for this particular panel, so these values should be known.
| return 0; | ||
| } | ||
|
|
||
| static void jd9365tx_run_dsi_diag(struct mipi_dsi_device *dsi, const char *tag) |
There was a problem hiding this comment.
DSI reads are generally a little dubious. This adds nothing.
|
|
||
| /* | ||
| * Parse ESRAM section table for coordinate_report address / length. | ||
| * Layout matches official jd9365tx_ChipInfoInit / ReadSectionInfo. |
There was a problem hiding this comment.
If you're referencing documentation, then it needs to be a complete reference to what the document is.
| if (ret == 1) | ||
| return 0; | ||
| msleep(20); | ||
| } |
There was a problem hiding this comment.
Why is I2C communication with this device so unreliable that it requires retries? That implies some issue in the design somewhere.
| if (down) { | ||
| touchscreen_report_pos(ts->input, &ts->prop, x, y, true); | ||
| input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, w); | ||
| ts->prev_x[i] = x; |
There was a problem hiding this comment.
Why store the points? They are never read back.
| if (ts->suspended) | ||
| return; | ||
|
|
||
| mutex_lock(&ts->lock); |
There was a problem hiding this comment.
What do you think this mutex is protecting against? The poll worker will only ever be called from one workqueue context, and this is the only thing that locks and unlocks the mutex.
| }; | ||
|
|
||
| fragment@2 { | ||
| target = <&i2c_csi_dsi1>; |
There was a problem hiding this comment.
On pi5, i2c_csi_dsi is an alias of i2c_csi_dsi1, so no need for a separate overlay at all as that is the only diff.
|
This needs to be broken into at least the following patches:
The kernel requires all patches to have the Signed-off-by: to be an identifiable individual. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin The weird bus driver setup for touchscreen and backlight needs some explanation. They're on separate I2C addresses, and all i2c_transfer calls are atomic, so why the need to try synchronising them? And all the drivers and binding should be submitted upstream. Having drivers in this downstream kernel tree adds an undesirable maintenance burden. Upstream will give you a significantly more thorough review as well. |
Add the device tree binding for the VIEWE 7KF82 720x1280 Jadard JD9365TX MIPI-DSI panel. Signed-off-by: Hefei <3066883572@qq.com>
Add the binding for the Jadard JD9365TX in-cell touchscreen used on the VIEWE 7KF82 module. Signed-off-by: Hefei <3066883572@qq.com>
Add the binding for the VIEWE panel backlight MCU on I2C. Signed-off-by: Hefei <3066883572@qq.com>
Add a DRM MIPI-DSI panel driver for the VIEWE 7KF82 module based on Jadard JD9365TX (2-lane video mode). Signed-off-by: Hefei <3066883572@qq.com>
Add a polling I2C touchscreen driver for Jadard JD9365TX in-cell panels that do not wire an interrupt line on the FPC. Signed-off-by: Hefei <3066883572@qq.com>
Add a simple I2C backlight class driver for the VIEWE panel MCU at address 0x45, register 0x86. Signed-off-by: Hefei <3066883572@qq.com>
Enable the VIEWE panel, touch and backlight drivers as modules in the Raspberry Pi bcm2711 and bcm2712 defconfigs. Signed-off-by: Hefei <3066883572@qq.com>
Add a dtoverlay for the VIEWE 7 inch JD9365TX panel on DSI1 using i2c_csi_dsi for touch and backlight. Signed-off-by: Hefei <3066883572@qq.com>
|
We greatly appreciate your meticulous guidance. The code will be revised strictly in compliance with your specified requirements. Our sincere gratitude goes to you for your support.
Original
From:6by9 ***@***.***>
Sent Time:Sep 22, 2026 23:16
To:raspberrypi/linux ***@***.***>
Cc:HF-2001 ***@***.***>, Author ***@***.***>
Subject:Re: [raspberrypi/linux] drm/panel: add VIEWE 7 inch JD9365TX in-cell DSI panel (PR #7633)
6by9 left a comment (raspberrypi/linux#7633)
This needs to be broken into at least the following patches:
DT bindings for panel driver
DT bindings for touch driver
DT bindings for backlight MCU
DRM panel driver
touch driver
backlight MCU driver
defconfig changes (ideally splitting arm from arm64)
overlay
The kernel requires all patches to have the Signed-off-by: to be an identifiable individual. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
We don't care as much for dtoverlays, but drivers need it.
The weird bus driver setup for touchscreen and backlight needs some explanation. They're on separate I2C addresses, and all i2c_transfer calls are atomic, so why the need to try synchronising them?
And all the drivers and binding should be submitted upstream. Having drivers in this downstream kernel tree adds an undesirable maintenance burden. Upstream will give you a significantly more thorough review as well.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.
|
48f2e04 to
d9149a0
Compare
|
Thanks for the review. Pushed an updated series:
The queue was left over from early bring-up. Touch and backlight are on different addresses, so normal independent I2C transfers are enough. |
2 participants
Add support for the VIEWE 7" 720x1280 MIPI DSI panel (Jadard JD9365TX, module 7KF82). Includes the DRM panel driver, in-cell touch (I2C poll), I2C backlight, DT bindings, and overlays for Pi 4 and Pi 5.
Usage:
dtoverlay=vc4-kms-dsi-viewe-7inch # Pi 4
dtoverlay=vc4-kms-dsi-viewe-7inch-pi5 # Pi 5
Panel is 2-lane only. Touch is at 0x68, backlight MCU at 0x45.
Tested on Raspberry Pi 4 and Raspberry Pi 5.