-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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.
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
drivers: display: ssd16xx: Add support for the ssd1677 EPD driver chip #75403
base: main
Are you sure you want to change the base?
Conversation
Hello @nisargjhaveri, and thank you very much for your first pull request to the Zephyr project! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change itself looks ok to me- one formatting nit, and please address the errors highlighted by the compliance script. You should be able to see the specific lines in the Github UI when looking at the PR diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add additional "fixup" commits to address reviewer requests- squash the commits making changes requested by reviewers back into your original commit, and force push over the PR branch. If you'd like to run clang format on the PR to silence those CI warnings that is ok, but please do so in a separate commit from where you have functional changes (it seems you already did this, just want to make sure it is clear this change should not be squashed into the commit adding functionality)
98e7d77
to
4a6e631
Compare
@danieldegrasse Thanks for the suggestions. I've rebased and force pushed the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need a commit body in both commits, that is going to fail the CI checks currently
4a6e631
to
fedcf79
Compare
@danieldegrasse Updated again. Sorry for the troubles. Hope it makes sense this time? :) |
Add support for the SSD1677 EPD driver chip with support for up to 960x680 pixel displays. Tested with the Waveshare 4.26" 800x480 display with XIAO BLE board. I believe it is the same as Good Display GDEQ0426T82. Tested with the samples/drivers/display sample. The SSD1677 requires x address to be full address, instead of the byte address used by SSD16XX. Added a new quirk to handle this. The display requires a different GDO control flag as the panel layout might be different, add an option to set this. The display also requires the scan direction for y axis to be reversed, add an option to set this as well. Signed-off-by: Nisarg Jhaveri <nisargjhaveri@gmail.com>
fedcf79
to
126774c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping my review as I don't think I can add any value here. My initial comment was answered and all is clear to me.
@danieldegrasse Could you have another look? I had to push more minor changes to fix the CI checks. |
Looks like there is still an issue. You can run this check locally (on a Linux system) with |
Run `clang format` on parts of `ssd16xx.c` to format some recent changes correctly. Signed-off-by: Nisarg Jhaveri <nisargjhaveri@gmail.com>
126774c
to
f8d4fa1
Compare
Thanks for that! I pushed again, and it passes the failing check (gitlint) locally. |
Hi, @danieldegrasse. Is there anything else pending from my end? What needs to be done to merge this PR if it looks good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping, LGTM
@galak, @jfischer-no, could you please have a look as well? Let me know if this needs any more changes? |
@andysan Could you please take a look? |
I will, but it'll be a couple of days. I can't promise I'll be able to find time before the weekend. |
Add support for the SSD1677 EPD driver chip with support for up to 960x680 pixel displays.
Tested with the Waveshare 4.26" 800x480 display with XIAO BLE board. I believe it is the same as Good Display GDEQ0426T82. Tested with the samples/drivers/display sample.
The SSD1677 requires x address to be full address, instead of the byte address used by SSD16XX. Added a new quirk to handle this.
The display requires a different GDO control flag as the panel layout might be different, add an option to set this.
The display also requires the scan direction for y axis to be reversed, add an option to set this as well.
Example devicetree overlay for the display: