[Openchrome-devel] xf86-video-openchrome: src/via_display.c src/via_ums.h

[Luc Verhaegen]

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 <kevinbrace at gmx.com>
> 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 <kevinbrace at gmx.com>
> 
> 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