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

Message ID	1490098556-23853-5-git-send-email-shawnguo@kernel.org (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> From: Shawn Guo <shawnguo@kernel.org> To: dri-devel@lists.freedesktop.org Subject: [PATCH 4/4] drm: zte: add VGA driver support Date: Tue, 21 Mar 2017 20:15:56 +0800 Message-Id: <1490098556-23853-5-git-send-email-shawnguo@kernel.org> In-Reply-To: <1490098556-23853-1-git-send-email-shawnguo@kernel.org> References: <1490098556-23853-1-git-send-email-shawnguo@kernel.org> Cc: Xin Zhou <zhou.xin8@zte.com.cn>, Daniel Vetter <daniel.vetter@ffwll.ch>, Baoyou Xie <xie.baoyou@zte.com.cn>, Jun Nie <jun.nie@linaro.org> Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>

Shawn Guo March 21, 2017, 12:15 p.m. UTC

From: Shawn Guo <shawn.guo@linaro.org>

It adds VGA driver support, which needs to configure corresponding VOU
interface in RGB_888 format, and thus the following changes are needed
on zx_vou.

- Rename the CSC block of Graphic Layer a bit to make it more specific,
  and add CSC of Channel to support RGB output.
- Bypass Dither block for RGB output.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpu/drm/zte/Makefile      |   1 +
 drivers/gpu/drm/zte/zx_drm_drv.c  |   1 +
 drivers/gpu/drm/zte/zx_drm_drv.h  |   1 +
 drivers/gpu/drm/zte/zx_vga.c      | 500 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/zte/zx_vga_regs.h |  36 +++
 drivers/gpu/drm/zte/zx_vou.c      |  33 ++-
 drivers/gpu/drm/zte/zx_vou_regs.h |  12 +-
 7 files changed, 580 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/zte/zx_vga.c
 create mode 100644 drivers/gpu/drm/zte/zx_vga_regs.h

Daniel Vetter April 3, 2017, 9:23 a.m. UTC | #1

On Tue, Mar 21, 2017 at 08:15:56PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> It adds VGA driver support, which needs to configure corresponding VOU
> interface in RGB_888 format, and thus the following changes are needed
> on zx_vou.
> 
> - Rename the CSC block of Graphic Layer a bit to make it more specific,
>   and add CSC of Channel to support RGB output.
> - Bypass Dither block for RGB output.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpu/drm/zte/Makefile      |   1 +
>  drivers/gpu/drm/zte/zx_drm_drv.c  |   1 +
>  drivers/gpu/drm/zte/zx_drm_drv.h  |   1 +
>  drivers/gpu/drm/zte/zx_vga.c      | 500 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/zte/zx_vga_regs.h |  36 +++
>  drivers/gpu/drm/zte/zx_vou.c      |  33 ++-
>  drivers/gpu/drm/zte/zx_vou_regs.h |  12 +-
>  7 files changed, 580 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/zte/zx_vga.c
>  create mode 100644 drivers/gpu/drm/zte/zx_vga_regs.h
> 
> diff --git a/drivers/gpu/drm/zte/Makefile b/drivers/gpu/drm/zte/Makefile
> index 01352b56c418..9df7766a7f9d 100644
> --- a/drivers/gpu/drm/zte/Makefile
> +++ b/drivers/gpu/drm/zte/Makefile
> @@ -3,6 +3,7 @@ zxdrm-y := \
>  	zx_hdmi.o \
>  	zx_plane.o \
>  	zx_tvenc.o \
> +	zx_vga.o \
>  	zx_vou.o
>  
>  obj-$(CONFIG_DRM_ZTE) += zxdrm.o
> diff --git a/drivers/gpu/drm/zte/zx_drm_drv.c b/drivers/gpu/drm/zte/zx_drm_drv.c
> index 5c6944a1e72c..8a6892eeb44f 100644
> --- a/drivers/gpu/drm/zte/zx_drm_drv.c
> +++ b/drivers/gpu/drm/zte/zx_drm_drv.c
> @@ -248,6 +248,7 @@ static int zx_drm_remove(struct platform_device *pdev)
>  	&zx_crtc_driver,
>  	&zx_hdmi_driver,
>  	&zx_tvenc_driver,
> +	&zx_vga_driver,
>  	&zx_drm_platform_driver,
>  };
>  
> diff --git a/drivers/gpu/drm/zte/zx_drm_drv.h b/drivers/gpu/drm/zte/zx_drm_drv.h
> index 5ca035b079c7..2a8cdc5f8be4 100644
> --- a/drivers/gpu/drm/zte/zx_drm_drv.h
> +++ b/drivers/gpu/drm/zte/zx_drm_drv.h
> @@ -14,6 +14,7 @@
>  extern struct platform_driver zx_crtc_driver;
>  extern struct platform_driver zx_hdmi_driver;
>  extern struct platform_driver zx_tvenc_driver;
> +extern struct platform_driver zx_vga_driver;
>  
>  static inline u32 zx_readl(void __iomem *reg)
>  {
> diff --git a/drivers/gpu/drm/zte/zx_vga.c b/drivers/gpu/drm/zte/zx_vga.c
> new file mode 100644
> index 000000000000..35eaf98458af
> --- /dev/null
> +++ b/drivers/gpu/drm/zte/zx_vga.c
> @@ -0,0 +1,500 @@
> +/*
> + * Copyright (C) 2017 Sanechips Technology Co., Ltd.
> + * Copyright 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drmP.h>
> +
> +#include "zx_drm_drv.h"
> +#include "zx_vga_regs.h"
> +#include "zx_vou.h"
> +
> +struct zx_vga_pwrctrl {
> +	struct regmap *regmap;
> +	u32 reg;
> +	u32 mask;
> +};
> +
> +struct zx_vga_i2c {
> +	struct i2c_adapter adap;
> +	struct mutex lock;
> +};
> +
> +struct zx_vga {
> +	struct drm_connector connector;
> +	struct drm_encoder encoder;
> +	struct zx_vga_i2c *ddc;
> +	struct device *dev;
> +	void __iomem *mmio;
> +	struct clk *i2c_wclk;
> +	struct zx_vga_pwrctrl pwrctrl;
> +	struct completion complete;
> +	bool connected;
> +};
> +
> +#define to_zx_vga(x) container_of(x, struct zx_vga, x)
> +
> +static void zx_vga_encoder_enable(struct drm_encoder *encoder)
> +{
> +	struct zx_vga *vga = to_zx_vga(encoder);
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +
> +	/* Set bit to power up VGA DACs */
> +	regmap_update_bits(pwrctrl->regmap, pwrctrl->reg, pwrctrl->mask,
> +			   pwrctrl->mask);
> +
> +	vou_inf_enable(VOU_VGA, encoder->crtc);
> +}
> +
> +static void zx_vga_encoder_disable(struct drm_encoder *encoder)
> +{
> +	struct zx_vga *vga = to_zx_vga(encoder);
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +
> +	vou_inf_disable(VOU_VGA, encoder->crtc);
> +
> +	/* Clear bit to power down VGA DACs */
> +	regmap_update_bits(pwrctrl->regmap, pwrctrl->reg, pwrctrl->mask, 0);
> +}
> +
> +static const struct drm_encoder_helper_funcs zx_vga_encoder_helper_funcs = {
> +	.enable	= zx_vga_encoder_enable,
> +	.disable = zx_vga_encoder_disable,
> +};
> +
> +static const struct drm_encoder_funcs zx_vga_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static int zx_vga_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct zx_vga *vga = to_zx_vga(connector);
> +	struct edid *edid;
> +	int ret;
> +
> +	/*
> +	 * Clear both detection bits to switch I2C bus from device
> +	 * detecting to EDID reading.
> +	 */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0);
> +
> +	edid = drm_get_edid(connector, &vga->ddc->adap);
> +	if (!edid)
> +		return 0;
> +
> +	/*
> +	 * As edid reading succeeds, device must be connected, so we set
> +	 * up detection bit for unplug interrupt here.
> +	 */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE);
> +
> +	drm_mode_connector_update_edid_property(connector, edid);
> +	ret = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
> +
> +	return ret;
> +}
> +
> +static enum drm_mode_status
> +zx_vga_connector_mode_valid(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	return MODE_OK;
> +}
> +
> +static struct drm_connector_helper_funcs zx_vga_connector_helper_funcs = {
> +	.get_modes = zx_vga_connector_get_modes,
> +	.mode_valid = zx_vga_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +zx_vga_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct zx_vga *vga = to_zx_vga(connector);
> +
> +	return vga->connected ? connector_status_connected :
> +				connector_status_disconnected;
> +}
> +
> +static const struct drm_connector_funcs zx_vga_connector_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = zx_vga_connector_detect,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +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);
> +
> +	return 0;
> +}
> +
> +static int zx_vga_pwrctrl_init(struct zx_vga *vga)
> +{
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +	struct device *dev = vga->dev;
> +	struct of_phandle_args out_args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_fixed_args(dev->of_node,
> +				"zte,vga-power-control", 2, 0, &out_args);
> +	if (ret)
> +		return ret;
> +
> +	regmap = syscon_node_to_regmap(out_args.np);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		goto out;
> +	}
> +
> +	pwrctrl->regmap = regmap;
> +	pwrctrl->reg = out_args.args[0];
> +	pwrctrl->mask = out_args.args[1];
> +
> +out:
> +	of_node_put(out_args.np);
> +	return ret;
> +}
> +
> +static int zx_vga_i2c_read(struct zx_vga *vga, struct i2c_msg *msg)
> +{
> +	int len = msg->len;
> +	u8 *buf = msg->buf;
> +	u32 offset = 0;
> +	int i;
> +
> +	reinit_completion(&vga->complete);
> +
> +	/* Select combo write */
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_COMBO, VGA_CMD_COMBO);
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_RW, 0);
> +
> +	while (len > 0) {
> +		u32 cnt;
> +
> +		/* Clear RX FIFO */
> +		zx_writel_mask(vga->mmio + VGA_RXF_CTRL, VGA_RX_FIFO_CLEAR,
> +			       VGA_RX_FIFO_CLEAR);
> +
> +		/* Data offset to read from */
> +		zx_writel(vga->mmio + VGA_SUB_ADDR, offset);
> +
> +		/* Kick off the transfer */
> +		zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_TRANS,
> +			       VGA_CMD_TRANS);
> +
> +		if (!wait_for_completion_timeout(&vga->complete,
> +						 msecs_to_jiffies(1000))) {
> +			DRM_DEV_ERROR(vga->dev, "transfer timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		cnt = zx_readl(vga->mmio + VGA_RXF_STATUS);
> +		cnt = (cnt & VGA_RXF_COUNT_MASK) >> VGA_RXF_COUNT_SHIFT;
> +		/* FIFO status may report more data than we need to read */
> +		cnt = min_t(u32, len, cnt);
> +
> +		for (i = 0; i < cnt; i++)
> +			*buf++ = zx_readl(vga->mmio + VGA_DATA);
> +
> +		len -= cnt;
> +		offset += cnt;
> +	}
> +
> +	return 0;
> +}
> +
> +static int zx_vga_i2c_write(struct zx_vga *vga, struct i2c_msg *msg)
> +{
> +	/*
> +	 * The DDC I2C adapter is only for reading EDID data, so we assume
> +	 * that the write to this adapter must be the EDID data offset.
> +	 */
> +	if ((msg->len != 1) || ((msg->addr != DDC_ADDR)))
> +		return -EINVAL;
> +
> +	/* Hardware will take care of the slave address shifting */
> +	zx_writel(vga->mmio + VGA_DEVICE_ADDR, msg->addr);
> +
> +	return 0;
> +}
> +
> +static int zx_vga_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct zx_vga *vga = i2c_get_adapdata(adap);
> +	struct zx_vga_i2c *ddc = vga->ddc;
> +	int ret = 0;
> +	int i;
> +
> +	mutex_lock(&ddc->lock);
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = zx_vga_i2c_read(vga, &msgs[i]);
> +		else
> +			ret = zx_vga_i2c_write(vga, &msgs[i]);
> +
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (!ret)
> +		ret = num;
> +
> +	mutex_unlock(&ddc->lock);
> +
> +	return ret;
> +}
> +
> +static u32 zx_vga_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm zx_vga_algorithm = {
> +	.master_xfer	= zx_vga_i2c_xfer,
> +	.functionality	= zx_vga_i2c_func,
> +};
> +
> +static int zx_vga_ddc_register(struct zx_vga *vga)
> +{
> +	struct device *dev = vga->dev;
> +	struct i2c_adapter *adap;
> +	struct zx_vga_i2c *ddc;
> +	int ret;
> +
> +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> +	if (!ddc)
> +		return -ENOMEM;
> +
> +	vga->ddc = ddc;
> +	mutex_init(&ddc->lock);
> +
> +	adap = &ddc->adap;
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_DDC;
> +	adap->dev.parent = dev;
> +	adap->algo = &zx_vga_algorithm;
> +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_adapdata(adap, vga);

Since this really isn't a full-featured i2c adapter but a specialized
"give me the edid" thing I think you should look into drm_do_get_edid and
_not_ expose the i2c thing. That should simplify your code a lot, and
avoid any kind of synchronization issues between userspace i2c access and
your driver's access (which you atm don't handle at all).

