Wow. The new funtion version is much more simple and clean. You don't need any commment to the function, because the name of the function is describing by ownself. (the comment will only repeat what was already written in function name) So every invocation of this function will be fully undestandable. Nice catch. Small nitpick: Screen_State variable is using underscore for word separation. In future it will be great to be consistent with the rest part of the code and separate it with camel case. So in that case it could be: ScreenState Also what do you think about convention: - variable names starts from small letter - Type names starts from capital letter ? Best Regards Bartosz W dniu wtorek, 26 kwietnia 2016 Luc Verhaegen <<a href="mailto:libv@skynet.be">libv@skynet.be</a>> napisał(a): > On Tue, Apr 26, 2016 at 09:38:52AM -0700, Kevin Brace wrote: >> src/via_display.c | 24 ++++++++++++++++++++++++ >> src/via_ums.h | 1 + >> 2 files changed, 25 insertions(+) >> >> New commits: >> commit 479b2ab119d05c22e1541f4cf25fe816419a86e9 >> Author: Kevin Brace <<a href="mailto:kevinbrace@gmx.com">kevinbrace@gmx.com</a>> >> Date: Tue Apr 26 09:31:58 2016 -0700 >> >> Turning off IGA2 correctly >> >> Added viaIGA2Screen function to via_display.c. >> >> Signed-off-by: Kevin Brace <<a href="mailto:kevinbrace@gmx.com">kevinbrace@gmx.com</a>> >> >> diff --git a/src/via_display.c b/src/via_display.c >> index 0fe0c13..8dbfeea 100644 >> --- a/src/via_display.c >> +++ b/src/via_display.c >> @@ -49,6 +49,26 @@ viaIGA1DPMSControl(ScrnInfoPtr pScrn, CARD8 DPMS_Control) >> } >> >> /* >> + * Controls IGA2 screen state. >> + */ >> +void >> +viaIGA2Screen(ScrnInfoPtr pScrn, Bool Screen_State) >> +{ >> + vgaHWPtr hwp = VGAHWPTR(pScrn); >> + >> + DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO, >> + "Entered viaIGA2Screen.\n")); >> + >> + /* 3X5.6B[2]: IGA2 Screen Off >> + * 0 = Screen on >> + * 1 = Screen off */ >> + ViaCrtcMask(hwp, 0x6B, Screen_State ? 0x00 : 0x04, 0x04); > > I wrote the mask functions to aid readability. > > By naming the variable "Screen_State" with a bool, you removed > readability. If you had named it "Enabled", things would be clearer. > > If you had called the function viaIGA2Enable, things would be > clearer as well. > > You would then not need to add a snippet of datasheet information. > > Also, by using Mask, you improve readability several orders of > magnitude, but by using the ? : construct, you remove readability. > > This here > > void > viaIGA2Enable(ScrnInfoPtr pScrn, Bool Enable) > { > vgaHWPtr hwp = VGAHWPTR(pScrn); > > DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO, "%s().\n", __FUNC__)); > > if (Enable) > ViaCrtcMask(hwp, 0x6B, 0, 0x04); > else > ViaCrtcMask(hwp, 0x6B, 0x04, 0x04); > } > > requires absolutely no mental arithmetic and needs no comment. > > Less is more. > > Note that i am, after almost a decade, no longer sure whether 0x6B 0x04 > was Crtc enable, or Crtc scanout enable. But that only means that your > previous version was not verbose enough, or was anti-verbose. > > Luc Verhaegen > _______________________________________________ > Openchrome-devel mailing list > <a href="mailto:Openchrome-devel@lists.freedesktop.org">Openchrome-devel@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/openchrome-devel">https://lists.freedesktop.org/mailman/listinfo/openchrome-devel</a> >