Skip to content

drivers: display: ssd16xx: implement display_set_orientation #73360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nashif merged 3 commits into from
Jun 4, 2024
Merged

drivers: display: ssd16xx: implement display_set_orientation #73360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
deb1b18
drivers: display: ssd16xx: implement display_set_orientation API
Snevzor May 27, 2024
0637eaa
drivers: display: ssd16xx: performance improvement
Snevzor May 27, 2024
93b5d12
drivers: display: ili9xxx: add justification for default rotation
Snevzor May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 117 additions & 43 deletions drivers/display/ssd16xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ struct ssd16xx_data {
uint8_t scan_mode;
bool blanking_on;
enum ssd16xx_profile_type profile;
enum display_orientation orientation;
};

struct ssd16xx_dt_array {
Expand Down Expand Up @@ -100,7 +101,7 @@ struct ssd16xx_config {

const struct ssd16xx_profile *profiles[SSD16XX_NUM_PROFILES];

bool orientation;
uint16_t rotation;
uint16_t height;
uint16_t width;
uint8_t tssv;
Expand Down Expand Up @@ -380,53 +381,78 @@ static int ssd16xx_set_window(const struct device *dev,
return -ENOTSUP;
}

if ((y + desc->height) > panel_h) {
LOG_ERR("Buffer out of bounds (height)");
return -EINVAL;
}
if (data->orientation == DISPLAY_ORIENTATION_NORMAL ||
data->orientation == DISPLAY_ORIENTATION_ROTATED_180) {
if ((y + desc->height) > panel_h) {
LOG_ERR("Buffer out of bounds (height)");
return -EINVAL;
}

if ((x + desc->width) > config->width) {
LOG_ERR("Buffer out of bounds (width)");
return -EINVAL;
}
if ((x + desc->width) > config->width) {
LOG_ERR("Buffer out of bounds (width)");
return -EINVAL;
}

if ((desc->height % EPD_PANEL_NUMOF_ROWS_PER_PAGE) != 0U) {
LOG_ERR("Buffer height not multiple of %d",
EPD_PANEL_NUMOF_ROWS_PER_PAGE);
return -EINVAL;
}
if ((desc->height % EPD_PANEL_NUMOF_ROWS_PER_PAGE) != 0U) {
LOG_ERR("Buffer height not multiple of %d", EPD_PANEL_NUMOF_ROWS_PER_PAGE);
return -EINVAL;
}

if ((y % EPD_PANEL_NUMOF_ROWS_PER_PAGE) != 0U) {
LOG_ERR("Y coordinate not multiple of %d",
EPD_PANEL_NUMOF_ROWS_PER_PAGE);
return -EINVAL;
if ((y % EPD_PANEL_NUMOF_ROWS_PER_PAGE) != 0U) {
LOG_ERR("Y coordinate not multiple of %d", EPD_PANEL_NUMOF_ROWS_PER_PAGE);
return -EINVAL;
}
} else {
if ((y + desc->height) > config->width) {
LOG_ERR("Buffer out of bounds (height)");
return -EINVAL;
}

if ((x + desc->width) > panel_h) {
LOG_ERR("Buffer out of bounds (width)");
return -EINVAL;
}

if ((desc->width % SSD16XX_PIXELS_PER_BYTE) != 0U) {
LOG_ERR("Buffer width not multiple of %d", SSD16XX_PIXELS_PER_BYTE);
return -EINVAL;
}

if ((x % SSD16XX_PIXELS_PER_BYTE) != 0U) {
LOG_ERR("X coordinate not multiple of %d", SSD16XX_PIXELS_PER_BYTE);
return -EINVAL;
}
}

switch (data->scan_mode) {
case SSD16XX_DATA_ENTRY_XIYDY:
switch (data->orientation) {
case DISPLAY_ORIENTATION_NORMAL:
x_start = (panel_h - 1 - y) / SSD16XX_PIXELS_PER_BYTE;
x_end = (panel_h - 1 - (y + desc->height - 1)) / SSD16XX_PIXELS_PER_BYTE;
y_start = x;
y_end = (x + desc->width - 1);
break;
case DISPLAY_ORIENTATION_ROTATED_90:
x_start = (panel_h - 1 - x) / SSD16XX_PIXELS_PER_BYTE;
x_end = (panel_h - 1 - (x + desc->width - 1)) / SSD16XX_PIXELS_PER_BYTE;
y_start = (config->width - 1 - y);
y_end = (config->width - 1 - (y + desc->height - 1));
break;
case DISPLAY_ORIENTATION_ROTATED_180:
x_start = y / SSD16XX_PIXELS_PER_BYTE;
x_end = (y + desc->height - 1) / SSD16XX_PIXELS_PER_BYTE;
y_start = (x + desc->width - 1);
y_end = x;
break;

case SSD16XX_DATA_ENTRY_XDYIY:
x_start = (panel_h - 1 - y) / SSD16XX_PIXELS_PER_BYTE;
x_end = (panel_h - 1 - (y + desc->height - 1)) /
SSD16XX_PIXELS_PER_BYTE;
y_start = x;
y_end = (x + desc->width - 1);
case DISPLAY_ORIENTATION_ROTATED_270:
x_start = x / SSD16XX_PIXELS_PER_BYTE;
x_end = (x + desc->width - 1) / SSD16XX_PIXELS_PER_BYTE;
y_start = y;
y_end = (y + desc->height - 1);
Copy link
Contributor

@danieldegrasse danieldegrasse May 30, 2024

Choose a reason for hiding this comment

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

Do we still need to write the scan_mode here, since we do it within the set_orientation function? IE will the scan mode setting persist between display frames, or does it need to be set every time?

Copy link
Contributor Author

@Snevzor Snevzor May 30, 2024

Choose a reason for hiding this comment

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

Thanks for reviewing @danieldegrasse!

In the second commit I removed writing the scan_mode on each set_window call because I didn't find any indication in the datasheet that this should not persist between resets. So I tested it with the two mentioned display controllers and it worked so that seems to prove that the scan_mode indeed persists between display frames.
Because there is a software reset in the set_profile function I also added writing the scan_mode over there.

I do wonder why it was added to the set_window function in the first place instead of in the controller_init so I completely understand your doubts.
If anyone with a ssd16xx laying around could double check this, that would be great.
@andysan, I see you have made some amazing improvements to this driver in the past, would you mind checking this as well?

break;
default:
return -EINVAL;
}

err = ssd16xx_write_cmd(dev, SSD16XX_CMD_ENTRY_MODE,
&data->scan_mode, sizeof(data->scan_mode));
if (err < 0) {
return err;
}

err = ssd16xx_set_ram_param(dev, x_start, x_end, y_start, y_end);
if (err < 0) {
return err;
Expand Down Expand Up @@ -585,16 +611,22 @@ static void ssd16xx_get_capabilities(const struct device *dev,
struct display_capabilities *caps)
{
const struct ssd16xx_config *config = dev->config;
struct ssd16xx_data *data = dev->data;

memset(caps, 0, sizeof(struct display_capabilities));
caps->x_resolution = config->width;
caps->y_resolution = config->height -
config->height % EPD_PANEL_NUMOF_ROWS_PER_PAGE;
caps->supported_pixel_formats = PIXEL_FORMAT_MONO10;
caps->current_pixel_format = PIXEL_FORMAT_MONO10;
caps->screen_info = SCREEN_INFO_MONO_VTILED |
SCREEN_INFO_MONO_MSB_FIRST |
SCREEN_INFO_EPD;
caps->screen_info = SCREEN_INFO_MONO_MSB_FIRST | SCREEN_INFO_EPD;

if (data->orientation == DISPLAY_ORIENTATION_NORMAL ||
data->orientation == DISPLAY_ORIENTATION_ROTATED_180) {
caps->screen_info |= SCREEN_INFO_MONO_VTILED;
}

caps->current_orientation = data->orientation;
}

static int ssd16xx_set_pixel_format(const struct device *dev,
Expand All @@ -608,6 +640,32 @@ static int ssd16xx_set_pixel_format(const struct device *dev,
return -ENOTSUP;
}

static int ssd16xx_set_orientation(const struct device *dev,
const enum display_orientation orientation)
{
struct ssd16xx_data *data = dev->data;
int err;

if (orientation == DISPLAY_ORIENTATION_NORMAL) {
data->scan_mode = SSD16XX_DATA_ENTRY_XDYIY;
} else if (orientation == DISPLAY_ORIENTATION_ROTATED_90) {
data->scan_mode = SSD16XX_DATA_ENTRY_XDYDX;
} else if (orientation == DISPLAY_ORIENTATION_ROTATED_180) {
data->scan_mode = SSD16XX_DATA_ENTRY_XIYDY;
} else if (orientation == DISPLAY_ORIENTATION_ROTATED_270) {
data->scan_mode = SSD16XX_DATA_ENTRY_XIYIX;
}

err = ssd16xx_write_uint8(dev, SSD16XX_CMD_ENTRY_MODE, data->scan_mode);
if (err < 0) {
return err;
}

data->orientation = orientation;

return 0;
}

static int ssd16xx_clear_cntlr_mem(const struct device *dev, uint8_t ram_cmd)
{
const struct ssd16xx_config *config = dev->config;
Expand Down Expand Up @@ -831,6 +889,11 @@ static int ssd16xx_set_profile(const struct device *dev,
}
}

err = ssd16xx_write_uint8(dev, SSD16XX_CMD_ENTRY_MODE, data->scan_mode);
if (err < 0) {
return err;
}

data->profile = type;

return 0;
Expand All @@ -840,6 +903,7 @@ static int ssd16xx_controller_init(const struct device *dev)
{
const struct ssd16xx_config *config = dev->config;
struct ssd16xx_data *data = dev->data;
enum display_orientation orientation;
int err;

LOG_DBG("");
Expand All @@ -860,12 +924,6 @@ static int ssd16xx_controller_init(const struct device *dev)

k_msleep(SSD16XX_RESET_DELAY);

if (config->orientation == 1) {
data->scan_mode = SSD16XX_DATA_ENTRY_XIYDY;
} else {
data->scan_mode = SSD16XX_DATA_ENTRY_XDYIY;
}

err = ssd16xx_set_profile(dev, SSD16XX_PROFILE_FULL);
if (err < 0) {
return err;
Expand All @@ -881,6 +939,21 @@ static int ssd16xx_controller_init(const struct device *dev)
return err;
}

if (config->rotation == 0U) {
orientation = DISPLAY_ORIENTATION_NORMAL;
} else if (config->rotation == 90U) {
orientation = DISPLAY_ORIENTATION_ROTATED_90;
} else if (config->rotation == 180U) {
orientation = DISPLAY_ORIENTATION_ROTATED_180;
} else {
orientation = DISPLAY_ORIENTATION_ROTATED_270;
}

err = ssd16xx_set_orientation(dev, orientation);
if (err < 0) {
return err;
}

err = ssd16xx_update_display(dev);
if (err < 0) {
return err;
Expand Down Expand Up @@ -954,6 +1027,7 @@ static struct display_driver_api ssd16xx_driver_api = {
.read = ssd16xx_read,
.get_capabilities = ssd16xx_get_capabilities,
.set_pixel_format = ssd16xx_set_pixel_format,
.set_orientation = ssd16xx_set_orientation,
};

#if DT_HAS_COMPAT_STATUS_OKAY(solomon_ssd1608)
Expand Down Expand Up @@ -1070,7 +1144,7 @@ static struct ssd16xx_quirks quirks_solomon_ssd1681 = {
.quirks = quirks_ptr, \
.height = DT_PROP(n, height), \
.width = DT_PROP(n, width), \
.orientation = DT_PROP(n, orientation_flipped), \
.rotation = DT_PROP(n, rotation), \
.tssv = DT_PROP_OR(n, tssv, 0), \
.softstart = SSD16XX_ASSIGN_ARRAY(n, softstart), \
.profiles = { \
Expand Down
1 change: 1 addition & 0 deletions dts/bindings/display/ilitek,ili9xxx-common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ properties:
- 270
description:
Display rotation (CW) in degrees.
If not defined, rotation is off by default.

display-inversion:
type: boolean
Expand Down
16 changes: 12 additions & 4 deletions dts/bindings/display/solomon,ssd16xx-common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ properties:
type: uint8-array
description: Booster soft start values

orientation-flipped:
type: boolean
description: Last column address is mapped to first segment

reset-gpios:
type: phandle-array
required: true
Expand Down Expand Up @@ -49,6 +45,18 @@ properties:
an external temperature sensor is connected to the controller.
The value selects which sensor should be used.

rotation:
type: int
default: 0
Copy link
Member

@decsny decsny May 28, 2024

Choose a reason for hiding this comment

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

default needs justification

https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values

Copy link
Contributor Author

@Snevzor Snevzor May 28, 2024

Choose a reason for hiding this comment

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

Thanks for checking and pointing out @decsny. I looked at existing bindings to not re-invent the wheel. This example also lacks the justification: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/display/ilitek%2Cili9xxx-common.yaml#L23

I really struggle to find a good justification as a description for this. It does not correspond to default values after reset, nor is it a manufacturer recommendation.

The default makes sense for when not interested in the rotation property, and it helps to avoid breaking existing sdd16xx usages.

Can you help find a suitable description? Or do I need to remove the default if it is not possible to give a good justification?

Copy link
Member

@decsny decsny May 28, 2024

Choose a reason for hiding this comment

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

You can say rotation is off by default, for properties like this where one of the values corresponds to some kind of "none" meaning. The binding you linked is wrong for not clarifying this. Since you found it, please add a commit to fix it

Copy link
Contributor Author

@Snevzor Snevzor May 29, 2024

Choose a reason for hiding this comment

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

Thanks for the excellent suggestion. I've updated this and added a commit for ili9xxx.

enum:
- 0
- 90
- 180
- 270
description:
Display rotation (CW) in degrees.
If not defined, rotation is off by default.

child-binding:
description: |
Child nodes describe refresh profiles. Each refresh profile
Expand Down