Waveshare 10.1inch DSI LCD (E) - #7662
Conversation
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
Required change:
Panel driver change and dtoverlay change need to be in separate commits. Ideally rebase the branch instead of adding a merge commit.
Other comments:
I'm not going to fuss as this driver is only used by Waveshare and won't be upstreamed by us, but the cleaner approach would have been to do git revert fd7924c3ab70e and git revert 5d31114d5994 to cleanly discard the old commits, and then add new commits with the desired solution. In the rebasing onto the next release, the reverted commits would be dropped.
As we don't need the history for this driver, the reality is that all the driver commits will get squashed into one anyway.
I obviously missed it last time, but your Signed-off-by: doesn't really fulfill the requirements of being a known identity - https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
Whilst I've made a comment on the implementation, again I'm not fussed as this only used by you.
| is_10_1_e_2lane = of_device_is_compatible(dev->of_node, | ||
| "waveshare,10.1inch-e-2lane-panel"); | ||
| if (is_10_1_e_4lane || is_10_1_e_2lane) | ||
| ws_panel_i2c_write(ts, 0xd0, is_10_1_e_4lane ? 60 : 30); |
There was a problem hiding this comment.
Stylistically I'd have written this as
if (of_device_is_compatible(dev->of_node, "waveshare,10.1inch-e-4lane-panel"))
ws_panel_i2c_write(ts, 0xd0, 60);
else if (of_device_is_compatible(dev->of_node, "waveshare,10.1inch-e-2lane-panel"))
ws_panel_i2c_write(ts, 0xd0, 30);
Saves having the local variables that are only really used once.
This reverts commit fd7924c. Signed-off-by: Yu Dukai <646689853@qq.com>
This reverts commit 5d31114. Signed-off-by: Yu Dukai <646689853@qq.com>
The 10.1inch DSI LCD (E) panel (1920x1200) is wired either with 4 DSI lanes running at 60fps or with 2 DSI lanes running at 30fps. Expose these as two explicit variants selected via the overlay parameter instead of runtime I2C auto-detection, keeping only the default landscape orientation. Write the corresponding refresh rate to register 0xd0 so the panel controller runs in the expected mode. Signed-off-by: Yu Dukai <646689853@qq.com>
Add the 10_1_inchE_4lane and 10_1_inchE_2lane overlay parameters matching the new panel driver compatible strings (waveshare,10.1inch-e-4lane-panel / waveshare,10.1inch-e-2lane-panel) for the 10.1inch DSI LCD (E): 1920x1200 at 60fps over 4 DSI lanes or at 30fps over 2 DSI lanes, keeping only the default landscape orientation. Signed-off-by: Yu Dukai <646689853@qq.com>
e75d2ef to
4355a7a
Compare
|
Thanks for the review, and sorry for the messy history. The branch has been rebuilt as suggested:
Please take another look. |
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
Looks good to me. I obviously haven't tested it as I don't have the hardware.
3 participants
Summary
Reverts the automatic panel detection previously added for the Waveshare 10.1inch DSI LCD (E) and replaces it with two explicit overlay parameters:
10_1_inchE_4lane— 1920x1200 @ 60fps, 4 DSI lanes10_1_inchE_2lane— 1920x1200 @ 30fps, 2 DSI lanesWhy
After the auto-detection support was merged, several people tested it and found the behaviour unfriendly for end customers:
This change removes the auto-detect code path (I2C reads, rotation handling and the portrait 1200x1920 modes) and keeps only the default landscape orientation. The panel variant is now chosen explicitly via the overlay parameter, and the driver writes the desired refresh rate to register 0xd0 accordingly.