Re: [PATCH 3/4] drm/tilcdc: add encoder slave

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 22, 2013 at 04:36:24PM -0600, Rob Clark wrote:
> Add output panel driver for i2c encoder slaves.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

A few questions below, otherwise

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/tilcdc/Kconfig        |  12 ++
>  drivers/gpu/drm/tilcdc/Makefile       |   1 +
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c   |   5 +-
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c | 380 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_slave.h |  26 +++
>  5 files changed, 423 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
>  create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h
> 
> diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
> index ee9b592..99beca2 100644
> --- a/drivers/gpu/drm/tilcdc/Kconfig
> +++ b/drivers/gpu/drm/tilcdc/Kconfig
> @@ -8,3 +8,15 @@ config DRM_TILCDC
>  	  Choose this option if you have an TI SoC with LCDC display
>  	  controller, for example AM33xx in beagle-bone, DA8xx, or
>  	  OMAP-L1xx.  This driver replaces the FB_DA8XX fbdev driver.
> +
> +menu "I2C encoder or helper chips"
> +	depends on DRM && DRM_KMS_HELPER && I2C
> +
> +config DRM_I2C_NXP_TDA998X
> +	tristate "NXP Semiconductors TDA998X HDMI encoder"
> +	default m if DRM_TILCDC
> +	help
> +	  Support for NXP Semiconductors TDA998X HDMI encoders.
> +
> +endmenu
> +

Shouldn't that hunk be in patch 2?

> diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
> index 1359cc2..aa9097e 100644
> --- a/drivers/gpu/drm/tilcdc/Makefile
> +++ b/drivers/gpu/drm/tilcdc/Makefile
> @@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm -Werror
>  tilcdc-y := \
>  	tilcdc_crtc.o \
>  	tilcdc_tfp410.o \
> +	tilcdc_slave.o \
>  	tilcdc_drv.o
>  
>  obj-$(CONFIG_DRM_TILCDC)	+= tilcdc.o
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index cf1fddc..ca76dbe 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -20,6 +20,7 @@
>  #include "tilcdc_drv.h"
>  #include "tilcdc_regs.h"
>  #include "tilcdc_tfp410.h"
> +#include "tilcdc_slave.h"
>  
>  #include "drm_fb_helper.h"
>  
> @@ -587,6 +588,7 @@ static int __init tilcdc_drm_init(void)
>  {
>  	DBG("init");
>  	tilcdc_tfp410_init();
> +	tilcdc_slave_init();
>  	return platform_driver_register(&tilcdc_platform_driver);
>  }
>  
> @@ -594,10 +596,11 @@ static void __exit tilcdc_drm_fini(void)
>  {
>  	DBG("fini");
>  	tilcdc_tfp410_fini();
> +	tilcdc_slave_fini();
>  	platform_driver_unregister(&tilcdc_platform_driver);
>  }
>  
> -module_init(tilcdc_drm_init);
> +late_initcall(tilcdc_drm_init);
>  module_exit(tilcdc_drm_fini);
>  
>  MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> new file mode 100644
> index 0000000..b6f3e63
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
> @@ -0,0 +1,380 @@
> +/*
> + * 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/of_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 = {
> +		.min_bpp                = 16,
> +		.max_bpp                = 16,
> +		.bpp                    = 16,
> +		.ac_bias                = 255,
> +		.ac_bias_intrpt         = 0,
> +		.dma_burst_sz           = 16,
> +		.fdd                    = 0x80,
> +		.tft_alt_mode           = 0,
> +		.stn_565_mode           = 0,
> +		.mono_8bit_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)

Since you have a 1:1:1 relationship between module/drm_encoder, the
drm_encoder_slave and the connector I'd just smash this all into one
struct. Pure bikeshed though ;-)

> +
> +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 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     = drm_i2c_encoder_mode_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_LVDS);

DRM_MODE_ENCODER_TMDS? Although I guess adding a new kind of
multi-function encoder type would make more sense and also useful in other
places. E.g. i915-sdvo/dvo just set meaningless types for multi-function
encoders (i.e. more than one connector on the same output), namely the
type which matches the connector last initalized.

> +	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_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);

Shouldn't we get the connector type from the drm_encoder_slave somehow?
Works here for now, so just food for thought for future encoder slave
refactorings and infrastructure work.

> +	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_sysfs_connector_add(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 void slave_destroy(struct tilcdc_module *mod)
> +{
> +	struct slave_module *slave_mod = to_slave_module(mod);
> +
> +	tilcdc_module_cleanup(mod);
> +	kfree(slave_mod);
> +}
> +
> +static const struct tilcdc_module_ops slave_module_ops = {
> +		.modeset_init = slave_modeset_init,
> +		.destroy = slave_destroy,
> +};
> +
> +/*
> + * 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;
> +	int ret = -EINVAL;
> +
> +	/* bail out early if no DT data: */
> +	if (!node) {
> +		dev_err(&pdev->dev, "device-tree data is missing\n");
> +		return -ENXIO;
> +	}
> +
> +	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
> +	if (!slave_mod)
> +		return -ENOMEM;
> +
> +	mod = &slave_mod->base;
> +
> +	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");
> +
> +	if (of_property_read_u32(node, "i2c", &i2c_phandle)) {
> +		dev_err(&pdev->dev, "could not get i2c bus phandle\n");
> +		goto fail;
> +	}
> +
> +	i2c_node = of_find_node_by_phandle(i2c_phandle);
> +	if (!i2c_node) {
> +		dev_err(&pdev->dev, "could not get i2c bus node\n");
> +		goto fail;
> +	}
> +
> +	slave_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
> +	if (!slave_mod->i2c) {
> +		dev_err(&pdev->dev, "could not get i2c\n");
> +		goto fail;
> +	}
> +
> +	of_node_put(i2c_node);
> +
> +	return 0;
> +
> +fail:
> +	slave_destroy(mod);
> +	return ret;
> +}
> +
> +static int slave_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static struct of_device_id slave_of_match[] = {
> +		{ .compatible = "tilcdc,slave", },
> +		{ },
> +};
> +MODULE_DEVICE_TABLE(of, slave_of_match);
> +
> +struct platform_driver slave_driver = {
> +	.probe = slave_probe,
> +	.remove = slave_remove,
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "slave",
> +		.of_match_table = slave_of_match,
> +	},
> +};

No idea how this devicetree matching stuff is supposed to work, but I
guess this needs to be updated in the devictree docs like the base 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
> new file mode 100644
> index 0000000..2f85048
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch