drivers: display: Introduce ST7565R #91320
Conversation
github-actions
bot
requested review from
danieldegrasse,
JarmouniA,
jfischer-no and
VynDragon
drivers/display/display_st7565r.c
Outdated
| * Copyright (c) 2025 Giyora Haroon | ||
| * | ||
| * This controller supports 8-bit parallel and 4-line SPI interfaces. | ||
| * This implementation covers the SPI interface. |
drivers/display/display_st7565r.c
Outdated
|
|
||
| /* Configure data_cmd pin if specified */ | ||
| if (config->data_cmd.port && | ||
| gpio_pin_configure_dt(&config->data_cmd, GPIO_OUTPUT_INACTIVE) < 0) { |
There was a problem hiding this comment.
Combine those statement with the previous one
drivers/display/display_st7565r.c
Outdated
| { | ||
| const struct st7565r_config *config = dev->config; | ||
| uint8_t zero_buf[config->width]; | ||
| memset(zero_buf, 0, sizeof(zero_buf)); |
There was a problem hiding this comment.
can be combined with previous statement = {0}
There was a problem hiding this comment.
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.
|
drivers/display/display_st7565r.h
Outdated
| */ | ||
|
|
||
| #ifndef ST7565R_H | ||
| #define ST7565R_H |
There was a problem hiding this comment.
Needs full path for uniqueness reasons -- check other headers for the convention to follow
There was a problem hiding this comment.
I see two different conventions (1 from ssd1306_regs, 2 from display_st7735r):
- DISPLAY_ST7565R_H
- ST7565R_DISPLAY_DRIVER_H__
Which one should it be?
There was a problem hiding this comment.
_ZEPHYR_DRIVERS_DISPLAY_ST565R_H_
drivers/display/display_st7565r.c
Outdated
| uint8_t zero_buf[ST7565R_MAX_WIDTH] = {0}; | ||
| uint8_t col_cmds[2] = {ST7565R_SET_HIGHER_COL_ADDRESS_CMD, | ||
| ST7565R_SET_LOWER_COL_ADDRESS_CMD}; | ||
| const uint8_t num_pages = (config->height + 7) / 8; |
There was a problem hiding this comment.
You might want to set ST7565R_MAX_HEIGHT to 64 or carefully review how you're handling the special "COMS" case.
I don't think the 65th line is meant to be used for displayable pixels anyway. This comment applies to multiple other places in the driver where you're treating page 8 improperly.
Datasheet says:
Page address 8 (D3, D2, D1, D0 = 1, 0, 0, 0) is a special RAM for icons, and only display data D0 is used.
There was a problem hiding this comment.
You are correct. Is setting ST7565R_MAX_HEIGHT to 64 acceptable? Maybe someone can add support for "COMS" in the future (my display doesn't support it).
There was a problem hiding this comment.
yeah that's perfectly reasonable IMO (see https://github.com/adafruit/ST7565-LCD/blob/master/ST7565/ST7565.h#L37)
drivers/display/display_st7565r.c
Outdated
| if (pf == PIXEL_FORMAT_MONO10) { /* 1=black, 0=white */ | ||
| cmd = ST7565R_SET_REVERSE_DISPLAY; | ||
| } else if (pf == PIXEL_FORMAT_MONO01) { /* 0=black, 1=white */ | ||
| cmd = ST7565R_SET_NORMAL_DISPLAY; | ||
| } else { |
There was a problem hiding this comment.
you have this backwards AFAICT (fix everywhere)
There was a problem hiding this comment.
Tested this again. My code correctly reflects the following "RAM -> LCD" mode:
Edit: I'm not sure what's going on, will debug more when I get a chance.
Edit 2: there was a bug in cfb, addressed lately in Cfb fixes #85893. My ncs workspace did not include this fix, and that was the issue. Working as expected now
henrikbrixandersen
dismissed
their stale review
GENMASK() review comment addressed.
Introduce support for Sitronix's ST7565R display controller. Tested on JHD12864 LCD display. Signed-off-by: Giyora Haroon <dev@giyorah.com>
|
kartben
dismissed
their stale review
i believe my comments to date have been addressed - thanks!
Introduce support for Sitronix's ST7565R display controller.
Tested on JHD12864 LCD display.