With that fixed, on the entire series.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
> +	return 0;
> +}
> +
> +static irqreturn_t zx_vga_irq_thread(int irq, void *dev_id)
> +{
> +	struct zx_vga *vga = dev_id;
> +
> +	drm_helper_hpd_irq_event(vga->connector.dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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.
> +		 */
> +		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);
> +		vga->connected = false;
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	if (status & VGA_TRANS_DONE) {
> +		complete(&vga->complete);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static void zx_vga_hw_init(struct zx_vga *vga)
> +{
> +	unsigned long ref = clk_get_rate(vga->i2c_wclk);
> +	int div;
> +
> +	/*
> +	 * Set up I2C fast speed divider per formula below to get 400kHz.
> +	 *   scl = ref / ((div + 1) * 4)
> +	 */
> +	div = DIV_ROUND_UP(ref / 1000, 400 * 4) - 1;
> +	zx_writel(vga->mmio + VGA_CLK_DIV_FS, div);
> +
> +	/* Set up device detection */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_PARA, 0x80);
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_NO_DEVICE);
> +
> +	/*
> +	 * We need to poke monitor via DDC bus to get connection irq
> +	 * start working.
> +	 */
> +	zx_writel(vga->mmio + VGA_DEVICE_ADDR, DDC_ADDR);
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_TRANS, VGA_CMD_TRANS);
> +}
> +
> +static int zx_vga_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct resource *res;
> +	struct zx_vga *vga;
> +	int irq;
> +	int ret;
> +
> +	vga = devm_kzalloc(dev, sizeof(*vga), GFP_KERNEL);
> +	if (!vga)
> +		return -ENOMEM;
> +
> +	vga->dev = dev;
> +	dev_set_drvdata(dev, vga);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vga->mmio = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(vga->mmio))
> +		return PTR_ERR(vga->mmio);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	vga->i2c_wclk = devm_clk_get(dev, "i2c_wclk");
> +	if (IS_ERR(vga->i2c_wclk)) {
> +		ret = PTR_ERR(vga->i2c_wclk);
> +		DRM_DEV_ERROR(dev, "failed to get i2c_wclk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_pwrctrl_init(vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to init power control: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_ddc_register(vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to register ddc: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_register(drm, vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to register vga: %d\n", ret);
> +		return ret;
> +	}
> +
> +	init_completion(&vga->complete);
> +
> +	ret = devm_request_threaded_irq(dev, irq, zx_vga_irq_handler,
> +					zx_vga_irq_thread, IRQF_SHARED,
> +					dev_name(dev), vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to request threaded irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(vga->i2c_wclk);
> +	if (ret)
> +		return ret;
> +
> +	zx_vga_hw_init(vga);
> +
> +	return 0;
> +}
> +
> +static void zx_vga_unbind(struct device *dev, struct device *master,
> +			  void *data)
> +{
> +	struct zx_vga *vga = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(vga->i2c_wclk);
> +}
> +
> +static const struct component_ops zx_vga_component_ops = {
> +	.bind = zx_vga_bind,
> +	.unbind = zx_vga_unbind,
> +};
> +
> +static int zx_vga_probe(struct platform_device *pdev)
> +{
> +	return component_add(&pdev->dev, &zx_vga_component_ops);
> +}
> +
> +static int zx_vga_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &zx_vga_component_ops);
> +	return 0;
> +}
> +
> +static const struct of_device_id zx_vga_of_match[] = {
> +	{ .compatible = "zte,zx296718-vga", },
> +	{ /* end */ },
> +};
> +MODULE_DEVICE_TABLE(of, zx_vga_of_match);
> +
> +struct platform_driver zx_vga_driver = {
> +	.probe = zx_vga_probe,
> +	.remove = zx_vga_remove,
> +	.driver	= {
> +		.name = "zx-vga",
> +		.of_match_table	= zx_vga_of_match,
> +	},
> +};
> diff --git a/drivers/gpu/drm/zte/zx_vga_regs.h b/drivers/gpu/drm/zte/zx_vga_regs.h
> new file mode 100644
> index 000000000000..feaa345fe6a6
> --- /dev/null
> +++ b/drivers/gpu/drm/zte/zx_vga_regs.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) 2017 Sanechips Technology Co., Ltd.
> + * Copyright 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ZX_VGA_REGS_H__
> +#define __ZX_VGA_REGS_H__
> +
> +#define VGA_CMD_CFG			0x04
> +#define VGA_CMD_TRANS			BIT(6)
> +#define VGA_CMD_COMBO			BIT(5)
> +#define VGA_CMD_RW			BIT(4)
> +#define VGA_SUB_ADDR			0x0c
> +#define VGA_DEVICE_ADDR			0x10
> +#define VGA_CLK_DIV_FS			0x14
> +#define VGA_RXF_CTRL			0x20
> +#define VGA_RX_FIFO_CLEAR		BIT(7)
> +#define VGA_DATA			0x24
> +#define VGA_I2C_STATUS			0x28
> +#define VGA_DEVICE_DISCONNECTED		BIT(7)
> +#define VGA_DEVICE_CONNECTED		BIT(6)
> +#define VGA_CLEAR_IRQ			BIT(4)
> +#define VGA_TRANS_DONE			BIT(0)
> +#define VGA_RXF_STATUS			0x30
> +#define VGA_RXF_COUNT_SHIFT		2
> +#define VGA_RXF_COUNT_MASK		GENMASK(7, 2)
> +#define VGA_AUTO_DETECT_PARA		0x34
> +#define VGA_AUTO_DETECT_SEL		0x38
> +#define VGA_DETECT_SEL_HAS_DEVICE	BIT(1)
> +#define VGA_DETECT_SEL_NO_DEVICE	BIT(0)
> +
> +#endif /* __ZX_VGA_REGS_H__ */
> diff --git a/drivers/gpu/drm/zte/zx_vou.c b/drivers/gpu/drm/zte/zx_vou.c
> index 2a2d90bd9425..cc5bdfc53e8e 100644
> --- a/drivers/gpu/drm/zte/zx_vou.c
> +++ b/drivers/gpu/drm/zte/zx_vou.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drmP.h>
>  
> +#include "zx_common_regs.h"
>  #include "zx_drm_drv.h"
>  #include "zx_plane.h"
>  #include "zx_vou.h"
> @@ -122,6 +123,8 @@ struct zx_crtc {
>  	struct drm_plane *primary;
>  	struct zx_vou_hw *vou;
>  	void __iomem *chnreg;
> +	void __iomem *chncsc;
> +	void __iomem *dither;
>  	const struct zx_crtc_regs *regs;
>  	const struct zx_crtc_bits *bits;
>  	enum vou_chn_type chn_type;
> @@ -204,6 +207,11 @@ struct vou_inf {
>  		.clocks_en_bits = BIT(15),
>  		.clocks_sel_bits = BIT(11) | BIT(0),
>  	},
> +	[VOU_VGA] = {
> +		.data_sel = VOU_RGB_888,
> +		.clocks_en_bits = BIT(1),
> +		.clocks_sel_bits = BIT(10),
> +	},
>  };
>  
>  static inline struct zx_vou_hw *crtc_to_vou(struct drm_crtc *crtc)
> @@ -227,9 +235,26 @@ void vou_inf_enable(enum vou_inf_id id, struct drm_crtc *crtc)
>  	struct zx_crtc *zcrtc = to_zx_crtc(crtc);
>  	struct zx_vou_hw *vou = zcrtc->vou;
>  	struct vou_inf *inf = &vou_infs[id];
> +	void __iomem *dither = zcrtc->dither;
> +	void __iomem *csc = zcrtc->chncsc;
>  	bool is_main = zcrtc->chn_type == VOU_CHN_MAIN;
>  	u32 data_sel_shift = id << 1;
>  
> +	if (inf->data_sel != VOU_YUV444) {
> +		/* Enable channel CSC for RGB output */
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_COV_MODE_MASK,
> +			       CSC_BT709_IMAGE_YCBCR2RGB << CSC_COV_MODE_SHIFT);
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_WORK_ENABLE,
> +			       CSC_WORK_ENABLE);
> +
> +		/* Bypass Dither block for RGB output */
> +		zx_writel_mask(dither + OSD_DITHER_CTRL0, DITHER_BYSPASS,
> +			       DITHER_BYSPASS);
> +	} else {
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_WORK_ENABLE, 0);
> +		zx_writel_mask(dither + OSD_DITHER_CTRL0, DITHER_BYSPASS, 0);
> +	}
> +
>  	/* Select data format */
>  	zx_writel_mask(vou->vouctl + VOU_INF_DATA_SEL, 0x3 << data_sel_shift,
>  		       inf->data_sel << data_sel_shift);
> @@ -502,20 +527,24 @@ static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
>  
>  	if (chn_type == VOU_CHN_MAIN) {
>  		zplane->layer = vou->osd + MAIN_GL_OFFSET;
> -		zplane->csc = vou->osd + MAIN_CSC_OFFSET;
> +		zplane->csc = vou->osd + MAIN_GL_CSC_OFFSET;
>  		zplane->hbsc = vou->osd + MAIN_HBSC_OFFSET;
>  		zplane->rsz = vou->otfppu + MAIN_RSZ_OFFSET;
>  		zplane->bits = &zx_gl_bits[0];
>  		zcrtc->chnreg = vou->osd + OSD_MAIN_CHN;
> +		zcrtc->chncsc = vou->osd + MAIN_CHN_CSC_OFFSET;
> +		zcrtc->dither = vou->osd + MAIN_DITHER_OFFSET;
>  		zcrtc->regs = &main_crtc_regs;
>  		zcrtc->bits = &main_crtc_bits;
>  	} else {
>  		zplane->layer = vou->osd + AUX_GL_OFFSET;
> -		zplane->csc = vou->osd + AUX_CSC_OFFSET;
> +		zplane->csc = vou->osd + AUX_GL_CSC_OFFSET;
>  		zplane->hbsc = vou->osd + AUX_HBSC_OFFSET;
>  		zplane->rsz = vou->otfppu + AUX_RSZ_OFFSET;
>  		zplane->bits = &zx_gl_bits[1];
>  		zcrtc->chnreg = vou->osd + OSD_AUX_CHN;
> +		zcrtc->chncsc = vou->osd + AUX_CHN_CSC_OFFSET;
> +		zcrtc->dither = vou->osd + AUX_DITHER_OFFSET;
>  		zcrtc->regs = &aux_crtc_regs;
>  		zcrtc->bits = &aux_crtc_bits;
>  	}
> diff --git a/drivers/gpu/drm/zte/zx_vou_regs.h b/drivers/gpu/drm/zte/zx_vou_regs.h
> index c066ef123434..5a218351b497 100644
> --- a/drivers/gpu/drm/zte/zx_vou_regs.h
> +++ b/drivers/gpu/drm/zte/zx_vou_regs.h
> @@ -13,13 +13,17 @@
>  
>  /* Sub-module offset */
>  #define MAIN_GL_OFFSET			0x130
> -#define MAIN_CSC_OFFSET			0x580
> +#define MAIN_GL_CSC_OFFSET		0x580
> +#define MAIN_CHN_CSC_OFFSET		0x6c0
>  #define MAIN_HBSC_OFFSET		0x820
> +#define MAIN_DITHER_OFFSET		0x960
>  #define MAIN_RSZ_OFFSET			0x600 /* OTFPPU sub-module */
>  
>  #define AUX_GL_OFFSET			0x200
> -#define AUX_CSC_OFFSET			0x5d0
> +#define AUX_GL_CSC_OFFSET		0x5d0
> +#define AUX_CHN_CSC_OFFSET		0x710
>  #define AUX_HBSC_OFFSET			0x860
> +#define AUX_DITHER_OFFSET		0x970
>  #define AUX_RSZ_OFFSET			0x800
>  
>  #define OSD_VL0_OFFSET			0x040
> @@ -78,6 +82,10 @@
>  #define CHN_INTERLACE_BUF_CTRL		0x24
>  #define CHN_INTERLACE_EN		BIT(2)
>  
> +/* Dither registers */
> +#define OSD_DITHER_CTRL0		0x00
> +#define DITHER_BYSPASS			BIT(31)
> +
>  /* TIMING_CTRL registers */
>  #define TIMING_TC_ENABLE		0x04
>  #define AUX_TC_EN			BIT(1)
> -- 
> 1.9.1
>

Shawn Guo April 4, 2017, 1:47 p.m. UTC | #2

On Mon, Apr 03, 2017 at 11:23:51AM +0200, Daniel Vetter wrote:
> > +static int zx_vga_ddc_register(struct zx_vga *vga)
> > +{
> > +	struct device *dev = vga->dev;
> > +	struct i2c_adapter *adap;
> > +	struct zx_vga_i2c *ddc;
> > +	int ret;
> > +
> > +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> > +	if (!ddc)
> > +		return -ENOMEM;
> > +
> > +	vga->ddc = ddc;
> > +	mutex_init(&ddc->lock);
> > +
> > +	adap = &ddc->adap;
> > +	adap->owner = THIS_MODULE;
> > +	adap->class = I2C_CLASS_DDC;
> > +	adap->dev.parent = dev;
> > +	adap->algo = &zx_vga_algorithm;
> > +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> > +
> > +	ret = i2c_add_adapter(adap);
> > +	if (ret) {
> > +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	i2c_set_adapdata(adap, vga);
> 
> Since this really isn't a full-featured i2c adapter but a specialized
> "give me the edid" thing I think you should look into drm_do_get_edid and
> _not_ expose the i2c thing. That should simplify your code a lot, and
> avoid any kind of synchronization issues between userspace i2c access and
> your driver's access (which you atm don't handle at all).

I'm a bit confused here.  Could you give me some more details on how to
know it's a full-featured I2C bus or specialized "give me the edid" one?
My understanding is that the ZTE hardware for reading HDMI and VGA EDID
are different but pretty similar.  But in HDMI driver review [1], you
gave the opposite comment, asking to change to I2C adapter from
drm_do_get_edid().  I'm wondering how you see that the hardware in HDMI
is a full-featured I2C bus, while the one in VGA is a specialized bus?

Shawn

[1] https://patchwork.kernel.org/patch/9349195/

Sean Paul April 4, 2017, 5:32 p.m. UTC | #3

On Tue, Apr 04, 2017 at 09:47:38PM +0800, Shawn Guo wrote:
> On Mon, Apr 03, 2017 at 11:23:51AM +0200, Daniel Vetter wrote:
> > > +static int zx_vga_ddc_register(struct zx_vga *vga)
> > > +{
> > > +	struct device *dev = vga->dev;
> > > +	struct i2c_adapter *adap;
> > > +	struct zx_vga_i2c *ddc;
> > > +	int ret;
> > > +
> > > +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> > > +	if (!ddc)
> > > +		return -ENOMEM;
> > > +
> > > +	vga->ddc = ddc;
> > > +	mutex_init(&ddc->lock);
> > > +
> > > +	adap = &ddc->adap;
> > > +	adap->owner = THIS_MODULE;
> > > +	adap->class = I2C_CLASS_DDC;
> > > +	adap->dev.parent = dev;
> > > +	adap->algo = &zx_vga_algorithm;
> > > +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> > > +
> > > +	ret = i2c_add_adapter(adap);
> > > +	if (ret) {
> > > +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	i2c_set_adapdata(adap, vga);
> > 
> > Since this really isn't a full-featured i2c adapter but a specialized
> > "give me the edid" thing I think you should look into drm_do_get_edid and
> > _not_ expose the i2c thing. That should simplify your code a lot, and
> > avoid any kind of synchronization issues between userspace i2c access and
> > your driver's access (which you atm don't handle at all).
> 
> I'm a bit confused here.  Could you give me some more details on how to
> know it's a full-featured I2C bus or specialized "give me the edid" one?
> My understanding is that the ZTE hardware for reading HDMI and VGA EDID
> are different but pretty similar.  But in HDMI driver review [1], you
> gave the opposite comment, asking to change to I2C adapter from
> drm_do_get_edid().  I'm wondering how you see that the hardware in HDMI
> is a full-featured I2C bus, while the one in VGA is a specialized bus?

That was my suggestion :-)

At the time, it seemed like the hardware implemented a fully fledged i2c bus, so
I suggested you made it generic. In retrospect, since it can only read from one
address, the original implementation was fine.

Given the HDMI adapter is implemented the same way, I'm inclined to favor this
version, but Daniel might disagree with me.

Sean

> 
> Shawn
> 
> [1] https://patchwork.kernel.org/patch/9349195/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Sean Paul April 4, 2017, 5:46 p.m. UTC | #4

On Tue, Mar 21, 2017 at 08:15:56PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> It adds VGA driver support, which needs to configure corresponding VOU
> interface in RGB_888 format, and thus the following changes are needed
> on zx_vou.
> 
> - Rename the CSC block of Graphic Layer a bit to make it more specific,
>   and add CSC of Channel to support RGB output.
> - Bypass Dither block for RGB output.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpu/drm/zte/Makefile      |   1 +
>  drivers/gpu/drm/zte/zx_drm_drv.c  |   1 +
>  drivers/gpu/drm/zte/zx_drm_drv.h  |   1 +
>  drivers/gpu/drm/zte/zx_vga.c      | 500 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/zte/zx_vga_regs.h |  36 +++
>  drivers/gpu/drm/zte/zx_vou.c      |  33 ++-
>  drivers/gpu/drm/zte/zx_vou_regs.h |  12 +-
>  7 files changed, 580 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/zte/zx_vga.c
>  create mode 100644 drivers/gpu/drm/zte/zx_vga_regs.h
> 
> diff --git a/drivers/gpu/drm/zte/Makefile b/drivers/gpu/drm/zte/Makefile
> index 01352b56c418..9df7766a7f9d 100644
> --- a/drivers/gpu/drm/zte/Makefile
> +++ b/drivers/gpu/drm/zte/Makefile
> @@ -3,6 +3,7 @@ zxdrm-y := \
>  	zx_hdmi.o \
>  	zx_plane.o \
>  	zx_tvenc.o \
> +	zx_vga.o \
>  	zx_vou.o
>  
>  obj-$(CONFIG_DRM_ZTE) += zxdrm.o
> diff --git a/drivers/gpu/drm/zte/zx_drm_drv.c b/drivers/gpu/drm/zte/zx_drm_drv.c
> index 5c6944a1e72c..8a6892eeb44f 100644
> --- a/drivers/gpu/drm/zte/zx_drm_drv.c
> +++ b/drivers/gpu/drm/zte/zx_drm_drv.c
> @@ -248,6 +248,7 @@ static int zx_drm_remove(struct platform_device *pdev)
>  	&zx_crtc_driver,
>  	&zx_hdmi_driver,
>  	&zx_tvenc_driver,
> +	&zx_vga_driver,
>  	&zx_drm_platform_driver,
>  };
>  
> diff --git a/drivers/gpu/drm/zte/zx_drm_drv.h b/drivers/gpu/drm/zte/zx_drm_drv.h
> index 5ca035b079c7..2a8cdc5f8be4 100644
> --- a/drivers/gpu/drm/zte/zx_drm_drv.h
> +++ b/drivers/gpu/drm/zte/zx_drm_drv.h
> @@ -14,6 +14,7 @@
>  extern struct platform_driver zx_crtc_driver;
>  extern struct platform_driver zx_hdmi_driver;
>  extern struct platform_driver zx_tvenc_driver;
> +extern struct platform_driver zx_vga_driver;
>  
>  static inline u32 zx_readl(void __iomem *reg)
>  {
> diff --git a/drivers/gpu/drm/zte/zx_vga.c b/drivers/gpu/drm/zte/zx_vga.c
> new file mode 100644
> index 000000000000..35eaf98458af
> --- /dev/null
> +++ b/drivers/gpu/drm/zte/zx_vga.c
> @@ -0,0 +1,500 @@
> +/*
> + * Copyright (C) 2017 Sanechips Technology Co., Ltd.
> + * Copyright 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drmP.h>
> +
> +#include "zx_drm_drv.h"
> +#include "zx_vga_regs.h"
> +#include "zx_vou.h"
> +
> +struct zx_vga_pwrctrl {
> +	struct regmap *regmap;
> +	u32 reg;
> +	u32 mask;
> +};
> +
> +struct zx_vga_i2c {
> +	struct i2c_adapter adap;
> +	struct mutex lock;
> +};
> +
> +struct zx_vga {
> +	struct drm_connector connector;
> +	struct drm_encoder encoder;
> +	struct zx_vga_i2c *ddc;
> +	struct device *dev;
> +	void __iomem *mmio;
> +	struct clk *i2c_wclk;
> +	struct zx_vga_pwrctrl pwrctrl;
> +	struct completion complete;
> +	bool connected;
> +};
> +
> +#define to_zx_vga(x) container_of(x, struct zx_vga, x)
> +
> +static void zx_vga_encoder_enable(struct drm_encoder *encoder)
> +{
> +	struct zx_vga *vga = to_zx_vga(encoder);
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +
> +	/* Set bit to power up VGA DACs */
> +	regmap_update_bits(pwrctrl->regmap, pwrctrl->reg, pwrctrl->mask,
> +			   pwrctrl->mask);
> +
> +	vou_inf_enable(VOU_VGA, encoder->crtc);
> +}
> +
> +static void zx_vga_encoder_disable(struct drm_encoder *encoder)
> +{
> +	struct zx_vga *vga = to_zx_vga(encoder);
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +
> +	vou_inf_disable(VOU_VGA, encoder->crtc);
> +
> +	/* Clear bit to power down VGA DACs */
> +	regmap_update_bits(pwrctrl->regmap, pwrctrl->reg, pwrctrl->mask, 0);
> +}
> +
> +static const struct drm_encoder_helper_funcs zx_vga_encoder_helper_funcs = {
> +	.enable	= zx_vga_encoder_enable,
> +	.disable = zx_vga_encoder_disable,
> +};
> +
> +static const struct drm_encoder_funcs zx_vga_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static int zx_vga_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct zx_vga *vga = to_zx_vga(connector);
> +	struct edid *edid;
> +	int ret;
> +
> +	/*
> +	 * Clear both detection bits to switch I2C bus from device
> +	 * detecting to EDID reading.
> +	 */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0);
> +
> +	edid = drm_get_edid(connector, &vga->ddc->adap);
> +	if (!edid)
> +		return 0;
> +
> +	/*
> +	 * As edid reading succeeds, device must be connected, so we set
> +	 * up detection bit for unplug interrupt here.
> +	 */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE);
> +
> +	drm_mode_connector_update_edid_property(connector, edid);
> +	ret = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
> +
> +	return ret;
> +}
> +
> +static enum drm_mode_status
> +zx_vga_connector_mode_valid(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	return MODE_OK;
> +}
> +
> +static struct drm_connector_helper_funcs zx_vga_connector_helper_funcs = {
> +	.get_modes = zx_vga_connector_get_modes,
> +	.mode_valid = zx_vga_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +zx_vga_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct zx_vga *vga = to_zx_vga(connector);
> +
> +	return vga->connected ? connector_status_connected :
> +				connector_status_disconnected;
> +}
> +
> +static const struct drm_connector_funcs zx_vga_connector_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = zx_vga_connector_detect,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +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.

