[Intel-gfx] [PATCH 3/4] drm/i915/vga: switch to use VGA definitions from video/vga.h

[Jani Nikula]

On Wed, 12 Jan 2022, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Tue, Jan 11, 2022 at 10:55:44AM +0200, Jani Nikula wrote:
>> On Mon, 10 Jan 2022, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> > On Mon, Jan 10, 2022 at 11:57:39AM +0200, Jani Nikula wrote:
>> >> The video/vga.h has macros for the VGA registers. Switch to use them.
>> >> 
>> >> Suggested-by: Matt Roper <matthew.d.roper at intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_vga.c | 9 +++++----
>> >>  1 file changed, 5 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
>> >> index fa779f7ea415..43c12036c1fa 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_vga.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
>> >> @@ -7,6 +7,7 @@
>> >>  #include <linux/vgaarb.h>
>> >>  
>> >>  #include <drm/i915_drm.h>
>> >> +#include <video/vga.h>
>> >>  
>> >>  #include "i915_drv.h"
>> >>  #include "intel_de.h"
>> >> @@ -34,9 +35,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
>> >>  
>> >>  	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
>> >>  	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
>> >> -	outb(SR01, VGA_SR_INDEX);
>> >> -	sr1 = inb(VGA_SR_DATA);
>> >> -	outb(sr1 | 1 << 5, VGA_SR_DATA);
>> >> +	outb(VGA_SEQ_CLOCK_MODE, VGA_SEQ_I);
>> >
>> > Not a huge fan of some of these defines since now I have
>> > no idea what register this is selecting.
>> 
>> It's a bit silly that we have our own macros for this stuff, but I get
>> the point. Took me a while to figure the changes out because the macros
>> in video/vga.h aren't even grouped in a helpful way.
>> 
>> I guess you'd prefer patch [1] over patches 3-4 in this series then? For
>> me the main goal is to just reduce the size of i915_reg.h.
>
> I guess another option is to go with this and just
> s/VGA_SEQ_CLOCK_MODE/0x01/ or something. I think the rest
> of the defines are probably clear enough.

I dropped the ball here a bit. If I respin the same patches, but with
the above line changed to one of these, is it okay? Which do you prefer?

1)	outb(VGA_SEQ_CLOCK_MODE, VGA_SEQ_I); /* SR01 */

2)	#define SR01 VGA_SEQ_CLOCK_MODE
	outb(SR01, VGA_SEQ_I);

3)	outb(0x01, VGA_SEQ_I);


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center