Message-ID: <20160610141342.GZ3363@phenom.ffwll.local>
Date:	Fri, 10 Jun 2016 16:13:42 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Peter Senna Tschudin <peter.senna@...labora.com>
Cc:	airlied@...ux.ie, akpm@...ux-foundation.org, davem@...emloft.net,
	devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	enric.balletbo@...labora.com, eballetbo@...il.com,
	galak@...eaurora.org, gregkh@...uxfoundation.org, heiko@...ech.de,
	ijc+devicetree@...lion.org.uk, jslaby@...e.cz,
	kernel@...gutronix.de, linux-arm-kernel@...ts.infradead.org,
	linux@...linux.org.uk, linux-kernel@...r.kernel.org,
	linux@...ck-us.net, mark.rutland@....com, martin.donnelly@...com,
	martyn.welch@...labora.co.uk, mchehab@....samsung.com,
	pawel.moll@....com, peter.senna@...il.com, p.zabel@...gutronix.de,
	thierry.reding@...il.com, rmk+kernel@...linux.org.uk,
	robh+dt@...nel.org, shawnguo@...nel.org, tiwai@...e.com,
	treding@...dia.com, ykk@...k-chips.com,
	Fabio Estevam <fabio.estevam@....com>
Subject: Re: [PATCH V2 4/5] drm/bridge: Add driver for GE B850v3 LVDS/DP++
 Bridge

On Thu, Jun 09, 2016 at 06:25:04PM +0200, Peter Senna Tschudin wrote:
> Add a driver that create a drm_bridge and a drm_connector for the LVDS
> to DP++ display bridge of the GE B850v3.
> 
> There are two physical bridges on the video signal pipeline: a
> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> firmware made it complicated for this binding to comprise two device
> tree nodes, as the design goal is to configure both bridges based on
> the LVDS signal, which leave the driver powerless to control the video
> processing pipeline. The two bridges behaves as a single bridge, and
> the driver is only needed for telling the host about EDID / HPD, and
> for giving the host powers to ack interrupts. The video signal pipeline
> is as follows:
> 
>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> 
> Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> Cc: Philipp Zabel <p.zabel@...gutronix.de>
> Cc: Rob Herring <robh@...nel.org>
> Cc: Fabio Estevam <fabio.estevam@....com>
> CC: David Airlie <airlied@...ux.ie>
> CC: Thierry Reding <treding@...dia.com>
> CC: Thierry Reding <thierry.reding@...il.com>
> Signed-off-by: Peter Senna Tschudin <peter.senna@...labora.com>
> ---
> Changes from V1:
>  - New commit message
>  - Removed 3 empty entry points
>  - Removed memory leak from ge_b850v3_lvds_dp_get_modes()
>  - Added a lock for mode setting
>  - Removed a few blank lines
>  - Changed the order at Makefile and Kconfig

Two comments below.

> 
>  MAINTAINERS                                |   8 +
>  drivers/gpu/drm/bridge/Kconfig             |  11 +
>  drivers/gpu/drm/bridge/Makefile            |   1 +
>  drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c | 392 +++++++++++++++++++++++++++++
>  4 files changed, 412 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ce5e91..2dd3d7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5010,6 +5010,14 @@ W:	https://linuxtv.org
>  S:	Maintained
>  F:	drivers/media/radio/radio-gemtek*
>  
> +GENERAL ELECTRIC B850V3 LVDS/DP++ BRIDGE
> +M:	Martin Donnelly <martin.donnelly@...com>
> +M:	Peter Senna Tschudin <peter.senna@...labora.com>
> +M:	Martyn Welch <martyn.welch@...labora.co.uk>
> +S:	Maintained
> +F:	drivers/gpu/drm/bridge/ge_b850v3_dp2.c
> +F:	Documentation/devicetree/bindings/ge/b850v3_dp2_bridge.txt
> +
>  GENERIC GPIO I2C DRIVER
>  M:	Haavard Skinnemoen <hskinnemoen@...il.com>
>  S:	Supported
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 8f7423f..93dae5bd 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,17 @@ config DRM_DW_HDMI_AHB_AUDIO
>  	  Designware HDMI block.  This is used in conjunction with
>  	  the i.MX6 HDMI driver.
>  
> +config DRM_GE_B850V3_LVDS_DP
> +	tristate "GE B850v3 LVDS to DP++ display bridge"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	select DRM_PANEL
> +	---help---
> +          This is a driver for the display bridge of
> +          GE B850v3 that convert dual channel LVDS
> +          to DP++. This is used with the i.MX6 imx-ldb
> +          driver.
> +
>  config DRM_NXP_PTN3460
>  	tristate "NXP PTN3460 DP/LVDS bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 96b13b3..47ea6c1 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
> +obj-$(CONFIG_DRM_GE_B850V3_LVDS_DP) += ge_b850v3_lvds_dp.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> diff --git a/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> new file mode 100644
> index 0000000..c73cd77
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> @@ -0,0 +1,392 @@
> +/*
> + * Driver for GE B850v3 DP display bridge
> +
> + * Copyright (c) 2016, Collabora Ltd.
> + * Copyright (c) 2016, General Electric Company
> +
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> +
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> +
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> + * This driver creates a drm_bridge and a drm_connector for the LVDS to DP++
> + * display bridge of the GE B850v3. There are two physical bridges on the video
> + * signal pipeline: a STDP4028(LVDS to DP) and a STDP2690(DP to DP++). However
> + * the physical bridges are automatically configured by the input video signal,
> + * and the driver has no access to the video processing pipeline. The driver is
> + * only needed to read EDID from the STDP2690 and to handle HPD events from the
> + * STDP4028. The driver communicates with both bridges over i2c. The video
> + * signal pipeline is as follows:
> + *
> + *   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> +
> + *
> + */
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include "drm_crtc.h"
> +#include "drm_crtc_helper.h"
> +#include "drm_edid.h"
> +#include "drmP.h"
> +
> +#define EDID_EXT_BLOCK_CNT 0x7E
> +
> +#define STDP4028_IRQ_OUT_CONF_REG 0x02
> +#define STDP4028_DPTX_IRQ_EN_REG 0x3C
> +#define STDP4028_DPTX_IRQ_STS_REG 0x3D
> +#define STDP4028_DPTX_STS_REG 0x3E
> +
> +#define STDP4028_DPTX_DP_IRQ_EN 0x1000
> +
> +#define STDP4028_DPTX_HOTPLUG_IRQ_EN 0x0400
> +#define STDP4028_DPTX_LINK_CH_IRQ_EN 0x2000
> +#define STDP4028_DPTX_IRQ_CONFIG \
> +		(STDP4028_DPTX_LINK_CH_IRQ_EN | STDP4028_DPTX_HOTPLUG_IRQ_EN)
> +
> +#define STDP4028_DPTX_HOTPLUG_STS 0x0200
> +#define STDP4028_DPTX_LINK_STS 0x1000
> +#define STDP4028_CON_STATE_CONNECTED \
> +		(STDP4028_DPTX_HOTPLUG_STS | STDP4028_DPTX_LINK_STS)
> +
> +#define STDP4028_DPTX_HOTPLUG_CH_STS 0x0400
> +#define STDP4028_DPTX_LINK_CH_STS 0x2000
> +#define STDP4028_DPTX_IRQ_CLEAR \
> +		(STDP4028_DPTX_LINK_CH_STS | STDP4028_DPTX_HOTPLUG_CH_STS)
> +
> +struct ge_b850v3_lvds_dp {
> +	struct drm_connector connector;
> +	struct drm_bridge bridge;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c;
> +	struct i2c_client *edid_i2c;
> +	struct edid *edid;
> +	struct mutex lock;
> +};
> +
> +static inline struct ge_b850v3_lvds_dp *
> +		bridge_to_ge_b850v3_lvds_dp(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct ge_b850v3_lvds_dp, bridge);
> +}
> +
> +static inline struct ge_b850v3_lvds_dp *
> +		connector_to_ge_b850v3_lvds_dp(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct ge_b850v3_lvds_dp, connector);
> +}
> +
> +static void ge_b850v3_lvds_dp_enable(struct drm_bridge *bridge)
> +{
> +}
> +
> +static void ge_b850v3_lvds_dp_disable(struct drm_bridge *bridge)
> +{
> +}

Please remove the above two dummy funcs, they should be unecessary. If
they're not that would be an issue in there core - we want to avoid
boilerplat like this.

> +
> +u8 *stdp2690_get_edid(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	unsigned char start = 0x00;
> +	unsigned int total_size;
> +	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= &start,
> +		}, {
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= EDID_LENGTH,
> +			.buf	= block,
> +		}
> +	};
> +
> +	if (!block)
> +		return NULL;
> +
> +	if (i2c_transfer(adapter, msgs, 2) != 2) {
> +		DRM_ERROR("Unable to read EDID.\n");
> +		goto err;
> +	}
> +
> +	if (!drm_edid_block_valid(block, 0, false, NULL)) {
> +		DRM_ERROR("Invalid EDID block\n");
> +		goto err;
> +	}
> +
> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> +	if (total_size > EDID_LENGTH) {
> +		kfree(block);
> +		block = kmalloc(total_size, GFP_KERNEL);
> +		if (!block)
> +			return NULL;
> +
> +		/* Yes, read the entire buffer, and do not skip the first
> +		 * EDID_LENGTH bytes.
> +		 */
> +		start = 0x00;
> +		msgs[1].len = total_size;
> +		msgs[1].buf = block;
> +
> +		if (i2c_transfer(adapter, msgs, 2) != 2) {
> +			DRM_ERROR("Unable to read EDID extension blocks.\n");
> +			goto err;
> +		}
> +	}
> +
> +	return block;
> +
> +err:
> +	kfree(block);
> +	return NULL;
> +}
> +
> +static int ge_b850v3_lvds_dp_get_modes(struct drm_connector *connector)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge;
> +	struct i2c_client *client;
> +	int num_modes = 0;
> +
> +	ptn_bridge = connector_to_ge_b850v3_lvds_dp(connector);
> +	client = ptn_bridge->edid_i2c;
> +
> +	mutex_lock(&ptn_bridge->lock);
> +
> +	kfree(ptn_bridge->edid);
> +	ptn_bridge->edid = (struct edid *) stdp2690_get_edid(client);
> +
> +	if (ptn_bridge->edid) {
> +		drm_mode_connector_update_edid_property(connector,
> +				ptn_bridge->edid);
> +		num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> +	}
> +
> +	mutex_unlock(&ptn_bridge->lock);
> +
> +	return num_modes;
> +}
> +
> +static struct drm_encoder
> +*ge_b850v3_lvds_dp_best_encoder(struct drm_connector *connector)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge =
> +		connector_to_ge_b850v3_lvds_dp(connector);
> +
> +	return ptn_bridge->bridge.encoder;
> +}

Please remove your best_encoder callback, that just became unecessary in
drm-next, thanks to some work from Boris.
-Daniel