> +
> +	return 0;
> +}
> +
> +static int zx_vga_pwrctrl_init(struct zx_vga *vga)
> +{
> +	struct zx_vga_pwrctrl *pwrctrl = &vga->pwrctrl;
> +	struct device *dev = vga->dev;
> +	struct of_phandle_args out_args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_fixed_args(dev->of_node,
> +				"zte,vga-power-control", 2, 0, &out_args);
> +	if (ret)
> +		return ret;
> +
> +	regmap = syscon_node_to_regmap(out_args.np);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		goto out;
> +	}
> +
> +	pwrctrl->regmap = regmap;
> +	pwrctrl->reg = out_args.args[0];
> +	pwrctrl->mask = out_args.args[1];
> +
> +out:
> +	of_node_put(out_args.np);
> +	return ret;
> +}
> +
> +static int zx_vga_i2c_read(struct zx_vga *vga, struct i2c_msg *msg)
> +{
> +	int len = msg->len;
> +	u8 *buf = msg->buf;
> +	u32 offset = 0;
> +	int i;
> +
> +	reinit_completion(&vga->complete);
> +
> +	/* Select combo write */
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_COMBO, VGA_CMD_COMBO);
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_RW, 0);
> +
> +	while (len > 0) {
> +		u32 cnt;
> +
> +		/* Clear RX FIFO */
> +		zx_writel_mask(vga->mmio + VGA_RXF_CTRL, VGA_RX_FIFO_CLEAR,
> +			       VGA_RX_FIFO_CLEAR);
> +
> +		/* Data offset to read from */
> +		zx_writel(vga->mmio + VGA_SUB_ADDR, offset);
> +
> +		/* Kick off the transfer */
> +		zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_TRANS,
> +			       VGA_CMD_TRANS);
> +
> +		if (!wait_for_completion_timeout(&vga->complete,
> +						 msecs_to_jiffies(1000))) {
> +			DRM_DEV_ERROR(vga->dev, "transfer timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		cnt = zx_readl(vga->mmio + VGA_RXF_STATUS);
> +		cnt = (cnt & VGA_RXF_COUNT_MASK) >> VGA_RXF_COUNT_SHIFT;
> +		/* FIFO status may report more data than we need to read */
> +		cnt = min_t(u32, len, cnt);
> +
> +		for (i = 0; i < cnt; i++)
> +			*buf++ = zx_readl(vga->mmio + VGA_DATA);
> +
> +		len -= cnt;
> +		offset += cnt;
> +	}
> +
> +	return 0;
> +}
> +
> +static int zx_vga_i2c_write(struct zx_vga *vga, struct i2c_msg *msg)
> +{
> +	/*
> +	 * The DDC I2C adapter is only for reading EDID data, so we assume
> +	 * that the write to this adapter must be the EDID data offset.
> +	 */
> +	if ((msg->len != 1) || ((msg->addr != DDC_ADDR)))
> +		return -EINVAL;
> +
> +	/* Hardware will take care of the slave address shifting */
> +	zx_writel(vga->mmio + VGA_DEVICE_ADDR, msg->addr);
> +
> +	return 0;
> +}
> +
> +static int zx_vga_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct zx_vga *vga = i2c_get_adapdata(adap);
> +	struct zx_vga_i2c *ddc = vga->ddc;
> +	int ret = 0;
> +	int i;
> +
> +	mutex_lock(&ddc->lock);
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = zx_vga_i2c_read(vga, &msgs[i]);
> +		else
> +			ret = zx_vga_i2c_write(vga, &msgs[i]);
> +
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (!ret)
> +		ret = num;
> +
> +	mutex_unlock(&ddc->lock);
> +
> +	return ret;
> +}
> +
> +static u32 zx_vga_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm zx_vga_algorithm = {
> +	.master_xfer	= zx_vga_i2c_xfer,
> +	.functionality	= zx_vga_i2c_func,
> +};
> +
> +static int zx_vga_ddc_register(struct zx_vga *vga)
> +{
> +	struct device *dev = vga->dev;
> +	struct i2c_adapter *adap;
> +	struct zx_vga_i2c *ddc;
> +	int ret;
> +
> +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> +	if (!ddc)
> +		return -ENOMEM;
> +
> +	vga->ddc = ddc;
> +	mutex_init(&ddc->lock);
> +
> +	adap = &ddc->adap;
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_DDC;
> +	adap->dev.parent = dev;
> +	adap->algo = &zx_vga_algorithm;
> +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_adapdata(adap, vga);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t zx_vga_irq_thread(int irq, void *dev_id)
> +{
> +	struct zx_vga *vga = dev_id;
> +
> +	drm_helper_hpd_irq_event(vga->connector.dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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?

> +		 */
> +		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.

> +		vga->connected = false;
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	if (status & VGA_TRANS_DONE) {
> +		complete(&vga->complete);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static void zx_vga_hw_init(struct zx_vga *vga)
> +{
> +	unsigned long ref = clk_get_rate(vga->i2c_wclk);
> +	int div;
> +
> +	/*
> +	 * Set up I2C fast speed divider per formula below to get 400kHz.
> +	 *   scl = ref / ((div + 1) * 4)
> +	 */
> +	div = DIV_ROUND_UP(ref / 1000, 400 * 4) - 1;
> +	zx_writel(vga->mmio + VGA_CLK_DIV_FS, div);
> +
> +	/* Set up device detection */
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_PARA, 0x80);
> +	zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_NO_DEVICE);
> +
> +	/*
> +	 * We need to poke monitor via DDC bus to get connection irq
> +	 * start working.
> +	 */
> +	zx_writel(vga->mmio + VGA_DEVICE_ADDR, DDC_ADDR);
> +	zx_writel_mask(vga->mmio + VGA_CMD_CFG, VGA_CMD_TRANS, VGA_CMD_TRANS);
> +}
> +
> +static int zx_vga_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct resource *res;
> +	struct zx_vga *vga;
> +	int irq;
> +	int ret;
> +
> +	vga = devm_kzalloc(dev, sizeof(*vga), GFP_KERNEL);
> +	if (!vga)
> +		return -ENOMEM;
> +
> +	vga->dev = dev;
> +	dev_set_drvdata(dev, vga);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vga->mmio = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(vga->mmio))
> +		return PTR_ERR(vga->mmio);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	vga->i2c_wclk = devm_clk_get(dev, "i2c_wclk");
> +	if (IS_ERR(vga->i2c_wclk)) {
> +		ret = PTR_ERR(vga->i2c_wclk);
> +		DRM_DEV_ERROR(dev, "failed to get i2c_wclk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_pwrctrl_init(vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to init power control: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_ddc_register(vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to register ddc: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = zx_vga_register(drm, vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to register vga: %d\n", ret);
> +		return ret;
> +	}
> +
> +	init_completion(&vga->complete);
> +
> +	ret = devm_request_threaded_irq(dev, irq, zx_vga_irq_handler,
> +					zx_vga_irq_thread, IRQF_SHARED,
> +					dev_name(dev), vga);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to request threaded irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(vga->i2c_wclk);
> +	if (ret)
> +		return ret;
> +
> +	zx_vga_hw_init(vga);
> +
> +	return 0;
> +}
> +
> +static void zx_vga_unbind(struct device *dev, struct device *master,
> +			  void *data)
> +{
> +	struct zx_vga *vga = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(vga->i2c_wclk);
> +}
> +
> +static const struct component_ops zx_vga_component_ops = {
> +	.bind = zx_vga_bind,
> +	.unbind = zx_vga_unbind,
> +};
> +
> +static int zx_vga_probe(struct platform_device *pdev)
> +{
> +	return component_add(&pdev->dev, &zx_vga_component_ops);
> +}
> +
> +static int zx_vga_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &zx_vga_component_ops);
> +	return 0;
> +}
> +
> +static const struct of_device_id zx_vga_of_match[] = {
> +	{ .compatible = "zte,zx296718-vga", },
> +	{ /* end */ },
> +};
> +MODULE_DEVICE_TABLE(of, zx_vga_of_match);
> +
> +struct platform_driver zx_vga_driver = {
> +	.probe = zx_vga_probe,
> +	.remove = zx_vga_remove,
> +	.driver	= {
> +		.name = "zx-vga",
> +		.of_match_table	= zx_vga_of_match,
> +	},
> +};
> diff --git a/drivers/gpu/drm/zte/zx_vga_regs.h b/drivers/gpu/drm/zte/zx_vga_regs.h
> new file mode 100644
> index 000000000000..feaa345fe6a6
> --- /dev/null
> +++ b/drivers/gpu/drm/zte/zx_vga_regs.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) 2017 Sanechips Technology Co., Ltd.
> + * Copyright 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ZX_VGA_REGS_H__
> +#define __ZX_VGA_REGS_H__
> +
> +#define VGA_CMD_CFG			0x04
> +#define VGA_CMD_TRANS			BIT(6)
> +#define VGA_CMD_COMBO			BIT(5)
> +#define VGA_CMD_RW			BIT(4)
> +#define VGA_SUB_ADDR			0x0c
> +#define VGA_DEVICE_ADDR			0x10
> +#define VGA_CLK_DIV_FS			0x14
> +#define VGA_RXF_CTRL			0x20
> +#define VGA_RX_FIFO_CLEAR		BIT(7)
> +#define VGA_DATA			0x24
> +#define VGA_I2C_STATUS			0x28
> +#define VGA_DEVICE_DISCONNECTED		BIT(7)
> +#define VGA_DEVICE_CONNECTED		BIT(6)
> +#define VGA_CLEAR_IRQ			BIT(4)
> +#define VGA_TRANS_DONE			BIT(0)
> +#define VGA_RXF_STATUS			0x30
> +#define VGA_RXF_COUNT_SHIFT		2
> +#define VGA_RXF_COUNT_MASK		GENMASK(7, 2)
> +#define VGA_AUTO_DETECT_PARA		0x34
> +#define VGA_AUTO_DETECT_SEL		0x38
> +#define VGA_DETECT_SEL_HAS_DEVICE	BIT(1)
> +#define VGA_DETECT_SEL_NO_DEVICE	BIT(0)
> +
> +#endif /* __ZX_VGA_REGS_H__ */
> diff --git a/drivers/gpu/drm/zte/zx_vou.c b/drivers/gpu/drm/zte/zx_vou.c
> index 2a2d90bd9425..cc5bdfc53e8e 100644
> --- a/drivers/gpu/drm/zte/zx_vou.c
> +++ b/drivers/gpu/drm/zte/zx_vou.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drmP.h>
>  
> +#include "zx_common_regs.h"
>  #include "zx_drm_drv.h"
>  #include "zx_plane.h"
>  #include "zx_vou.h"
> @@ -122,6 +123,8 @@ struct zx_crtc {
>  	struct drm_plane *primary;
>  	struct zx_vou_hw *vou;
>  	void __iomem *chnreg;
> +	void __iomem *chncsc;
> +	void __iomem *dither;
>  	const struct zx_crtc_regs *regs;
>  	const struct zx_crtc_bits *bits;
>  	enum vou_chn_type chn_type;
> @@ -204,6 +207,11 @@ struct vou_inf {
>  		.clocks_en_bits = BIT(15),
>  		.clocks_sel_bits = BIT(11) | BIT(0),
>  	},
> +	[VOU_VGA] = {
> +		.data_sel = VOU_RGB_888,
> +		.clocks_en_bits = BIT(1),
> +		.clocks_sel_bits = BIT(10),
> +	},
>  };
>  
>  static inline struct zx_vou_hw *crtc_to_vou(struct drm_crtc *crtc)
> @@ -227,9 +235,26 @@ void vou_inf_enable(enum vou_inf_id id, struct drm_crtc *crtc)
>  	struct zx_crtc *zcrtc = to_zx_crtc(crtc);
>  	struct zx_vou_hw *vou = zcrtc->vou;
>  	struct vou_inf *inf = &vou_infs[id];
> +	void __iomem *dither = zcrtc->dither;
> +	void __iomem *csc = zcrtc->chncsc;
>  	bool is_main = zcrtc->chn_type == VOU_CHN_MAIN;
>  	u32 data_sel_shift = id << 1;
>  
> +	if (inf->data_sel != VOU_YUV444) {
> +		/* Enable channel CSC for RGB output */
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_COV_MODE_MASK,
> +			       CSC_BT709_IMAGE_YCBCR2RGB << CSC_COV_MODE_SHIFT);
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_WORK_ENABLE,
> +			       CSC_WORK_ENABLE);
> +
> +		/* Bypass Dither block for RGB output */
> +		zx_writel_mask(dither + OSD_DITHER_CTRL0, DITHER_BYSPASS,
> +			       DITHER_BYSPASS);
> +	} else {
> +		zx_writel_mask(csc + CSC_CTRL0, CSC_WORK_ENABLE, 0);
> +		zx_writel_mask(dither + OSD_DITHER_CTRL0, DITHER_BYSPASS, 0);
> +	}
> +
>  	/* Select data format */
>  	zx_writel_mask(vou->vouctl + VOU_INF_DATA_SEL, 0x3 << data_sel_shift,
>  		       inf->data_sel << data_sel_shift);
> @@ -502,20 +527,24 @@ static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
>  
>  	if (chn_type == VOU_CHN_MAIN) {
>  		zplane->layer = vou->osd + MAIN_GL_OFFSET;
> -		zplane->csc = vou->osd + MAIN_CSC_OFFSET;
> +		zplane->csc = vou->osd + MAIN_GL_CSC_OFFSET;
>  		zplane->hbsc = vou->osd + MAIN_HBSC_OFFSET;
>  		zplane->rsz = vou->otfppu + MAIN_RSZ_OFFSET;
>  		zplane->bits = &zx_gl_bits[0];
>  		zcrtc->chnreg = vou->osd + OSD_MAIN_CHN;
> +		zcrtc->chncsc = vou->osd + MAIN_CHN_CSC_OFFSET;
> +		zcrtc->dither = vou->osd + MAIN_DITHER_OFFSET;
>  		zcrtc->regs = &main_crtc_regs;
>  		zcrtc->bits = &main_crtc_bits;
>  	} else {
>  		zplane->layer = vou->osd + AUX_GL_OFFSET;
> -		zplane->csc = vou->osd + AUX_CSC_OFFSET;
> +		zplane->csc = vou->osd + AUX_GL_CSC_OFFSET;
>  		zplane->hbsc = vou->osd + AUX_HBSC_OFFSET;
>  		zplane->rsz = vou->otfppu + AUX_RSZ_OFFSET;
>  		zplane->bits = &zx_gl_bits[1];
>  		zcrtc->chnreg = vou->osd + OSD_AUX_CHN;
> +		zcrtc->chncsc = vou->osd + AUX_CHN_CSC_OFFSET;
> +		zcrtc->dither = vou->osd + AUX_DITHER_OFFSET;
>  		zcrtc->regs = &aux_crtc_regs;
>  		zcrtc->bits = &aux_crtc_bits;
>  	}
> diff --git a/drivers/gpu/drm/zte/zx_vou_regs.h b/drivers/gpu/drm/zte/zx_vou_regs.h
> index c066ef123434..5a218351b497 100644
> --- a/drivers/gpu/drm/zte/zx_vou_regs.h
> +++ b/drivers/gpu/drm/zte/zx_vou_regs.h
> @@ -13,13 +13,17 @@
>  
>  /* Sub-module offset */
>  #define MAIN_GL_OFFSET			0x130
> -#define MAIN_CSC_OFFSET			0x580
> +#define MAIN_GL_CSC_OFFSET		0x580
> +#define MAIN_CHN_CSC_OFFSET		0x6c0
>  #define MAIN_HBSC_OFFSET		0x820
> +#define MAIN_DITHER_OFFSET		0x960
>  #define MAIN_RSZ_OFFSET			0x600 /* OTFPPU sub-module */
>  
>  #define AUX_GL_OFFSET			0x200
> -#define AUX_CSC_OFFSET			0x5d0
> +#define AUX_GL_CSC_OFFSET		0x5d0
> +#define AUX_CHN_CSC_OFFSET		0x710
>  #define AUX_HBSC_OFFSET			0x860
> +#define AUX_DITHER_OFFSET		0x970
>  #define AUX_RSZ_OFFSET			0x800
>  
>  #define OSD_VL0_OFFSET			0x040
> @@ -78,6 +82,10 @@
>  #define CHN_INTERLACE_BUF_CTRL		0x24
>  #define CHN_INTERLACE_EN		BIT(2)
>  
> +/* Dither registers */
> +#define OSD_DITHER_CTRL0		0x00
> +#define DITHER_BYSPASS			BIT(31)
> +
>  /* TIMING_CTRL registers */
>  #define TIMING_TC_ENABLE		0x04
>  #define AUX_TC_EN			BIT(1)
> -- 
> 1.9.1

