Skip to content
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

Jump to bottom

drivers: display: uc81xx: add support for uc8175 #61439

Merged
merged 1 commit into from Dec 11, 2023
Merged

drivers: display: uc81xx: add support for uc8175 #61439

merged 1 commit into from Dec 11, 2023

Conversation

xqinx
Copy link
Contributor

@xqinx xqinx commented Aug 11, 2023 •
edited

Add support for uc8175 display driver. uc8175 has a slightly different command/data length requirements for certain registers, namely TRES and PTL, compared to uc8176/uc8179

This commit refactors the driver code and such that setting TRES and PTL registers are now done by function pointers provided by config->quirks, by the same token as how it is done for the CDI register

@xqinx xqinx requested a review from galak as a code owner August 11, 2023 23:08
@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Display labels Aug 11, 2023
@xqinx xqinx force-pushed the uc8175 branch 2 times, most recently from 60e3910 to cd1c4a9 Compare August 12, 2023 22:11
@galak galak removed their assignment Aug 30, 2023
@MaureenHelm MaureenHelm requested review from danieldegrasse and removed request for galak September 14, 2023 15:43
jfischer-no
jfischer-no requested changes Sep 14, 2023
View reviewed changes
Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

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

Please drop all of the necessary code reformatting, then we can continue with the review.

drivers/display/uc81xx.c Outdated Show resolved Hide resolved
drivers/display/uc81xx.c Outdated Show resolved Hide resolved
drivers/display/uc81xx.c Outdated Show resolved Hide resolved
@xqinx
Copy link
Contributor Author

xqinx commented Sep 14, 2023

Please drop all of the necessary code reformatting, then we can continue with the review.

Got it, thank you!

jfischer-no
jfischer-no requested changes Sep 19, 2023
View reviewed changes
drivers/display/uc81xx.c Outdated Show resolved Hide resolved
drivers/display/uc81xx.c Outdated Show resolved Hide resolved
drivers/display/uc81xx_regs.h Outdated Show resolved Hide resolved
@xqinx xqinx force-pushed the uc8175 branch 2 times, most recently from 5ff8a20 to 6d30fe4 Compare October 4, 2023 20:26
@xqinx xqinx requested a review from jfischer-no October 4, 2023 21:12
@github-actions github-actions bot added the Stale label Dec 4, 2023
jfischer-no
jfischer-no previously approved these changes Dec 4, 2023
View reviewed changes
@zephyrproject-rtos zephyrproject-rtos deleted a comment from github-actions bot Dec 4, 2023
@jfischer-no jfischer-no removed the Stale label Dec 4, 2023
@xqinx
Copy link
Contributor Author

xqinx commented Dec 7, 2023

Hi @danieldegrasse @galak can we get a quick review to unblock the merging? Thanks

andysan
andysan suggested changes Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@andysan andysan left a comment

Choose a reason for hiding this comment

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

Thanks for this change. This looks like a good way of implementing UC8175 support. I have a minor change request (see comment below), but it looks good other than that.

drivers/display/uc81xx.c Outdated
@@ -425,12 +413,6 @@ static int uc81xx_write(const struct device *dev, const uint16_t x, const uint16
__ASSERT(!(desc->width % UC81XX_PIXELS_PER_BYTE),
"Buffer width not multiple of %d", UC81XX_PIXELS_PER_BYTE);

if ((y_end_idx > (config->height - 1)) ||
Copy link
Collaborator

@andysan andysan Dec 8, 2023 •
edited

Choose a reason for hiding this comment

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

I'd suggest keeping this check here to limit code duplication. That implies that you'll need to pass x_end_idx and y_end_idx to the quirk function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thank you!

@xqinx xqinx requested a review from andysan December 8, 2023 20:14
andysan
andysan reviewed Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@andysan andysan left a comment

Choose a reason for hiding this comment

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

Thanks for the updated change. I'm OK with this version of the change, but I would prefer it if you could address the two comments I have added in this review.

drivers/display/uc81xx.c Outdated
.vred = sys_cpu_to_be16(y_end_idx),
.flags = UC81XX_PTL_FLAG_PT_SCAN,
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

This newline seems redundant.

drivers/display/uc81xx.c Outdated
@@ -425,8 +417,7 @@ static int uc81xx_write(const struct device *dev, const uint16_t x, const uint16
__ASSERT(!(desc->width % UC81XX_PIXELS_PER_BYTE),
"Buffer width not multiple of %d", UC81XX_PIXELS_PER_BYTE);

if ((y_end_idx > (config->height - 1)) ||
(x_end_idx > (config->width - 1))) {
if ((y_end_idx > (config->height - 1)) || (x_end_idx > (config->width - 1))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice if you could keep the same formatting as as before to keep the diff as small as possible. That keeps the git annotate log clean and avoids merge conflicts in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry about that, fixed :)

@xqinx
drivers: display: uc81xx: add support for uc8175
6e906c2 
Add support for uc8175 display driver. uc8175 has a slightly
different command/data length requirements for certain registers,
namely TRES and PTL, compared to uc8176/uc8179

This commit refactors the driver code and such that setting TRES and PTL
registers are now done by function pointers provided by config->quirks,
by the same token as how it is done for setting CDI register

Signed-off-by: Xiao Qin <xiaoq@google.com>
andysan
andysan approved these changes Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@andysan andysan left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround. This looks good to me.

jfischer-no
jfischer-no approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

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

Thanks

@carlescufi carlescufi merged commit 7c46b0b into zephyrproject-rtos:main Dec 11, 2023
18 checks passed
@carlescufi
Copy link
Member

@andysan Thanks! please consider becoming a collaborator in this area:
https://docs.zephyrproject.org/latest/project/project_roles.html#collaborator

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

@andysan andysan andysan approved these changes

@jfischer-no jfischer-no jfischer-no approved these changes

@carlescufi carlescufi carlescufi approved these changes

@danieldegrasse danieldegrasse Awaiting requested review from danieldegrasse

@galak galak Awaiting requested review from galak

Assignees

@jfischer-no jfischer-no

Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Display
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@xqinx @carlescufi @andysan @jfischer-no @galak @zephyrbot