Tab5 ST7121 variant + fixes - #605
Conversation
Added the newest variant to the tab5, St7121. Fixed variant detection reliability Fixed tab5 camera WHO_AM_I failing sometimes Fixed tab5 keyboard live rotation on boot and after stopping/starting lvgl
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded an ST7121 MIPI-DSI driver and integrated it with Tab5 V3 detection and display initialization. Shared ST7123 touch setup now serves V2 and V3 devices. SC2356 startup can pulse a configured reset GPIO. Keyboard attachment detection now runs through a periodic timer with reinitialization and LVGL rotation handling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Drivers/sc2356-module/source/sc2356.cpp (1)
91-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve
pin_reset.flagson descriptor acquisition.The binding accepts a generic
struct GpioPinSpec, includingGPIO_FLAG_ACTIVE_LOW, butsc2356.cppcallsgpio_descriptor_acquirewith the configured value overwritten byGPIO_FLAG_DIRECTION_OUTPUT | GPIO_FLAG_ACTIVE_LOW. If a device tree intentionally needsactive.highreset polarity, it is ignored; either merge the spec flags withGPIO_FLAG_DIRECTION_OUTPUT, or change the binding to an explicit controller-plus-pin with an active-low-only API.Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a02211b0-53e9-42e7-bd62-33110d50798a
📒 Files selected for processing (29)
Devices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/devices/detect.cppDevices/m5stack-tab5/Source/devices/detect.hDevices/m5stack-tab5/Source/devices/devices_v2.cppDevices/m5stack-tab5/Source/devices/devices_v2_v3_touch.cppDevices/m5stack-tab5/Source/devices/devices_v2_v3_touch.hDevices/m5stack-tab5/Source/devices/devices_v3.cppDevices/m5stack-tab5/Source/devices/devices_v3.hDevices/m5stack-tab5/Source/devices/display_detect.cppDevices/m5stack-tab5/Source/devices/tab5_keyboard.cppDevices/m5stack-tab5/devicetree.yamlDevices/m5stack-tab5/m5stack,tab5.dtsDrivers/sc2356-module/bindings/smartsens,sc2356.yamlDrivers/sc2356-module/include/drivers/sc2356.hDrivers/sc2356-module/source/sc2356.cppDrivers/st7121-module/CMakeLists.txtDrivers/st7121-module/LICENSE-Apache-2.0.mdDrivers/st7121-module/README.mdDrivers/st7121-module/bindings/sitronix,st7121.yamlDrivers/st7121-module/devicetree.yamlDrivers/st7121-module/include/bindings/st7121.hDrivers/st7121-module/include/drivers/st7121.hDrivers/st7121-module/include/st7121_module.hDrivers/st7121-module/source/module.cppDrivers/st7121-module/source/st7121.cppFirmware/idf_component.ymlModules/lvgl-module/source/devices/devices.cppTactilityKernel/include/tactility/drivers/keyboard.hTactilityKernel/source/drivers/keyboard.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Devices/m5stack-tab5/m5stack,tab5.dts (1)
74-79: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the I/O expander labels.
The file references
&io_expander0and&io_expander1below, but both nodes are missing labels, so those phandles are undefined.Proposed fix
- io_expander0 { + io_expander0: io_expander0 { compatible = "diodes,pi4ioe5v6408"; reg = <0x43>; }; - io_expander1 { + io_expander1: io_expander1 { compatible = "diodes,pi4ioe5v6408"; reg = <0x44>; };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d504abc6-a23c-4262-868e-22a2377872ab
📒 Files selected for processing (3)
Devices/m5stack-tab5/Source/devices/detect.cppDevices/m5stack-tab5/m5stack,tab5.dtsDrivers/sc2356-module/source/sc2356.cpp
|
Thanks for the improvements! I only had 1 feedback topic wrt the keyboard <> lvgl state. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 182eaf04-03a2-4203-88f6-715b3b4e38e0
📒 Files selected for processing (6)
Devices/m5stack-tab5/Source/devices/tab5_keyboard.cppDevices/m5stack-tab5/Source/devices/tab5_keyboard.hDevices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.cppDevices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.hDevices/m5stack-tab5/Source/module.cppTactilityKernel/include/tactility/drivers/keyboard.h
💤 Files with no reviewable changes (1)
- TactilityKernel/include/tactility/drivers/keyboard.h
|
That looks good, thanks! The rabbit has some good points, and I added a suggestion in one of them. |
|
Great, thank you! |
2 participants
Added the newest variant to the tab5, St7121.
Fixed variant detection reliability
Fixed tab5 camera WHO_AM_I failing sometimes - moved reset to the driver rather than gpio hog
Fixed tab5 keyboard live rotation on boot and after stopping/starting lvgl
Summary by CodeRabbit