Shawn Guo April 5, 2017, 6:08 a.m. UTC | #5

On Tue, Apr 04, 2017 at 01:32:39PM -0400, Sean Paul wrote:
> On Tue, Apr 04, 2017 at 09:47:38PM +0800, Shawn Guo wrote:
> > On Mon, Apr 03, 2017 at 11:23:51AM +0200, Daniel Vetter wrote:
> > > > +static int zx_vga_ddc_register(struct zx_vga *vga)
> > > > +{
> > > > +	struct device *dev = vga->dev;
> > > > +	struct i2c_adapter *adap;
> > > > +	struct zx_vga_i2c *ddc;
> > > > +	int ret;
> > > > +
> > > > +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> > > > +	if (!ddc)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	vga->ddc = ddc;
> > > > +	mutex_init(&ddc->lock);
> > > > +
> > > > +	adap = &ddc->adap;
> > > > +	adap->owner = THIS_MODULE;
> > > > +	adap->class = I2C_CLASS_DDC;
> > > > +	adap->dev.parent = dev;
> > > > +	adap->algo = &zx_vga_algorithm;
> > > > +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> > > > +
> > > > +	ret = i2c_add_adapter(adap);
> > > > +	if (ret) {
> > > > +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	i2c_set_adapdata(adap, vga);
> > > 
> > > Since this really isn't a full-featured i2c adapter but a specialized
> > > "give me the edid" thing I think you should look into drm_do_get_edid and
> > > _not_ expose the i2c thing. That should simplify your code a lot, and
> > > avoid any kind of synchronization issues between userspace i2c access and
> > > your driver's access (which you atm don't handle at all).
> > 
> > I'm a bit confused here.  Could you give me some more details on how to
> > know it's a full-featured I2C bus or specialized "give me the edid" one?
> > My understanding is that the ZTE hardware for reading HDMI and VGA EDID
> > are different but pretty similar.  But in HDMI driver review [1], you
> > gave the opposite comment, asking to change to I2C adapter from
> > drm_do_get_edid().  I'm wondering how you see that the hardware in HDMI
> > is a full-featured I2C bus, while the one in VGA is a specialized bus?
> 
> That was my suggestion :-)

