Re: [PATCH 4/4] drm: zte: add VGA driver support

From: Shawn Guo <shawnguo@kernel.org>
To: Sean Paul <seanpaul@chromium.org>
Cc: Xin Zhou <zhou.xin8@zte.com.cn>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Baoyou Xie <xie.baoyou@zte.com.cn>,
	dri-devel@lists.freedesktop.org, Jun Nie <jun.nie@linaro.org>
Subject: Re: [PATCH 4/4] drm: zte: add VGA driver support
Date: Wed, 5 Apr 2017 22:08:21 +0800	[thread overview]
Message-ID: <20170405140815.GG3394@dragon> (raw)
In-Reply-To: <20170404174648.4khdo24dlmrfswuu@art_vandelay>

On Tue, Apr 04, 2017 at 01:46:48PM -0400, Sean Paul wrote:
> > +static int zx_vga_register(struct drm_device *drm, struct zx_vga *vga)
> > +{
> > +	struct drm_encoder *encoder = &vga->encoder;
> > +	struct drm_connector *connector = &vga->connector;
> > +
> > +	encoder->possible_crtcs = VOU_CRTC_MASK;
> > +
> > +	drm_encoder_init(drm, encoder, &zx_vga_encoder_funcs,
> > +			 DRM_MODE_ENCODER_DAC, NULL);
> > +	drm_encoder_helper_add(encoder, &zx_vga_encoder_helper_funcs);
> > +
> > +	vga->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +	drm_connector_init(drm, connector, &zx_vga_connector_funcs,
> > +			   DRM_MODE_CONNECTOR_VGA);
> > +	drm_connector_helper_add(connector, &zx_vga_connector_helper_funcs);
> > +
> > +	drm_mode_connector_attach_encoder(connector, encoder);
> 
> You're missing a couple of return value checks in this function.

Okay.  I will add return checks in the new version.

> 
> > +
> > +	return 0;
> > +}

<snip>

> > +static irqreturn_t zx_vga_irq_handler(int irq, void *dev_id)
> > +{
> > +	struct zx_vga *vga = dev_id;
> > +	u32 status;
> > +
> > +	status = zx_readl(vga->mmio + VGA_I2C_STATUS);
> > +
> > +	/* Clear interrupt status */
> > +	zx_writel_mask(vga->mmio + VGA_I2C_STATUS, VGA_CLEAR_IRQ,
> > +		       VGA_CLEAR_IRQ);
> > +
> > +	if (status & VGA_DEVICE_CONNECTED) {
> > +		/*
> > +		 * We should ideally set up VGA_DETECT_SEL_HAS_DEVICE bit here
> > +		 * for unplug detection, but doing so will stop DDC bus from
> > +		 * reading EDID later on. It looks like a HW limitation, and we
> > +		 * work around it by defering the bit setup to .get_modes hook
> > +		 * after EDID read succeeds.
> 
> get_modes resets VGA_AUTO_DETECT_SEL before and after reading edid, so is it
> really necessary to defer setting it here?

Okay, the comment in the code seems confusing.  Since we need to reset
VGA_AUTO_DETECT_SEL bits to switch DDC bus from device detecting to EDID
reading, VGA_DETECT_SEL_HAS_DEVICE will need to be set after reading
EDID anyway.  I will improve the comment.

> 
> > +		 */
> > +		vga->connected = true;
> > +		return IRQ_WAKE_THREAD;
> > +	}
> > +
> > +	if (status & VGA_DEVICE_DISCONNECTED) {
> > +		zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL,
> > +			  VGA_DETECT_SEL_NO_DEVICE);
> 
> This races with get_modes, you should serialize access to this register.

Good catch.  I will add lock for it.

Shawn

> 
> > +		vga->connected = false;
> > +		return IRQ_WAKE_THREAD;
> > +	}
> > +
> > +	if (status & VGA_TRANS_DONE) {
> > +		complete(&vga->complete);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel