[PATCH RFC 0/6] Use DRM component API in tilcdc to connect to tda998x

[PATCH RFC 0/6] Use DRM component API in tilcdc to connect to tda998x

[Jyri Sarha]

Remove tilcdc slave support and connect to tda998x trough its
component DRM API. For dtb backward compatibility the code creates at
boot time a DT overlay based on the earlier binding. The overlay
conforms to the new graph based binding.

The first patch is just a bugfix and can be applied or dropped
independenty.

Jyri Sarha (6):
  drm/tilcdc: Fix module unloading
  drm/tilcdc: Remove tilcdc slave support for tda998x driver
  drm/tilcdc: Add support for external compontised DRM encoder
  drm/tilcdc: Add DRM_TILCDC_INIT for "ti,tilcdc,slave" binding support
  drm/tilcdc: Force building of DRM_TILCDC_INIT
  ARM: dts: am335x-boneblack: Use new binding for HDMI

 .../devicetree/bindings/drm/tilcdc/slave.txt       |  18 -
 .../devicetree/bindings/drm/tilcdc/tilcdc.txt      |  27 ++
 arch/arm/boot/dts/am335x-boneblack.dts             |  20 +-
 drivers/gpu/drm/Makefile                           |   1 +
 drivers/gpu/drm/tilcdc/Kconfig                     |   6 +
 drivers/gpu/drm/tilcdc/Makefile                    |   4 +-
 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c          | 270 ++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  36 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  69 ++--
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   3 +-
 drivers/gpu/drm/tilcdc/tilcdc_external.c           | 105 ++++++
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |  24 ++
 drivers/gpu/drm/tilcdc/tilcdc_slave.c              | 411 ---------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h              |  26 --
 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts    |  70 ++++
 15 files changed, 601 insertions(+), 489 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.h
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts

-- 
1.9.1

[PATCH RFC 1/6] drm/tilcdc: Fix module unloading

[Jyri Sarha]

Force crtc dpms off before destroying the crtc instead of just
checking the dpms state. This fixes warning message and frozen picture
after tilcdc module unloading.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index c735884..c2d5980 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -135,11 +135,12 @@ static void stop(struct drm_crtc *crtc)
 	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
 }
 
+static void tilcdc_crtc_dpms(struct drm_crtc *crtc, int mode);
 static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
-	WARN_ON(tilcdc_crtc->dpms == DRM_MODE_DPMS_ON);
+	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 
 	drm_crtc_cleanup(crtc);
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
-- 
1.9.1

Re: [PATCH RFC 1/6] drm/tilcdc: Fix module unloading

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On 26/02/15 16:55, Jyri Sarha wrote:
> Force crtc dpms off before destroying the crtc instead of just
> checking the dpms state. This fixes warning message and frozen picture
> after tilcdc module unloading.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index c735884..c2d5980 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -135,11 +135,12 @@ static void stop(struct drm_crtc *crtc)
>  	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
>  }
>  
> +static void tilcdc_crtc_dpms(struct drm_crtc *crtc, int mode);
>  static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>  
> -	WARN_ON(tilcdc_crtc->dpms == DRM_MODE_DPMS_ON);
> +	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
>  
>  	drm_crtc_cleanup(crtc);
>  	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
> 

Looks fine to me, but makes me wonder what is the proper way to handle
this. I would presume that every DRM driver needs to shut down the
display HW when it's being unloaded.

And the current WARN_ON suggests that something is supposed to turn the
crtc off before it is destroyed. So either the tilcdc module unload has
never worked, or something has changed in the drm framework (or maybe
tilcdc).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH RFC 2/6] drm/tilcdc: Remove tilcdc slave support for tda998x driver

[Jyri Sarha]

Remove tilcdc slave support for tda998x driver. The tilcdc slave
support would conflicts with componentized use of tda998x.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/drm/tilcdc/slave.txt       |  18 -
 drivers/gpu/drm/tilcdc/Makefile                    |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  13 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_slave.c              | 411 ---------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h              |  26 --
 6 files changed, 470 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt b/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
deleted file mode 100644
index 3d2c524..0000000
--- a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-Device-Tree bindings for tilcdc DRM encoder slave output driver
-
-Required properties:
- - compatible: value should be "ti,tilcdc,slave".
- - i2c: the phandle for the i2c device the encoder slave is connected to
-
-Recommended properties:
- - pinctrl-names, pinctrl-0: the pincontrol settings to configure
-   muxing properly for pins that connect to TFP410 device
-
-Example:
-
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
-	};
diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index 7d2eefe..44485f9 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -6,7 +6,6 @@ endif
 tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
-	tilcdc_slave.o \
 	tilcdc_panel.o \
 	tilcdc_drv.o
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 095fca9..0f1e099 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -20,13 +20,11 @@
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 #include "tilcdc_tfp410.h"
-#include "tilcdc_slave.h"
 #include "tilcdc_panel.h"
 
 #include "drm_fb_helper.h"
 
 static LIST_HEAD(module_list);
-static bool slave_probing;
 
 void tilcdc_module_init(struct tilcdc_module *mod, const char *name,
 		const struct tilcdc_module_ops *funcs)
@@ -42,11 +40,6 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod)
 	list_del(&mod->list);
 }
 
-void tilcdc_slave_probedefer(bool defered)
-{
-	slave_probing = defered;
-}
-
 static struct of_device_id tilcdc_of_match[];
 
 static struct drm_framebuffer *tilcdc_fb_create(struct drm_device *dev,
@@ -620,10 +613,6 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	/* defer probing if slave is in deferred probing */
-	if (slave_probing == true)
-		return -EPROBE_DEFER;
-
 	return drm_platform_init(&tilcdc_driver, pdev);
 }
 
@@ -654,7 +643,6 @@ static int __init tilcdc_drm_init(void)
 {
 	DBG("init");
 	tilcdc_tfp410_init();
-	tilcdc_slave_init();
 	tilcdc_panel_init();
 	return platform_driver_register(&tilcdc_platform_driver);
 }
@@ -664,7 +652,6 @@ static void __exit tilcdc_drm_fini(void)
 	DBG("fini");
 	platform_driver_unregister(&tilcdc_platform_driver);
 	tilcdc_panel_fini();
-	tilcdc_slave_fini();
 	tilcdc_tfp410_fini();
 }
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 7596c14..6336512 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -116,7 +116,6 @@ struct tilcdc_module {
 void tilcdc_module_init(struct tilcdc_module *mod, const char *name,
 		const struct tilcdc_module_ops *funcs);
 void tilcdc_module_cleanup(struct tilcdc_module *mod);
-void tilcdc_slave_probedefer(bool defered);
 
 /* Panel config that needs to be set in the crtc, but is not coming from
  * the mode timings.  The display module is expected to call
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
deleted file mode 100644
index 3775fd4..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ /dev/null
@@ -1,411 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * 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.
- *
- * This program is distributed in the hope that 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/>.
- */
-
-#include <linux/i2c.h>
-#include <linux/pinctrl/pinmux.h>
-#include <linux/pinctrl/consumer.h>
-#include <drm/drm_encoder_slave.h>
-
-#include "tilcdc_drv.h"
-
-struct slave_module {
-	struct tilcdc_module base;
-	struct i2c_adapter *i2c;
-};
-#define to_slave_module(x) container_of(x, struct slave_module, base)
-
-static const struct tilcdc_panel_info slave_info = {
-		.bpp                    = 16,
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
-		.sync_edge              = 0,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
-};
-
-
-/*
- * Encoder:
- */
-
-struct slave_encoder {
-	struct drm_encoder_slave base;
-	struct slave_module *mod;
-};
-#define to_slave_encoder(x) container_of(to_encoder_slave(x), struct slave_encoder, base)
-
-static inline struct drm_encoder_slave_funcs *
-get_slave_funcs(struct drm_encoder *enc)
-{
-	return to_encoder_slave(enc)->slave_funcs;
-}
-
-static void slave_encoder_destroy(struct drm_encoder *encoder)
-{
-	struct slave_encoder *slave_encoder = to_slave_encoder(encoder);
-	if (get_slave_funcs(encoder))
-		get_slave_funcs(encoder)->destroy(encoder);
-	drm_encoder_cleanup(encoder);
-	kfree(slave_encoder);
-}
-
-static void slave_encoder_prepare(struct drm_encoder *encoder)
-{
-	drm_i2c_encoder_prepare(encoder);
-	tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
-}
-
-static bool slave_encoder_fixup(struct drm_encoder *encoder,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adjusted_mode)
-{
-	/*
-	 * tilcdc does not generate VESA-complient sync but aligns
-	 * VS on the second edge of HS instead of first edge.
-	 * We use adjusted_mode, to fixup sync by aligning both rising
-	 * edges and add HSKEW offset to let the slave encoder fix it up.
-	 */
-	adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
-	adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
-
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
-		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
-	} else {
-		adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
-	}
-
-	return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
-}
-
-
-static const struct drm_encoder_funcs slave_encoder_funcs = {
-		.destroy        = slave_encoder_destroy,
-};
-
-static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
-		.dpms           = drm_i2c_encoder_dpms,
-		.mode_fixup     = slave_encoder_fixup,
-		.prepare        = slave_encoder_prepare,
-		.commit         = drm_i2c_encoder_commit,
-		.mode_set       = drm_i2c_encoder_mode_set,
-		.save           = drm_i2c_encoder_save,
-		.restore        = drm_i2c_encoder_restore,
-};
-
-static const struct i2c_board_info info = {
-		I2C_BOARD_INFO("tda998x", 0x70)
-};
-
-static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
-		struct slave_module *mod)
-{
-	struct slave_encoder *slave_encoder;
-	struct drm_encoder *encoder;
-	int ret;
-
-	slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
-	if (!slave_encoder) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_encoder->mod = mod;
-
-	encoder = &slave_encoder->base.base;
-	encoder->possible_crtcs = 1;
-
-	ret = drm_encoder_init(dev, encoder, &slave_encoder_funcs,
-			DRM_MODE_ENCODER_TMDS);
-	if (ret)
-		goto fail;
-
-	drm_encoder_helper_add(encoder, &slave_encoder_helper_funcs);
-
-	ret = drm_i2c_encoder_init(dev, to_encoder_slave(encoder), mod->i2c, &info);
-	if (ret)
-		goto fail;
-
-	return encoder;
-
-fail:
-	slave_encoder_destroy(encoder);
-	return NULL;
-}
-
-/*
- * Connector:
- */
-
-struct slave_connector {
-	struct drm_connector base;
-
-	struct drm_encoder *encoder;  /* our connected encoder */
-	struct slave_module *mod;
-};
-#define to_slave_connector(x) container_of(x, struct slave_connector, base)
-
-static void slave_connector_destroy(struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-	kfree(slave_connector);
-}
-
-static enum drm_connector_status slave_connector_detect(
-		struct drm_connector *connector,
-		bool force)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->detect(encoder, connector);
-}
-
-static int slave_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->get_modes(encoder, connector);
-}
-
-static int slave_connector_mode_valid(struct drm_connector *connector,
-		  struct drm_display_mode *mode)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	int ret;
-
-	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
-	if (ret != MODE_OK)
-		return ret;
-
-	return get_slave_funcs(encoder)->mode_valid(encoder, mode);
-}
-
-static struct drm_encoder *slave_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	return slave_connector->encoder;
-}
-
-static int slave_connector_set_property(struct drm_connector *connector,
-		struct drm_property *property, uint64_t value)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->set_property(encoder,
-			connector, property, value);
-}
-
-static const struct drm_connector_funcs slave_connector_funcs = {
-	.destroy            = slave_connector_destroy,
-	.dpms               = drm_helper_connector_dpms,
-	.detect             = slave_connector_detect,
-	.fill_modes         = drm_helper_probe_single_connector_modes,
-	.set_property       = slave_connector_set_property,
-};
-
-static const struct drm_connector_helper_funcs slave_connector_helper_funcs = {
-	.get_modes          = slave_connector_get_modes,
-	.mode_valid         = slave_connector_mode_valid,
-	.best_encoder       = slave_connector_best_encoder,
-};
-
-static struct drm_connector *slave_connector_create(struct drm_device *dev,
-		struct slave_module *mod, struct drm_encoder *encoder)
-{
-	struct slave_connector *slave_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
-	if (!slave_connector) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_connector->encoder = encoder;
-	slave_connector->mod = mod;
-
-	connector = &slave_connector->base;
-
-	drm_connector_init(dev, connector, &slave_connector_funcs,
-			DRM_MODE_CONNECTOR_HDMIA);
-	drm_connector_helper_add(connector, &slave_connector_helper_funcs);
-
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-
-	connector->interlace_allowed = 0;
-	connector->doublescan_allowed = 0;
-
-	get_slave_funcs(encoder)->create_resources(encoder, connector);
-
-	ret = drm_mode_connector_attach_encoder(connector, encoder);
-	if (ret)
-		goto fail;
-
-	drm_connector_register(connector);
-
-	return connector;
-
-fail:
-	slave_connector_destroy(connector);
-	return NULL;
-}
-
-/*
- * Module:
- */
-
-static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct drm_encoder *encoder;
-	struct drm_connector *connector;
-
-	encoder = slave_encoder_create(dev, slave_mod);
-	if (!encoder)
-		return -ENOMEM;
-
-	connector = slave_connector_create(dev, slave_mod, encoder);
-	if (!connector)
-		return -ENOMEM;
-
-	priv->encoders[priv->num_encoders++] = encoder;
-	priv->connectors[priv->num_connectors++] = connector;
-
-	return 0;
-}
-
-static const struct tilcdc_module_ops slave_module_ops = {
-		.modeset_init = slave_modeset_init,
-};
-
-/*
- * Device:
- */
-
-static struct of_device_id slave_of_match[];
-
-static int slave_probe(struct platform_device *pdev)
-{
-	struct device_node *node = pdev->dev.of_node;
-	struct device_node *i2c_node;
-	struct slave_module *slave_mod;
-	struct tilcdc_module *mod;
-	struct pinctrl *pinctrl;
-	uint32_t i2c_phandle;
-	struct i2c_adapter *slavei2c;
-	int ret = -EINVAL;
-
-	/* bail out early if no DT data: */
-	if (!node) {
-		dev_err(&pdev->dev, "device-tree data is missing\n");
-		return -ENXIO;
-	}
-
-	/* Bail out early if i2c not specified */
-	if (of_property_read_u32(node, "i2c", &i2c_phandle)) {
-		dev_err(&pdev->dev, "could not get i2c bus phandle\n");
-		return ret;
-	}
-
-	i2c_node = of_find_node_by_phandle(i2c_phandle);
-	if (!i2c_node) {
-		dev_err(&pdev->dev, "could not get i2c bus node\n");
-		return ret;
-	}
-
-	/* but defer the probe if it can't be initialized it might come later */
-	slavei2c = of_find_i2c_adapter_by_node(i2c_node);
-	of_node_put(i2c_node);
-
-	if (!slavei2c) {
-		ret = -EPROBE_DEFER;
-		tilcdc_slave_probedefer(true);
-		dev_err(&pdev->dev, "could not get i2c\n");
-		return ret;
-	}
-
-	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod) {
-		ret = -ENOMEM;
-		goto fail_adapter;
-	}
-
-	mod = &slave_mod->base;
-	pdev->dev.platform_data = mod;
-
-	mod->preferred_bpp = slave_info.bpp;
-
-	slave_mod->i2c = slavei2c;
-
-	tilcdc_module_init(mod, "slave", &slave_module_ops);
-
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev, "pins are not configured\n");
-
-	tilcdc_slave_probedefer(false);
-
-	return 0;
-
-fail_adapter:
-	i2c_put_adapter(slavei2c);
-	return ret;
-}
-
-static int slave_remove(struct platform_device *pdev)
-{
-	struct tilcdc_module *mod = dev_get_platdata(&pdev->dev);
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-
-	return 0;
-}
-
-static struct of_device_id slave_of_match[] = {
-		{ .compatible = "ti,tilcdc,slave", },
-		{ },
-};
-
-struct platform_driver slave_driver = {
-	.probe = slave_probe,
-	.remove = slave_remove,
-	.driver = {
-		.owner = THIS_MODULE,
-		.name = "slave",
-		.of_match_table = slave_of_match,
-	},
-};
-
-int __init tilcdc_slave_init(void)
-{
-	return platform_driver_register(&slave_driver);
-}
-
-void __exit tilcdc_slave_fini(void)
-{
-	platform_driver_unregister(&slave_driver);
-}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.h b/drivers/gpu/drm/tilcdc/tilcdc_slave.h
deleted file mode 100644
index 2f85048..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * 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.
- *
- * This program is distributed in the hope that 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/>.
- */
-
-#ifndef __TILCDC_SLAVE_H__
-#define __TILCDC_SLAVE_H__
-
-/* sub-module for i2c slave encoder output */
-
-int tilcdc_slave_init(void);
-void tilcdc_slave_fini(void);
-
-#endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Jyri Sarha]

Add support for an external compontised DRM encoder. The external
encoder can be connected to tilcdc trough device tree graph binding.
The binding document for tilcdc has been updated. The support has only
been tested with tda998x encoder, but other encoders should work too
with a little tweaking.

I got the idea and some lines of code from Jean-Francois Moine's
"drm/tilcdc: Change the interface with the tda998x driver"-patch.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/drm/tilcdc/tilcdc.txt      |  27 ++++++
 drivers/gpu/drm/tilcdc/Makefile                    |   1 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  33 +++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  56 ++++++++---
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_external.c           | 105 +++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |  24 +++++
 7 files changed, 235 insertions(+), 13 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.h

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
index fff10da..2136ee8 100644
--- a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
@@ -18,6 +18,12 @@ Optional properties:
  - max-pixelclock: The maximum pixel clock that can be supported
    by the lcd controller in KHz.
 
+Optional nodes:
+
+ - port/ports: to describe a connection to an external encoder. The
+   binding follows Documentation/devicetree/bindings/graph.txt and
+   suppors a single port with a single endpoint.
+
 Example:
 
 	fb: fb@4830e000 {
@@ -26,4 +32,25 @@ Example:
 		interrupt-parent = <&intc>;
 		interrupts = <36>;
 		ti,hwmods = "lcdc";
+
+		port {
+			lcdc_0: endpoint@0 {
+				remote-endpoint = <&hdmi_0>;
+			};
+		};
+	};
+
+	tda19988: tda19988 {
+		compatible = "nxp,tda998x";
+		reg = <0x70>;
+
+		pinctrl-names = "default", "off";
+		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
+		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
+
+		port {
+			hdmi_0: endpoint@0 {
+				remote-endpoint = <&lcdc_0>;
+			};
+		};
 	};
diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index 44485f9..e1f738b 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -7,6 +7,7 @@ tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
 	tilcdc_panel.o \
+	tilcdc_external.o \
 	tilcdc_drv.o
 
 obj-$(CONFIG_DRM_TILCDC)	+= tilcdc.o
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index c2d5980..7d07733 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -37,6 +37,9 @@ struct tilcdc_crtc {
 
 	/* for deferred fb unref's: */
 	struct drm_flip_work unref_work;
+
+	/* Only set if an external encoder is connected */
+	bool simulate_vesa_sync;
 };
 #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
 
@@ -214,6 +217,28 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 		const struct drm_display_mode *mode,
 		struct drm_display_mode *adjusted_mode)
 {
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+	if (!tilcdc_crtc->simulate_vesa_sync)
+		return true;
+
+	/*
+	 * tilcdc does not generate VESA-compliant sync but aligns
+	 * VS on the second edge of HS instead of first edge.
+	 * We use adjusted_mode, to fixup sync by aligning both rising
+	 * edges and add HSKEW offset to fix the sync.
+	 */
+	adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+	adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+		adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+	} else {
+		adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+	}
+
 	return true;
 }
 
@@ -534,6 +559,14 @@ void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
 	tilcdc_crtc->info = info;
 }
 
+void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
+					bool simulate_vesa_sync)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+	tilcdc_crtc->simulate_vesa_sync = simulate_vesa_sync;
+}
+
 void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 0f1e099..50e1be1 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -17,10 +17,13 @@
 
 /* LCDC DRM driver, based on da8xx-fb */
 
+#include <linux/component.h>
+
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 #include "tilcdc_tfp410.h"
 #include "tilcdc_panel.h"
+#include "tilcdc_external.h"
 
 #include "drm_fb_helper.h"
 
@@ -73,13 +76,6 @@ static int modeset_init(struct drm_device *dev)
 		mod->funcs->modeset_init(mod, dev);
 	}
 
-	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
-		/* oh nos! */
-		dev_err(dev->dev, "no encoders/connectors found\n");
-		drm_mode_config_cleanup(dev);
-		return -ENXIO;
-	}
-
 	dev->mode_config.min_width = 0;
 	dev->mode_config.min_height = 0;
 	dev->mode_config.max_width = tilcdc_crtc_max_width(priv->crtc);
@@ -253,10 +249,28 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		goto fail_cpufreq_unregister;
 	}
 
+	platform_set_drvdata(pdev, dev);
+
+	ret = component_bind_all(dev->dev, dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);
+		goto fail_mode_config_cleanup;
+	}
+
+	ret = tilcdc_add_external_encoders(dev, &bpp);
+	if (ret < 0)
+		goto fail_component_cleanup;
+
+	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
+		dev_err(dev->dev, "no encoders/connectors found\n");
+		ret = -ENXIO;
+		goto fail_component_cleanup;
+	}
+
 	ret = drm_vblank_init(dev, 1);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to initialize vblank\n");
-		goto fail_mode_config_cleanup;
+		goto fail_component_cleanup;
 	}
 
 	pm_runtime_get_sync(dev->dev);
@@ -267,9 +281,6 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		goto fail_vblank_cleanup;
 	}
 
-	platform_set_drvdata(pdev, dev);
-
-
 	list_for_each_entry(mod, &module_list, list) {
 		DBG("%s: preferred_bpp: %d", mod->name, mod->preferred_bpp);
 		bpp = mod->preferred_bpp;
@@ -300,6 +311,9 @@ fail_vblank_cleanup:
 fail_mode_config_cleanup:
 	drm_mode_config_cleanup(dev);
 
+fail_component_cleanup:
+	component_unbind_all(dev->dev, dev);
+
 fail_cpufreq_unregister:
 	pm_runtime_disable(dev->dev);
 #ifdef CONFIG_CPU_FREQ
@@ -605,6 +619,22 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
  * Platform driver:
  */
 
+static int tilcdc_bind(struct device *dev)
+{
+	return drm_platform_init(&tilcdc_driver, to_platform_device(dev));
+}
+
+static void tilcdc_unbind(struct device *dev)
+{
+	drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops tilcdc_comp_ops = {
+	.add_components = tilcdc_add_external_components,
+	.bind = tilcdc_bind,
+	.unbind = tilcdc_unbind,
+};
+
 static int tilcdc_pdev_probe(struct platform_device *pdev)
 {
 	/* bail out early if no DT data: */
@@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	return drm_platform_init(&tilcdc_driver, pdev);
+	return component_master_add(&pdev->dev, &tilcdc_comp_ops);
 }
 
 static int tilcdc_pdev_remove(struct platform_device *pdev)
 {
-	drm_put_dev(platform_get_drvdata(pdev));
+	component_master_del(&pdev->dev, &tilcdc_comp_ops);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 6336512..70a06c7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -165,6 +165,8 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc);
 void tilcdc_crtc_update_clk(struct drm_crtc *crtc);
 void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
 		const struct tilcdc_panel_info *info);
+void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
+					bool simulate_vesa_sync);
 int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
new file mode 100644
index 0000000..7254151
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2015 Texas Instruments
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * 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.
+ *
+ */
+#define DEBUG 1
+
+#include <linux/component.h>
+#include <linux/of_graph.h>
+
+#include "tilcdc_drv.h"
+#include "tilcdc_external.h"
+
+static const struct tilcdc_panel_info panel_info_defaults = {
+		.ac_bias                = 255,
+		.ac_bias_intrpt         = 0,
+		.dma_burst_sz           = 16,
+		.bpp                    = 16,
+		.fdd                    = 0x80,
+		.tft_alt_mode           = 0,
+		.invert_pxl_clk		= 1,
+		.sync_edge              = 1,
+		.sync_ctrl              = 1,
+		.raster_order           = 0,
+};
+
+static int tilcdc_add_external_encoder(struct drm_device *dev, int *bpp,
+				       struct drm_connector *connector)
+{
+	struct tilcdc_drm_private *priv = dev->dev_private;
+
+	priv->connectors[priv->num_connectors++] = connector;
+	priv->encoders[priv->num_encoders++] = connector->encoder;
+
+	tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
+	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_defaults);
+	*bpp = panel_info_defaults.bpp;
+
+	dev_info(dev->dev, "External encoder '%s' connected\n",
+		 connector->encoder->name);
+
+	return 0;
+}
+
+
+int tilcdc_add_external_encoders(struct drm_device *dev, int *bpp)
+{
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	struct drm_connector *connector;
+	int num_internal_connectors = priv->num_connectors;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		bool found = false;
+		int i, ret;
+
+		for (i = 0; i < num_internal_connectors; i++)
+			if (connector == priv->connectors[i])
+				found = true;
+		if (!found) {
+			ret = tilcdc_add_external_encoder(dev, bpp, connector);
+			if (ret)
+				return ret;
+		}
+	}
+	if (priv->num_connectors - num_internal_connectors > 1) {
+		dev_err(dev->dev, "Only one external encoder is supported.");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+int tilcdc_add_external_components(struct device *master,
+				   struct master *m)
+{
+	struct device_node *ep = NULL;
+
+	while ((ep = of_graph_get_next_endpoint(master->of_node, ep))) {
+		struct device_node *node;
+		int ret;
+
+		node = of_graph_get_remote_port_parent(ep);
+		of_node_put(ep);
+		if (!node || !of_device_is_available(node))
+			continue;
+
+		dev_info(master, "Subdevice node '%s' found\n", node->name);
+		ret = component_master_add_child(m, of_dev_node_match, node);
+		of_node_put(node);
+		if (ret) {
+			dev_err(master, "Adding component failed: %d\n", ret);
+			of_node_put(ep);
+			return ret;
+		}
+	}
+	return 0;
+}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
new file mode 100644
index 0000000..8acf3a1
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2015 Texas Instruments
+ * Author: Jyri Sarha <jsarha@gmail.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that 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/>.
+ */
+
+#ifndef __TILCDC_EXTERNAL_H__
+#define __TILCDC_EXTERNAL_H__
+
+int tilcdc_add_external_encoders(struct drm_device *dev, int *bpp);
+int tilcdc_add_external_components(struct device *master, struct master *m);
+
+#endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]

On 26/02/15 16:55, Jyri Sarha wrote:
> Add support for an external compontised DRM encoder. The external
> encoder can be connected to tilcdc trough device tree graph binding.
> The binding document for tilcdc has been updated. The support has only
> been tested with tda998x encoder, but other encoders should work too
> with a little tweaking.
> 
> I got the idea and some lines of code from Jean-Francois Moine's
> "drm/tilcdc: Change the interface with the tda998x driver"-patch.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---

<snip>

> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> new file mode 100644
> index 0000000..7254151
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (C) 2015 Texas Instruments
> + * Author: Jyri Sarha <jsarha@ti.com>
> + *
> + * 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.
> + *
> + */
> +#define DEBUG 1

You probably didn't mean to include this.

> +
> +#include <linux/component.h>
> +#include <linux/of_graph.h>
> +
> +#include "tilcdc_drv.h"
> +#include "tilcdc_external.h"
> +
> +static const struct tilcdc_panel_info panel_info_defaults = {
> +		.ac_bias                = 255,
> +		.ac_bias_intrpt         = 0,
> +		.dma_burst_sz           = 16,
> +		.bpp                    = 16,
> +		.fdd                    = 0x80,
> +		.tft_alt_mode           = 0,
> +		.invert_pxl_clk		= 1,
> +		.sync_edge              = 1,
> +		.sync_ctrl              = 1,
> +		.raster_order           = 0,
> +};
> +
> +static int tilcdc_add_external_encoder(struct drm_device *dev, int *bpp,
> +				       struct drm_connector *connector)
> +{
> +	struct tilcdc_drm_private *priv = dev->dev_private;
> +
> +	priv->connectors[priv->num_connectors++] = connector;
> +	priv->encoders[priv->num_encoders++] = connector->encoder;
> +
> +	tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
> +	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_defaults);

Setting of the simulate vesa sync and the panel info here look a bit
like a hack to me. Both of them are for tda998x, not "defaults".

So... I don't know. You could state that at the moment tilcdc only
supports tda998x as an external encoder. Then the above would be ok, but
still it would be good to clearly state this in the desc, comments and
variable names.

Doing this properly may be more difficult. Some parameters should be
defined in the .dts, some should probably come from tda998x driver, and
some should be deduced by tilcdc driver internally.

> +	*bpp = panel_info_defaults.bpp;
> +
> +	dev_info(dev->dev, "External encoder '%s' connected\n",
> +		 connector->encoder->name);

This and the other dev_info in this patch look more like dev_dbg to me.

> +
> +	return 0;
> +}
> +
> +
> +int tilcdc_add_external_encoders(struct drm_device *dev, int *bpp)
> +{
> +	struct tilcdc_drm_private *priv = dev->dev_private;
> +	struct drm_connector *connector;
> +	int num_internal_connectors = priv->num_connectors;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		bool found = false;
> +		int i, ret;
> +
> +		for (i = 0; i < num_internal_connectors; i++)
> +			if (connector == priv->connectors[i])
> +				found = true;
> +		if (!found) {
> +			ret = tilcdc_add_external_encoder(dev, bpp, connector);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	if (priv->num_connectors - num_internal_connectors > 1) {
> +		dev_err(dev->dev, "Only one external encoder is supported.");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +int tilcdc_add_external_components(struct device *master,
> +				   struct master *m)
> +{
> +	struct device_node *ep = NULL;
> +
> +	while ((ep = of_graph_get_next_endpoint(master->of_node, ep))) {
> +		struct device_node *node;
> +		int ret;
> +
> +		node = of_graph_get_remote_port_parent(ep);
> +		of_node_put(ep);

Note that there's an unmerged series "Add of-graph helpers to loop over
endpoints and find ports by id" from Philipp which changes the behavior
of of_graph_get_next_endpoint.

> +		if (!node || !of_device_is_available(node))
> +			continue;

Should you of_node_put(node) if node != NULL above?

> +
> +		dev_info(master, "Subdevice node '%s' found\n", node->name);
> +		ret = component_master_add_child(m, of_dev_node_match, node);
> +		of_node_put(node);
> +		if (ret) {
> +			dev_err(master, "Adding component failed: %d\n", ret);
> +			of_node_put(ep);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}

I don't know if it matters, but as tilcdc only supports a single
endpoint, and I think this is the earliest place where it can be
detected, you could fail above if there are more than one endpoint.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Russell King - ARM Linux]

On Thu, Feb 26, 2015 at 04:55:32PM +0200, Jyri Sarha wrote:
> +	ret = component_bind_all(dev->dev, dev);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);

Do you need to print this?  The component helper is already fairly
verbose about what succeeds and fails.

> +static const struct component_master_ops tilcdc_comp_ops = {
> +	.add_components = tilcdc_add_external_components,

I'd much rather you used the new matching support rather than using the
old .add_components.  The new matching support is more efficient as you
only have to scan DT once, rather than each time we try to probe.  That
will mean...

> @@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
>  		return -ENXIO;
>  	}

You need to build a struct component_match array here using
component_match_add()...

>  
> -	return drm_platform_init(&tilcdc_driver, pdev);
> +	return component_master_add(&pdev->dev, &tilcdc_comp_ops);

and then finally register the ops with component_master_add_with_match().

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Jyri Sarha]

On 03/02/15 18:01, Russell King - ARM Linux wrote:
> On Thu, Feb 26, 2015 at 04:55:32PM +0200, Jyri Sarha wrote:
>> +	ret = component_bind_all(dev->dev, dev);
>> +	if (ret < 0) {
>> +		dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);
>
> Do you need to print this?  The component helper is already fairly
> verbose about what succeeds and fails.
>

Will remove.

>> +static const struct component_master_ops tilcdc_comp_ops = {
>> +	.add_components = tilcdc_add_external_components,
>
> I'd much rather you used the new matching support rather than using the
> old .add_components.  The new matching support is more efficient as you
> only have to scan DT once, rather than each time we try to probe.  That
> will mean...
>

That is otherwise fine, but with the match code it not possible to 
implement a master that may not have any components.

Would it be Ok to add a check that master->ops->add_components is 
defined, before calling it in find_componets() 
(drivers/base/component.c:120) and return 0 if it is not?

Best regards,
Jyri

>> @@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
>>   		return -ENXIO;
>>   	}
>
> You need to build a struct component_match array here using
> component_match_add()...
>
>>
>> -	return drm_platform_init(&tilcdc_driver, pdev);
>> +	return component_master_add(&pdev->dev, &tilcdc_comp_ops);
>
> and then finally register the ops with component_master_add_with_match().
>
> Thanks.
>

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote:
> Would it be Ok to add a check that master->ops->add_components is defined,
> before calling it in find_componets() (drivers/base/component.c:120) and
> return 0 if it is not?

No:

http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3

also:

http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e

is what's planned to be merged when I can get a round tuit, and people
stop using the old methods.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Jyri Sarha]

On 03/06/15 11:58, Russell King - ARM Linux wrote:
> On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote:
>> Would it be Ok to add a check that master->ops->add_components is defined,
>> before calling it in find_componets() (drivers/base/component.c:120) and
>> return 0 if it is not?
>
> No:
>
> http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3
>
> also:
>
> http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e
>
> is what's planned to be merged when I can get a round tuit, and people
> stop using the old methods.
>

Ok, but could it still be allowed to add a master without any components 
(match == NULL)?

Or do I have to handle the configurations without any components separately?

Best regards,
Jyri

Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 12:21:42PM +0200, Jyri Sarha wrote:
> On 03/06/15 11:58, Russell King - ARM Linux wrote:
> >On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote:
> >>Would it be Ok to add a check that master->ops->add_components is defined,
> >>before calling it in find_componets() (drivers/base/component.c:120) and
> >>return 0 if it is not?
> >
> >No:
> >
> >http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3
> >
> >also:
> >
> >http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e
> >
> >is what's planned to be merged when I can get a round tuit, and people
> >stop using the old methods.
> >
> 
> Ok, but could it still be allowed to add a master without any components
> (match == NULL)?
> 
> Or do I have to handle the configurations without any components separately?

That's not a decision I want to make in my current state.  Give me a
couple of week or two and re-ping me.

http://archive.arm.linux.org.uk/lurker/message/20150306.102749.fcabd2bf.en.html

(and the reason becomes self-evident when you realise that message did
not go to the right list on Tuesday evening when it was meant to.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH RFC 4/6] drm/tilcdc: Add DRM_TILCDC_INIT for "ti,tilcdc,slave" binding support

[Jyri Sarha]

Adds a DRM_TILCDC_INIT module for "ti,tilcdc,slave" node
conversion. The implementation is in tilcdc_boot_init.c and it uses
tilcdc_slave_convert.dts as a basis for creating a DTS overlay. The
DTS overlay adds an external tda998x encoder to tilcdc that
corresponds to the old tda998x based slave encoder.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/Kconfig                  |   6 +
 drivers/gpu/drm/tilcdc/Makefile                 |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c       | 270 ++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts |  70 ++++++
 4 files changed, 348 insertions(+)
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts

diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
index 8394a0b..c35d088 100644
--- a/drivers/gpu/drm/tilcdc/Kconfig
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -1,3 +1,8 @@
+config DRM_TILCDC_INIT
+	select OF_RESOLVE
+	select OF_OVERLAY
+	bool
+
 config DRM_TILCDC
 	tristate "DRM Support for TI LCDC Display Controller"
 	depends on DRM && OF && ARM && HAVE_DMA_ATTRS
@@ -8,6 +13,7 @@ config DRM_TILCDC
 	select VIDEOMODE_HELPERS
 	select BACKLIGHT_CLASS_DEVICE
 	select BACKLIGHT_LCD_SUPPORT
+	select DRM_TILCDC_INIT
 	help
 	  Choose this option if you have an TI SoC with LCDC display
 	  controller, for example AM33xx in beagle-bone, DA8xx, or
diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index e1f738b..fa184a5 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -3,6 +3,8 @@ ifeq (, $(findstring -W,$(EXTRA_CFLAGS)))
 	ccflags-y += -Werror
 endif
 
+obj-$(CONFIG_DRM_TILCDC_INIT) += tilcdc_boot_init.o tilcdc_slave_convert.dtb.o
+
 tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_boot_init.c b/drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
new file mode 100644
index 0000000..08f3110
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
@@ -0,0 +1,270 @@
+/*
+ * Copyright (C) 2015 Texas Instruments
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * 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.
+ *
+ */
+
+/*
+ * To support the old "ti,tilcdc,slave" binding the binding has to be
+ * transformed to the new external encoder binding.
+ */
+
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/of_fdt.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+
+struct kfree_table {
+	int total;
+	int num;
+	void **table;
+};
+
+static int __init kfree_table_init(struct kfree_table *kft)
+{
+	kft->total = 128;
+	kft->num = 0;
+	kft->table = kmalloc(kft->total * sizeof(*kft->table),
+			     GFP_KERNEL);
+	if (!kft->table)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int __init kfree_table_add(struct kfree_table *kft, void *p)
+{
+	if (kft->num == kft->total) {
+		void *old = kft->table;
+
+		kft->total *= 2;
+		kft->table = krealloc(old, kft->total * sizeof(*kft->table),
+				      GFP_KERNEL);
+		if (!kft->table) {
+			kft->table = old;
+			kfree(p);
+			return -ENOMEM;
+		}
+	}
+	kft->table[kft->num++] = p;
+	return 0;
+}
+
+static void __init kfree_table_free(struct kfree_table *kft)
+{
+	int i;
+
+	for (i = 0; i < kft->num; i++)
+		kfree(kft->table[i]);
+
+	kfree(kft->table);
+}
+
+static
+struct property * __init tilcdc_prop_dup(const struct property *prop,
+					 struct kfree_table *kft)
+{
+	struct property *nprop;
+
+	nprop = kzalloc(sizeof(*nprop), GFP_KERNEL);
+	if (!nprop || kfree_table_add(kft, nprop))
+		return NULL;
+
+	nprop->name = kstrdup(prop->name, GFP_KERNEL);
+	if (!nprop->name || kfree_table_add(kft, nprop->name))
+		return NULL;
+
+	nprop->value = kmemdup(prop->value, prop->length, GFP_KERNEL);
+	if (!nprop->value || kfree_table_add(kft, nprop->value))
+		return NULL;
+
+	nprop->length = prop->length;
+
+	return nprop;
+}
+
+static void __init tilcdc_copy_props(struct device_node *from,
+				     struct device_node *to,
+				     const char * const props[],
+				     struct kfree_table *kft)
+{
+	struct property *prop;
+	int i;
+
+	for (i = 0; props[i]; i++) {
+		prop = of_find_property(from, props[i], NULL);
+		if (!prop)
+			continue;
+
+		prop = tilcdc_prop_dup(prop, kft);
+		if (!prop)
+			continue;
+
+		prop->next = to->properties;
+		to->properties = prop;
+	}
+}
+
+static int __init tilcdc_prop_str_update(struct property *prop,
+					  const char *str,
+					  struct kfree_table *kft)
+{
+	prop->value = kstrdup(str, GFP_KERNEL);
+	if (kfree_table_add(kft, prop->value) || !prop->value)
+		return -ENOMEM;
+	prop->length = strlen(str)+1;
+	return 0;
+}
+
+static void __init tilcdc_node_disable(struct device_node *node)
+{
+	struct property *prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->name = "status";
+	prop->value = "disabled";
+	prop->length = strlen((char *)prop->value)+1;
+
+	of_update_property(node, prop);
+}
+
+struct device_node * __init tilcdc_get_overlay(struct kfree_table *kft)
+{
+	extern uint8_t __dtb_tilcdc_slave_convert_begin[];
+	extern uint8_t __dtb_tilcdc_slave_convert_end[];
+	const int size = __dtb_tilcdc_slave_convert_end -
+		__dtb_tilcdc_slave_convert_begin;
+	static void *overlay_data;
+	struct device_node *overlay;
+	int ret;
+
+	if (!size) {
+		pr_warn("%s: No overlay data\n", __func__);
+		return NULL;
+	}
+
+	overlay_data = kmemdup(__dtb_tilcdc_slave_convert_begin,
+			       size, GFP_KERNEL);
+	if (!overlay_data || kfree_table_add(kft, overlay_data))
+		return NULL;
+
+	of_fdt_unflatten_tree(overlay_data, &overlay);
+	if (!overlay) {
+		pr_warn("%s: Unfattening overlay tree failed\n", __func__);
+		return NULL;
+	}
+
+	of_node_set_flag(overlay, OF_DETACHED);
+	ret = of_resolve_phandles(overlay);
+	if (ret) {
+		pr_err("%s: Failed to resolve phandles: %d\n", __func__, ret);
+		return NULL;
+	}
+
+	return overlay;
+}
+
+static const struct of_device_id tilcdc_slave_of_match[] __initconst = {
+	{ .compatible = "ti,tilcdc,slave", },
+	{},
+};
+
+static const struct of_device_id tilcdc_of_match[] __initconst = {
+	{ .compatible = "ti,am33xx-tilcdc", },
+	{},
+};
+
+static const struct of_device_id tilcdc_tda998x_of_match[] __initconst = {
+	{ .compatible = "nxp,tda998x", },
+	{},
+};
+
+static const char * const tilcdc_slave_props[] __initconst = {
+	"pinctrl-names",
+	"pinctrl-0",
+	"pinctrl-1",
+	NULL
+};
+
+void __init tilcdc_convert_slave_node(void)
+{
+	struct device_node *slave = NULL, *lcdc = NULL;
+	struct device_node *i2c = NULL, *fragment = NULL;
+	struct device_node *overlay, *encoder;
+	struct property *prop;
+	/* For all memory needed for the overlay tree. This memory can
+	   be freed after the overlay has been applied. */
+	struct kfree_table kft;
+	int ret;
+
+	if (kfree_table_init(&kft))
+		goto out;
+
+	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
+	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
+
+	if (!slave || !of_device_is_available(lcdc))
+		goto out;
+
+	i2c = of_parse_phandle(slave, "i2c", 0);
+	if (!i2c) {
+		pr_err("%s: Can't find i2c node trough phandle\n", __func__);
+		goto out;
+	}
+
+	overlay = tilcdc_get_overlay(&kft);
+	if (!overlay)
+		goto out;
+
+	encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match);
+	if (!encoder) {
+		pr_err("%s: Failed to find tda998x node\n", __func__);
+		goto out;
+	}
+
+	tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft);
+
+	for_each_child_of_node(overlay, fragment) {
+		prop = of_find_property(fragment, "target-path", NULL);
+		if (!prop)
+			continue;
+		if (!strncmp("i2c", (char *)prop->value, prop->length))
+			if (tilcdc_prop_str_update(prop, i2c->full_name, &kft))
+				goto out;
+		if (!strncmp("lcdc", (char *)prop->value, prop->length))
+			if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft))
+				goto out;
+	}
+
+	tilcdc_node_disable(slave);
+
+	ret = of_overlay_create(overlay);
+	if (ret)
+		pr_err("%s: Creating overlay failed: %d\n", __func__, ret);
+	else
+		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
+			__func__);
+out:
+	kfree_table_free(&kft);
+	of_node_put(i2c);
+	of_node_put(slave);
+	of_node_put(lcdc);
+	of_node_put(fragment);
+}
+
+int __init tilcdc_boot_init(void)
+{
+	tilcdc_convert_slave_node();
+	return 0;
+}
+
+subsys_initcall(tilcdc_boot_init);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts b/drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts
new file mode 100644
index 0000000..8a2bfa9
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts
@@ -0,0 +1,70 @@
+/*
+ * DTS overlay for converting ti,tilcdc,slave binding to new binding.
+ *
+ * Copyright (C) 2015 Texas Instruments Inc.
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * 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.
+ */
+/*
+ * target-path property values are simple tags that are replaced with
+ * correct values in tildcdc_boot_init.c. Some properties are also copied
+ * over from the ti,tilcdc,slave node.
+ */
+/dts-v1/;
+/ {
+	fragment@0 {
+		target-path = "i2c";
+		__overlay__ {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			tda19988 {
+				compatible = "nxp,tda998x";
+				reg = <0x70>;
+				status = "okay";
+
+				port {
+					hdmi_0: endpoint@0 {
+						remote-endpoint = <&lcd_0>;
+					};
+				};
+			};
+		};
+	};
+
+	fragment@1 {
+		target-path = "lcdc";
+		__overlay__ {
+			port {
+				lcd_0: endpoint@0 {
+					remote-endpoint = <&hdmi_0>;
+				};
+			};
+		};
+	};
+
+	__local_fixups__ {
+		fragment@0 {
+			__overlay__ {
+				tda19988 {
+					port {
+						endpoint@0 {
+							remote-endpoint	= <0>;
+						};
+					};
+				};
+			};
+		};
+		fragment@1 {
+			__overlay__ {
+				port {
+					endpoint@0 {
+						remote-endpoint	= <0>;
+					};
+				};
+			};
+		};
+	};
+};
-- 
1.9.1

Re: [PATCH RFC 4/6] drm/tilcdc: Add DRM_TILCDC_INIT for "ti,tilcdc,slave" binding support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On 26/02/15 16:55, Jyri Sarha wrote:
> Adds a DRM_TILCDC_INIT module for "ti,tilcdc,slave" node
> conversion. The implementation is in tilcdc_boot_init.c and it uses
> tilcdc_slave_convert.dts as a basis for creating a DTS overlay. The
> DTS overlay adds an external tda998x encoder to tilcdc that
> corresponds to the old tda998x based slave encoder.
> 
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/gpu/drm/tilcdc/Kconfig                  |   6 +
>  drivers/gpu/drm/tilcdc/Makefile                 |   2 +
>  drivers/gpu/drm/tilcdc/tilcdc_boot_init.c       | 270 ++++++++++++++++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts |  70 ++++++
>  4 files changed, 348 insertions(+)
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts

All this is needed to support the old bindings, right? So I would
suggest scratching "DRM_TILCDC_INIT", and create
"DRM_TILCDC_COMPAT_SLAVE" or whatever, which would be a user visible
option, enabled by default.

This way one can just disable all this compatibility stuff if it's not
needed.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH RFC 5/6] drm/tilcdc: Force building of DRM_TILCDC_INIT

[Jyri Sarha]

If I read Documentation/kbuild/makefiles.txt section 3.6 right, this
patch should not be needed. However, without this patch the objects
needed for DRM_TILCDC_INIT are not linked, if DRM_TILCDC is built as
module.

Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
 drivers/gpu/drm/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2c239b9..62c6158 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-$(CONFIG_DRM_OMAP)	+= omapdrm/
 obj-$(CONFIG_DRM_TILCDC)	+= tilcdc/
+obj-$(CONFIG_DRM_TILCDC_INIT)	+= tilcdc/
 obj-$(CONFIG_DRM_QXL) += qxl/
 obj-$(CONFIG_DRM_BOCHS) += bochs/
 obj-$(CONFIG_DRM_MSM) += msm/
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 5/6] drm/tilcdc: Force building of DRM_TILCDC_INIT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

On 26/02/15 16:55, Jyri Sarha wrote:
> If I read Documentation/kbuild/makefiles.txt section 3.6 right, this
> patch should not be needed. However, without this patch the objects
> needed for DRM_TILCDC_INIT are not linked, if DRM_TILCDC is built as
> module.

I also think there's something funny either with the documentation or
the kernel build system.

To summarize (Jyri correct me if I'm wrong):

We enter tilcdc directory using "obj-$(CONFIG_DRM_TILCDC)", and
CONFIG_DRM_TILCDC is either y or m. Inside the directory we have the
necessary makefile and code to build the driver as module or built-in.
And this works fine.

But now Jyri added new piece of code to tilcdc directory, which is
always built-in, even if the tilcdc driver itself is a module. So now if
we enter the directory with "obj-m", and inside the tilcdc/Makefile we
do "obj-y += foo.o" line, the result is that foo.o is compiled, but it
is not linked either to the kernel image nor to the tilcdc.ko.

The doc says:

"Kbuild only uses this information to decide that it needs to visit
the directory, it is the Makefile in the subdirectory that
specifies what is modular and what is built-in."

which doesn't seem to work for us.

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2c239b9..62c6158 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
>  obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
>  obj-$(CONFIG_DRM_OMAP)	+= omapdrm/
>  obj-$(CONFIG_DRM_TILCDC)	+= tilcdc/
> +obj-$(CONFIG_DRM_TILCDC_INIT)	+= tilcdc/
>  obj-$(CONFIG_DRM_QXL) += qxl/
>  obj-$(CONFIG_DRM_BOCHS) += bochs/
>  obj-$(CONFIG_DRM_MSM) += msm/

I don't think the above is right. You add two rules for tilcdc
directory. I presume you meant to replace the current tilcdc line with
the new one?

But I don't think the new line makes sense. Using
obj-$(CONFIG_DRM_TILCDC) above makes sense, and also obj-y makes sense,
but obj-$(CONFIG_DRM_TILCDC_INIT) doesn't.

So I think it should be just obj-y, if obj-$(CONFIG_DRM_TILCDC) does not
work.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Jyri Sarha]

Use new binding for the external tda19988 HDMI encoder.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 arch/arm/boot/dts/am335x-boneblack.dts | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index 5c42d25..eadbba3 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -68,16 +68,26 @@
 
 &lcdc {
 	status = "okay";
+	port {
+		lcdc_0: endpoint@0 {
+			remote-endpoint = <&hdmi_0>;
+		};
+	};
 };
 
-/ {
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
+&i2c0 {
+	tda19988 {
+		compatible = "nxp,tda998x";
+		reg = <0x70>;
 		pinctrl-names = "default", "off";
 		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
 		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
-		status = "okay";
+
+		port {
+			hdmi_0: endpoint@0 {
+				remote-endpoint = <&lcdc_0>;
+			};
+		};
 	};
 };
 
-- 
1.9.1

Re: [PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On 26/02/15 16:55, Jyri Sarha wrote:
> Use new binding for the external tda19988 HDMI encoder.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  arch/arm/boot/dts/am335x-boneblack.dts | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
> index 5c42d25..eadbba3 100644
> --- a/arch/arm/boot/dts/am335x-boneblack.dts
> +++ b/arch/arm/boot/dts/am335x-boneblack.dts
> @@ -68,16 +68,26 @@
>  
>  &lcdc {
>  	status = "okay";
> +	port {
> +		lcdc_0: endpoint@0 {
> +			remote-endpoint = <&hdmi_0>;
> +		};
> +	};
>  };
>  
> -/ {
> -	hdmi {
> -		compatible = "ti,tilcdc,slave";
> -		i2c = <&i2c0>;
> +&i2c0 {
> +	tda19988 {
> +		compatible = "nxp,tda998x";
> +		reg = <0x70>;
>  		pinctrl-names = "default", "off";
>  		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
>  		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
> -		status = "okay";
> +
> +		port {
> +			hdmi_0: endpoint@0 {
> +				remote-endpoint = <&lcdc_0>;
> +			};
> +		};
>  	};
>  };

This is missing the output of tda998x. It should have two ports, input
and output, output going to hdmi-connector.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Russell King - ARM Linux]

On Mon, Mar 02, 2015 at 02:28:40PM +0200, Tomi Valkeinen wrote:
> On 26/02/15 16:55, Jyri Sarha wrote:
> > Use new binding for the external tda19988 HDMI encoder.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> >  arch/arm/boot/dts/am335x-boneblack.dts | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
> > index 5c42d25..eadbba3 100644
> > --- a/arch/arm/boot/dts/am335x-boneblack.dts
> > +++ b/arch/arm/boot/dts/am335x-boneblack.dts
> > @@ -68,16 +68,26 @@
> >  
> >  &lcdc {
> >  	status = "okay";
> > +	port {
> > +		lcdc_0: endpoint@0 {
> > +			remote-endpoint = <&hdmi_0>;
> > +		};
> > +	};
> >  };
> >  
> > -/ {
> > -	hdmi {
> > -		compatible = "ti,tilcdc,slave";
> > -		i2c = <&i2c0>;
> > +&i2c0 {
> > +	tda19988 {
> > +		compatible = "nxp,tda998x";
> > +		reg = <0x70>;
> >  		pinctrl-names = "default", "off";
> >  		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
> >  		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
> > -		status = "okay";
> > +
> > +		port {
> > +			hdmi_0: endpoint@0 {
> > +				remote-endpoint = <&lcdc_0>;
> > +			};
> > +		};
> >  	};
> >  };
> 
> This is missing the output of tda998x. It should have two ports, input
> and output, output going to hdmi-connector.

We don't have that kind of level of modelling in DRM right now - as far
as DRM is concerned, the tda998x is both the encoder _and_ the connector
since it supplies both functionalities.

We did discuss this ages ago, but afaik no concensus was reached how to
model physical connectors in DT, so it never moved forward in DRM (and
besides, everyone seems to be off doing their own thing when it comes
to writing DT descriptions for video hardware.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

On 02/03/15 18:06, Russell King - ARM Linux wrote:

>> This is missing the output of tda998x. It should have two ports, input
>> and output, output going to hdmi-connector.
> 
> We don't have that kind of level of modelling in DRM right now - as far
> as DRM is concerned, the tda998x is both the encoder _and_ the connector
> since it supplies both functionalities.

That's fine, but these are DT bindings which should reflect the
hardware, not the SW stack.

> We did discuss this ages ago, but afaik no concensus was reached how to
> model physical connectors in DT, so it never moved forward in DRM (and

I don't know about consensus, but omapdss is using connectors in DT, and
they were discussed in lists, and everyone seemed to be ok with that
model (Documentation/devicetree/bindings/video/hdmi-connector.txt). If I
recall right, the only real question was how the links should be modeled
(two way, one way, something else), but that's not specific to connectors.

So while it's open how they should be implemented in the DRM, I don't
see why we couldn't/shouldn't specify them in the .dts.

That said, if and when DRM supports connectors defined in .dts, we can
just assume that if tda998x does not have an output defined in the .dts,
it's connected to a HDMI connector. So we should do just fine even if we
end up not defining the connectors at this time.

> besides, everyone seems to be off doing their own thing when it comes
> to writing DT descriptions for video hardware.)

Yep... I've been trying to push the video ports/endpoints system which
afaik covers about all the use cases that have been raised. But the
counter argument usually is that "it's too complex".

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Russell King - ARM Linux]

On Mon, Mar 02, 2015 at 07:08:39PM +0200, Tomi Valkeinen wrote:
> On 02/03/15 18:06, Russell King - ARM Linux wrote:
> 
> >> This is missing the output of tda998x. It should have two ports, input
> >> and output, output going to hdmi-connector.
> > 
> > We don't have that kind of level of modelling in DRM right now - as far
> > as DRM is concerned, the tda998x is both the encoder _and_ the connector
> > since it supplies both functionalities.
> 
> That's fine, but these are DT bindings which should reflect the
> hardware, not the SW stack.

I still don't buy your argument.  The principle is right, but I think
you're taking it too far.

Look at ePAPR for a moment, and consider a serial port.  A serial UART
can be physically connected to a 9-pin or a 25-pin connector, which
may be male or female.  These details are not included in the DT
description.  Even when the serial port control signals are provided
by GPIOs rather than the UART, we don't model the connector - we
wrap the GPIOs directly into the UART driver.

Arguably, that's not following the hardware, it's following the software
representation - it's following the software representation of what a
serial port _should_ look like to a non-specific OS.

Consider an ethernet port.  You'll find the same thing applies - the
physical connector itself is not specified.

Yet, somehow, we're wanting to specify the physical connector for _all_
video devices?  I don't see why that should be mandatory when it's not
mandatory for other subsystems.

If we want to take this to the extreme, we should be specifying the
power connectors in DT and how they're wired up along with their
controls.  We don't though, we specify the control devices and
the function of those (eg, via a charger chip.)

To take this to the extreme, what about a device powered via PoE?
Should the PoE connector be modelled in DT?

When we say "DT should follow the hardware" we're not talking about
making DT be an alternative to the hardware's schematic.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 6/6] ARM: dts: am335x-boneblack: Use new binding for HDMI

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3123 bytes --]

On 02/03/15 19:42, Russell King - ARM Linux wrote:
> On Mon, Mar 02, 2015 at 07:08:39PM +0200, Tomi Valkeinen wrote:
>> On 02/03/15 18:06, Russell King - ARM Linux wrote:
>>
>>>> This is missing the output of tda998x. It should have two ports, input
>>>> and output, output going to hdmi-connector.
>>>
>>> We don't have that kind of level of modelling in DRM right now - as far
>>> as DRM is concerned, the tda998x is both the encoder _and_ the connector
>>> since it supplies both functionalities.
>>
>> That's fine, but these are DT bindings which should reflect the
>> hardware, not the SW stack.
> 
> I still don't buy your argument.  The principle is right, but I think
> you're taking it too far.

<snip>

> When we say "DT should follow the hardware" we're not talking about
> making DT be an alternative to the hardware's schematic.

I agree, and that's not what I'm suggesting. We should only model HW in
the DT when it makes sense, when it gives us something.

A HDMI connector can have (at least) two functionalities that the driver
for it may need to handle: HPD and +5V. On some SoCs/boards those are
handled by the HDMI encoder, but I have boards where they are not. So we
need to have the data somewhere, and a HDMI connector node at the end of
the video path is a logical choice.

The HDMI connector node is also a good place for a symbolic name which
can be shown to the user.

In this particular board, the HDMI encoder handles the HPD and the +5V
is always enabled, so there's no strict need to have the HDMI connector
node.

However, I still feel it's better to have the HDMI connector in .dts:

1) It's not said that a HDMI encoder always has a HDMI connector as the
next component. The next component could be a integrated panel, needing
a specific driver and there's no HDMI connector at all. Or there could
be something else, as is the case on some OMAP boards which have an ESD
protection chip (that requires controlling, i.e. a driver) between the
encoder and the connector.

2) I like that the beginning and the end of the video pipeline are
clearly defined. A video pipeline starts at a display controller, and
ends at a panel or a connector. This makes it easier to understand the
.dts as you know what to expect.

In the SW side these mean that every encoder (or whatever is doing this
stuff) should be able to handle any component after the encoder, be it a
connector, panel or something else.

If we allow leaving out the connector node, the code also needs to
handle the case when there's nothing after the encoder, which probably
means fabricating some connector data (at least if and when DRM can
handle arbitrary video pipelines).

But as I said earlier, we can do just fine without the HDMI connector
node on boards where the connector "just works". We can handle that in
the drivers with some extra code.

So if people think it's a big chore to add the connectors and don't see
the benefit in them (and they don't want the symbolic labels that could
be added there), I'm fine with having them optional.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 0/6] Use DRM component API in tilcdc to connect to tda998x

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

Hi Rob,

You weren't cc'd to this series. I don't know if you're still interested
in tilcdc, but if you are, any comments appreciated =).

 Tomi

On 26/02/15 16:55, Jyri Sarha wrote:
> Remove tilcdc slave support and connect to tda998x trough its
> component DRM API. For dtb backward compatibility the code creates at
> boot time a DT overlay based on the earlier binding. The overlay
> conforms to the new graph based binding.
> 
> The first patch is just a bugfix and can be applied or dropped
> independenty.
> 
> Jyri Sarha (6):
>   drm/tilcdc: Fix module unloading
>   drm/tilcdc: Remove tilcdc slave support for tda998x driver
>   drm/tilcdc: Add support for external compontised DRM encoder
>   drm/tilcdc: Add DRM_TILCDC_INIT for "ti,tilcdc,slave" binding support
>   drm/tilcdc: Force building of DRM_TILCDC_INIT
>   ARM: dts: am335x-boneblack: Use new binding for HDMI
> 
>  .../devicetree/bindings/drm/tilcdc/slave.txt       |  18 -
>  .../devicetree/bindings/drm/tilcdc/tilcdc.txt      |  27 ++
>  arch/arm/boot/dts/am335x-boneblack.dts             |  20 +-
>  drivers/gpu/drm/Makefile                           |   1 +
>  drivers/gpu/drm/tilcdc/Kconfig                     |   6 +
>  drivers/gpu/drm/tilcdc/Makefile                    |   4 +-
>  drivers/gpu/drm/tilcdc/tilcdc_boot_init.c          | 270 ++++++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  36 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  69 ++--
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   3 +-
>  drivers/gpu/drm/tilcdc/tilcdc_external.c           | 105 ++++++
>  drivers/gpu/drm/tilcdc/tilcdc_external.h           |  24 ++
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c              | 411 ---------------------
>  drivers/gpu/drm/tilcdc/tilcdc_slave.h              |  26 --
>  drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts    |  70 ++++
>  15 files changed, 601 insertions(+), 489 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_boot_init.c
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.c
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.h
>  delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
>  delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave_convert.dts
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]