It was Daniel's too.  He commented as below under function
zx_hdmi_get_edid_block().

"It looks like the ZX_DDC register range implements a hw i2c engine (since
you specifiy port and offsets and everything). Please implement it as an
i2c_adapter driver and use the normal drm_get_edid function."

Shawn

> 
> At the time, it seemed like the hardware implemented a fully fledged i2c bus, so
> I suggested you made it generic. In retrospect, since it can only read from one
> address, the original implementation was fine.
> 
> Given the HDMI adapter is implemented the same way, I'm inclined to favor this
> version, but Daniel might disagree with me.

Daniel Vetter April 5, 2017, 6:25 a.m. UTC | #6

On Wed, Apr 05, 2017 at 02:08:52PM +0800, Shawn Guo wrote:
> On Tue, Apr 04, 2017 at 01:32:39PM -0400, Sean Paul wrote:
> > On Tue, Apr 04, 2017 at 09:47:38PM +0800, Shawn Guo wrote:
> > > On Mon, Apr 03, 2017 at 11:23:51AM +0200, Daniel Vetter wrote:
> > > > > +static int zx_vga_ddc_register(struct zx_vga *vga)
> > > > > +{
> > > > > +	struct device *dev = vga->dev;
> > > > > +	struct i2c_adapter *adap;
> > > > > +	struct zx_vga_i2c *ddc;
> > > > > +	int ret;
> > > > > +
> > > > > +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> > > > > +	if (!ddc)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	vga->ddc = ddc;
> > > > > +	mutex_init(&ddc->lock);
> > > > > +
> > > > > +	adap = &ddc->adap;
> > > > > +	adap->owner = THIS_MODULE;
> > > > > +	adap->class = I2C_CLASS_DDC;
> > > > > +	adap->dev.parent = dev;
> > > > > +	adap->algo = &zx_vga_algorithm;
> > > > > +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> > > > > +
> > > > > +	ret = i2c_add_adapter(adap);
> > > > > +	if (ret) {
> > > > > +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	i2c_set_adapdata(adap, vga);
> > > > 
> > > > Since this really isn't a full-featured i2c adapter but a specialized
> > > > "give me the edid" thing I think you should look into drm_do_get_edid and
> > > > _not_ expose the i2c thing. That should simplify your code a lot, and
> > > > avoid any kind of synchronization issues between userspace i2c access and
> > > > your driver's access (which you atm don't handle at all).
> > > 
> > > I'm a bit confused here.  Could you give me some more details on how to
> > > know it's a full-featured I2C bus or specialized "give me the edid" one?
> > > My understanding is that the ZTE hardware for reading HDMI and VGA EDID
> > > are different but pretty similar.  But in HDMI driver review [1], you
> > > gave the opposite comment, asking to change to I2C adapter from
> > > drm_do_get_edid().  I'm wondering how you see that the hardware in HDMI
> > > is a full-featured I2C bus, while the one in VGA is a specialized bus?
> > 
> > That was my suggestion :-)
> 
> It was Daniel's too.  He commented as below under function
> zx_hdmi_get_edid_block().
> 
> "It looks like the ZX_DDC register range implements a hw i2c engine (since
> you specifiy port and offsets and everything). Please implement it as an
> i2c_adapter driver and use the normal drm_get_edid function."

Hm indeed, I guess I'm just bad at this review stuff :-)

> > At the time, it seemed like the hardware implemented a fully fledged i2c bus, so
> > I suggested you made it generic. In retrospect, since it can only read from one
> > address, the original implementation was fine.
> > 
> > Given the HDMI adapter is implemented the same way, I'm inclined to favor this
> > version, but Daniel might disagree with me.

Yeah, we can go with this one for now, but long-term might indeed be good
to switch both of them over to drm_do_get_edid, since that should fit
better. Sorry for all the confusion.
-Daniel

Shawn Guo April 5, 2017, 2:08 p.m. UTC | #7

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;
> > +}

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

Commit Message

Comments

Patch