Skip to content

Conversation

giyorah
Copy link

@giyorah giyorah commented Jun 9, 2025

Introduce support for Sitronix's ST7565R display controller.
Tested on JHD12864 LCD display.

Copy link
Contributor

@JarmouniA JarmouniA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A node is needed in tests/drivers/build_all/display/app.overlay to build controller code in CI.


/* Configure data_cmd pin if specified */
if (config->data_cmd.port &&
gpio_pin_configure_dt(&config->data_cmd, GPIO_OUTPUT_INACTIVE) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine those statement with the previous one

{
const struct st7565r_config *config = dev->config;
uint8_t zero_buf[config->width];
memset(zero_buf, 0, sizeof(zero_buf));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be combined with previous statement = {0}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Width does come from a static value in the devicetree. However, since config->width is a runtime value, zero_buf is considered a variable length array, which can't be initialized with = {0}. I'll have to stick with the memset call here.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments needs to be resolved, not just marked as resolved.

Copy link

@JarmouniA JarmouniA dismissed their stale review June 13, 2025 08:58

addressed

@henrikbrixandersen henrikbrixandersen dismissed their stale review June 18, 2025 09:59

GENMASK() review comment addressed.

@giyorah giyorah requested a review from kartben July 17, 2025 20:44
@kartben kartben dismissed their stale review July 22, 2025 12:06

i believe my comments to date have been addressed - thanks!

@giyorah
Copy link
Author

giyorah commented Jul 28, 2025

@kartben @JarmouniA ping. Can it be merged?

@giyorah giyorah requested a review from JarmouniA July 31, 2025 05:35
@kartben kartben requested a review from Copilot July 31, 2025 06:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Introduces support for the Sitronix ST7565R display controller driver, a dot matrix LCD controller that communicates over SPI. This adds comprehensive driver implementation with device tree bindings for configuration and test infrastructure.

  • Adds complete ST7565R driver implementation with SPI communication support
  • Provides device tree bindings with configurable LCD properties (bias, power control, booster ratio)
  • Includes build system integration and test overlay configuration

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
drivers/display/display_st7565r.c Main driver implementation with initialization, display operations, and SPI communication
drivers/display/display_st7565r.h Command definitions and constants for ST7565R controller
dts/bindings/display/sitronix,st7565r.yaml Device tree binding specification with configuration properties
drivers/display/Kconfig.st7565r Kconfig configuration for ST7565R driver
drivers/display/Kconfig Integration of ST7565R Kconfig into main display configuration
drivers/display/CMakeLists.txt Build system integration for ST7565R driver
tests/drivers/build_all/display/app.overlay Test configuration overlay for build verification

Comment on lines +468 to +469
/* Specify timeout as an absolute time since system boot */
k_sleep(K_TIMEOUT_ABS_MS(config->ready_time_ms));
Copy link

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using K_TIMEOUT_ABS_MS() for a delay is incorrect. This function expects an absolute timestamp since boot, not a relative delay. Use K_MSEC(config->ready_time_ms) instead for a relative delay.

Suggested change
/* Specify timeout as an absolute time since system boot */
k_sleep(K_TIMEOUT_ABS_MS(config->ready_time_ms));
/* Specify timeout as a relative delay in milliseconds */
k_sleep(K_MSEC(config->ready_time_ms));

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. ready-time-ms: time in milliseconds to wait before initializing the controller after power-on.

@github-actions github-actions bot added the Stale label Oct 7, 2025
@giyorah
Copy link
Author

giyorah commented Oct 8, 2025

Commenting to have the stale label removed.

@henrikbrixandersen
Copy link
Member

Rebased on main and resolve CI issue.

@kartben kartben closed this Oct 8, 2025
@kartben kartben reopened this Oct 8, 2025
@kartben
Copy link
Contributor

kartben commented Oct 8, 2025

Rebased on main and resolve CI issue.

Well CI issue was the runner crashing so trying to re-run it, let's see how this goes

@zephyrbot zephyrbot added area: Tests Issues related to a particular existing or missing test area: Boards/SoCs area: Devicetree Bindings labels Oct 8, 2025
@zephyrbot zephyrbot requested a review from nashif October 8, 2025 18:39
@nashif nashif removed their assignment Oct 9, 2025
nashif
nashif previously approved these changes Oct 9, 2025
@giyorah
Copy link
Author

giyorah commented Oct 9, 2025

Rebased on main and resolve CI issue.

Well CI issue was the runner crashing so trying to re-run it, let's see how this goes

Thanks, @kartben.

I see that it still fails (specifically, the 'twister-build-prep' job). Something to do with ModuleNotFoundError: No module named 'jsonschema'. Is there a way around this?

@kartben
Copy link
Contributor

kartben commented Oct 9, 2025

Rebased on main and resolve CI issue.

Well CI issue was the runner crashing so trying to re-run it, let's see how this goes

Thanks, @kartben.

I see that it still fails (specifically, the 'twister-build-prep' job). Something to do with ModuleNotFoundError: No module named 'jsonschema'. Can I run it locally or something? Is there a way around this?

please rebase your branch on main (and then force push) -- this will do the trick!

@kartben
Copy link
Contributor

kartben commented Oct 9, 2025

@giyorah

git reset bc78471694aed8447d3d73a999b8df47eceec183 --hard
git rebase origin/main
git push origin --force

(assuming that you have your fork repo named origin)

Introduce support for Sitronix's ST7565R display controller.
Tested on JHD12864 LCD display.

Signed-off-by: Giyora Haroon <[email protected]>
@zephyrproject-rtos zephyrproject-rtos deleted a comment from kartben Oct 11, 2025
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate header file is not needed.

.set_pixel_format = st7565r_set_pixel_format,
};

#define DT_DRV_COMPAT sitronix_st7565r
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this at the beginning of the file so it's easily found.


#define DT_DRV_COMPAT sitronix_st7565r

#if DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) == 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, Kconfig takes care of this with depends on DT_HAS_SITRONIX_ST7565R_ENABLED

# Copyright (c) 2025, Giyora Haroon
# SPDX-License-Identifier: Apache-2.0

title: Sitronix ST7565R dot matrix LCD controller (SPI)
Copy link
Contributor

@JarmouniA JarmouniA Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: Sitronix ST7565R dot matrix LCD controller (SPI)
title: ST7565R LCD controller (SPI)

Non-blocking

@giyorah giyorah closed this Oct 11, 2025
@JarmouniA JarmouniA reopened this Oct 11, 2025
@giyorah giyorah closed this Oct 11, 2025
@giyorah giyorah deleted the st7565r branch October 11, 2025 18:45

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards/SoCs area: Devicetree Bindings area: Display area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants