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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- 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 --]