[PATCH 2/2] drm/mgag200: Add vblank support

[Gerd Hoffmann]

On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> Support for vblank requires VSYNC to signal an interrupt, which is broken
> on Matrox chipsets. The workaround that is used here and in other free
> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> enable the VLINE interrupt. This triggers an interrupt at the same time
> when VSYNC begins.
> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.

Looks good overall, some minor nits ...

> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, iclear;
> +
> +	status = RREG32(0x1e14);
> +
> +	if (status & 0x00000020) { /* test <vlinepen> */
> +		drm_for_each_crtc(crtc, dev) {
> +			drm_crtc_handle_vblank(crtc);
> +		}
> +		iclear = RREG32(0x1e18);
> +		iclear |= 0x00000020; /* set <vlineiclr> */

#define for this would be good (you also don't need the comment then).

> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use

Codestyle.  "/*" should be on a separate line.

> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 ien;
> +
> +	ien = RREG32(0x1e1c);
> +	ien &= 0xffffffdf; /* clear <vlineien> */

That is typically written as value &= ~(bit);

cheers,
  Gerd