> +
> +static const struct
> +drm_connector_helper_funcs ge_b850v3_lvds_dp_connector_helper_funcs = {
> +	.get_modes = ge_b850v3_lvds_dp_get_modes,
> +	.best_encoder = ge_b850v3_lvds_dp_best_encoder,
> +};
> +
> +static enum drm_connector_status ge_b850v3_lvds_dp_detect(
> +		struct drm_connector *connector, bool force)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge =
> +			connector_to_ge_b850v3_lvds_dp(connector);
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c =
> +			ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +	s32 link_state;
> +
> +	link_state = i2c_smbus_read_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_STS_REG);
> +
> +	if (link_state == STDP4028_CON_STATE_CONNECTED)
> +		return connector_status_connected;
> +
> +	if (link_state == 0)
> +		return connector_status_disconnected;
> +
> +	return connector_status_unknown;
> +}
> +
> +static void ge_b850v3_lvds_dp_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs ge_b850v3_lvds_dp_connector_funcs = {
> +	.dpms = drm_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = ge_b850v3_lvds_dp_detect,
> +	.destroy = ge_b850v3_lvds_dp_connector_destroy,
> +};
> +
> +static irqreturn_t ge_b850v3_lvds_dp_irq_handler(int irq, void *dev_id)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge = dev_id;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +
> +	mutex_lock(&ptn_bridge->lock);
> +
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> +
> +	mutex_unlock(&ptn_bridge->lock);
> +
> +	if (ptn_bridge->connector.dev)
> +		drm_kms_helper_hotplug_event(ptn_bridge->connector.dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ge_b850v3_lvds_dp_attach(struct drm_bridge *bridge)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge
> +			= bridge_to_ge_b850v3_lvds_dp(bridge);
> +	struct drm_connector *connector = &ptn_bridge->connector;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +	int ret;
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> +	drm_connector_helper_add(connector,
> +			&ge_b850v3_lvds_dp_connector_helper_funcs);
> +
> +	ret = drm_connector_init(bridge->dev, connector,
> +			&ge_b850v3_lvds_dp_connector_funcs,
> +			DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	ret = drm_mode_connector_attach_encoder(connector, bridge->encoder);
> +	if (ret)
> +		return ret;
> +
> +	drm_bridge_enable(bridge);
> +	if (ge_b850v3_lvds_dp_i2c->irq) {
> +		drm_helper_hpd_irq_event(connector->dev);
> +
> +		ret = devm_request_threaded_irq(&ge_b850v3_lvds_dp_i2c->dev,
> +				ge_b850v3_lvds_dp_i2c->irq, NULL,
> +				ge_b850v3_lvds_dp_irq_handler,
> +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +				"ge-b850v3-lvds-dp", ptn_bridge);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_bridge_funcs ge_b850v3_lvds_dp_funcs = {
> +	.enable = ge_b850v3_lvds_dp_enable,
> +	.disable = ge_b850v3_lvds_dp_disable,
> +	.attach = ge_b850v3_lvds_dp_attach,
> +};
> +
> +static int ge_b850v3_lvds_dp_probe(struct i2c_client *ge_b850v3_lvds_dp_i2c,
> +				const struct i2c_device_id *id)
> +{
> +	struct device *dev = &ge_b850v3_lvds_dp_i2c->dev;
> +	struct ge_b850v3_lvds_dp *ptn_bridge;
> +	int ret;
> +	u32 edid_i2c_reg;
> +
> +	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
> +	if (!ptn_bridge)
> +		return -ENOMEM;
> +
> +	mutex_init(&ptn_bridge->lock);
> +
> +	ptn_bridge->ge_b850v3_lvds_dp_i2c = ge_b850v3_lvds_dp_i2c;
> +	ptn_bridge->bridge.driver_private = ptn_bridge;
> +	i2c_set_clientdata(ge_b850v3_lvds_dp_i2c, ptn_bridge);
> +
> +	ret = of_property_read_u32(dev->of_node, "edid-reg", &edid_i2c_reg);
> +	if (ret) {
> +		dev_err(dev, "edid-reg not specified, aborting...\n");
> +		return -ENODEV;
> +	}
> +
> +	ptn_bridge->edid_i2c = devm_kzalloc(dev,
> +			sizeof(struct i2c_client), GFP_KERNEL);
> +
> +	if (!ptn_bridge->edid_i2c)
> +		return -ENOMEM;
> +
> +	memcpy(ptn_bridge->edid_i2c, ge_b850v3_lvds_dp_i2c,
> +			sizeof(struct i2c_client));
> +
> +	ptn_bridge->edid_i2c->addr = (unsigned short) edid_i2c_reg;
> +
> +	/* Configures the bridge to re-enable interrupts after each ack */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_IRQ_OUT_CONF_REG, STDP4028_DPTX_DP_IRQ_EN);
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_EN_REG, STDP4028_DPTX_IRQ_CONFIG);
> +
> +	/* Clear pending interrupts since power up. */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> +
> +	ptn_bridge->bridge.funcs = &ge_b850v3_lvds_dp_funcs;
> +	ptn_bridge->bridge.of_node = dev->of_node;
> +	ret = drm_bridge_add(&ptn_bridge->bridge);
> +	if (ret) {
> +		DRM_ERROR("Failed to add bridge\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ge_b850v3_lvds_dp_remove(struct i2c_client *ge_b850v3_lvds_dp_i2c)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge =
> +		i2c_get_clientdata(ge_b850v3_lvds_dp_i2c);
> +
> +	drm_bridge_remove(&ptn_bridge->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ge_b850v3_lvds_dp_i2c_table[] = {
> +	{"b850v3-lvds-dp", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ge_b850v3_lvds_dp_i2c_table);
> +
> +static const struct of_device_id ge_b850v3_lvds_dp_match[] = {
> +	{ .compatible = "ge,b850v3-lvds-dp" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ge_b850v3_lvds_dp_match);
> +
> +static struct i2c_driver ge_b850v3_lvds_dp_driver = {
> +	.id_table	= ge_b850v3_lvds_dp_i2c_table,
> +	.probe		= ge_b850v3_lvds_dp_probe,
> +	.remove		= ge_b850v3_lvds_dp_remove,
> +	.driver		= {
> +		.name		= "ge,b850v3-lvds-dp",
> +		.of_match_table = ge_b850v3_lvds_dp_match,
> +	},
> +};
> +module_i2c_driver(ge_b850v3_lvds_dp_driver);
> +
> +MODULE_AUTHOR("Peter Senna Tschudin <peter.senna@...labora.com>");
> +MODULE_AUTHOR("Martyn Welch <martyn.welch@...labora.co.uk>");
> +MODULE_DESCRIPTION("GE LVDS to DP++ bridge)");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists