On Tue, 17 Apr 2018 15:10:51 +0200
Peter Rosin <p...@axentia.se> wrote:
> When the of-graph points to a tda998x-compatible HDMI encoder, register
> as a component master and bind to the encoder/connector provided by
> the tda998x driver.
Can't we do the opposite: make the tda998x driver expose its devices as
drm bridges. I'd rather not add another way to connect external
encoders (or bridges) to display controller drivers, especially since,
when I asked DRM maintainers/devs what was the good approach to
represent such external encoders they pointed me to the drm_bridge
interface.
>
> Signed-off-by: Peter Rosin <p...@axentia.se>
> ---
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 81 ++++++++++++--
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 15 +++
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 130
> +++++++++++++++++++++++
> 3 files changed, 220 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index c1ea5c36b006..8523c40fac94 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -20,6 +20,7 @@
> */
>
> #include <linux/clk.h>
> +#include <linux/component.h>
> #include <linux/irq.h>
> #include <linux/irqchip.h>
> #include <linux/module.h>
> @@ -568,10 +569,13 @@ static int atmel_hlcdc_dc_modeset_init(struct
> drm_device *dev)
>
> drm_mode_config_init(dev);
>
> - ret = atmel_hlcdc_create_outputs(dev);
> - if (ret) {
> - dev_err(dev->dev, "failed to create HLCDC outputs: %d\n", ret);
> - return ret;
> + if (!dc->is_componentized) {
> + ret = atmel_hlcdc_create_outputs(dev);
> + if (ret) {
> + dev_err(dev->dev,
> + "failed to create HLCDC outputs: %d\n", ret);
> + return ret;
> + }
> }
>
> ret = atmel_hlcdc_create_planes(dev);
> @@ -586,6 +590,16 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device
> *dev)
> return ret;
> }
>
> + if (dc->is_componentized) {
> + ret = component_bind_all(dev->dev, dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = atmel_hlcdc_add_component_encoder(dev);
> + if (ret < 0)
> + return ret;
> + }
> +
> dev->mode_config.min_width = dc->desc->min_width;
> dev->mode_config.min_height = dc->desc->min_height;
> dev->mode_config.max_width = dc->desc->max_width;
> @@ -617,6 +631,9 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
> if (!dc)
> return -ENOMEM;
>
> + dc->is_componentized =
> + atmel_hlcdc_get_external_components(dev->dev, NULL) > 0;
> +
> dc->wq = alloc_ordered_workqueue("atmel-hlcdc-dc", 0);
> if (!dc->wq)
> return -ENOMEM;
> @@ -751,7 +768,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
> .minor = 0,
> };
>
> -static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> +static int atmel_hlcdc_dc_drm_init(struct platform_device *pdev)
> {
> struct drm_device *ddev;
> int ret;
> @@ -779,7 +796,7 @@ static int atmel_hlcdc_dc_drm_probe(struct
> platform_device *pdev)
> return ret;
> }
>
> -static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> +static int atmel_hlcdc_dc_drm_fini(struct platform_device *pdev)
> {
> struct drm_device *ddev = platform_get_drvdata(pdev);
>
> @@ -790,6 +807,58 @@ static int atmel_hlcdc_dc_drm_remove(struct
> platform_device *pdev)
> return 0;
> }
>
> +static int atmel_hlcdc_bind(struct device *dev)
> +{
> + return atmel_hlcdc_dc_drm_init(to_platform_device(dev));
> +}
> +
> +static void atmel_hlcdc_unbind(struct device *dev)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> +
> + /* Check if a subcomponent has already triggered the unloading. */
> + if (!ddev->dev_private)
> + return;
> +
> + atmel_hlcdc_dc_drm_fini(to_platform_device(dev));
> +}
> +
> +static const struct component_master_ops atmel_hlcdc_comp_ops = {
> + .bind = atmel_hlcdc_bind,
> + .unbind = atmel_hlcdc_unbind,
> +};
> +
> +static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> +{
> + struct component_match *match = NULL;
> + int ret;
> +
> + ret = atmel_hlcdc_get_external_components(&pdev->dev, &match);
> + if (ret < 0)
> + return ret;
> + else if (ret)
> + return component_master_add_with_match(&pdev->dev,
> + &atmel_hlcdc_comp_ops,
> + match);
> + else
> + return atmel_hlcdc_dc_drm_init(pdev);
> +}
> +
> +static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = atmel_hlcdc_get_external_components(&pdev->dev, NULL);
> + if (ret < 0)
> + return ret;
> + else if (ret)
> + component_master_del(&pdev->dev, &atmel_hlcdc_comp_ops);
> + else
> + atmel_hlcdc_dc_drm_fini(pdev);
> +
> + return 0;
> +}
> +
> #ifdef CONFIG_PM_SLEEP
> static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
> {
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index ab32d5b268d2..cae77c245661 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -370,6 +370,10 @@ struct atmel_hlcdc_plane_properties {
> * @wq: display controller workqueue
> * @suspend: used to store the HLCDC state when entering suspend
> * @commit: used for async commit handling
> + * @external_encoder: used encoder when componentized
> + * @external_connector: used connector when componentized
> + * @connector_funcs: original helper funcs of the external connector
> + * @is_componentized: operating mode
> */
> struct atmel_hlcdc_dc {
> const struct atmel_hlcdc_dc_desc *desc;
> @@ -386,6 +390,12 @@ struct atmel_hlcdc_dc {
> wait_queue_head_t wait;
> bool pending;
> } commit;
> +
> + struct drm_encoder *external_encoder;
> + struct drm_connector *external_connector;
> + const struct drm_connector_helper_funcs *connector_funcs;
> +
> + bool is_componentized;
> };
>
> extern struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_formats;
> @@ -455,4 +465,9 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev);
>
> int atmel_hlcdc_create_outputs(struct drm_device *dev);
>
> +struct component_match;
> +int atmel_hlcdc_add_component_encoder(struct drm_device *dev);
> +int atmel_hlcdc_get_external_components(struct device *dev,
> + struct component_match **match);
> +
> #endif /* DRM_ATMEL_HLCDC_H */
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 8db51fb131db..3f86527e0473 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -6,6 +6,11 @@
> * Author: Jean-Jacques Hiblot <jjhib...@traphandler.com>
> * Author: Boris BREZILLON <boris.brezil...@free-electrons.com>
> *
> + * Handling of external components adapted from the tilcdc driver
> + * by Peter Rosin <p...@axentia.se>. That original code had:
> + * Copyright (C) 2015 Texas Instruments
> + * Author: Jyri Sarha <jsa...@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.
> @@ -19,6 +24,7 @@
> * this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <linux/component.h>
> #include <linux/of_graph.h>
>
> #include <drm/drmP.h>
> @@ -88,3 +94,127 @@ int atmel_hlcdc_create_outputs(struct drm_device *dev)
>
> return ret;
> }
> +
> +static int atmel_hlcdc_external_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct atmel_hlcdc_dc *dc = connector->dev->dev_private;
> + int ret;
> +
> + ret = atmel_hlcdc_dc_mode_valid(dc, mode);
> + if (ret != MODE_OK)
> + return ret;
> +
> + if (WARN_ON(dc->external_connector != connector))
> + return MODE_ERROR;
> + if (WARN_ON(!dc->connector_funcs))
> + return MODE_ERROR;
> +
> + if (IS_ERR(dc->connector_funcs) || !dc->connector_funcs->mode_valid)
> + return MODE_OK;
> +
> + /* The connector has its own mode_valid, call it. */
> + return dc->connector_funcs->mode_valid(connector, mode);
> +}
> +
> +static int atmel_hlcdc_add_external_connector(struct drm_device *dev,
> + struct drm_connector *connector)
> +{
> + struct atmel_hlcdc_dc *dc = dev->dev_private;
> + struct drm_connector_helper_funcs *connector_funcs;
> +
> + /* There should never be more than one connector. */
> + if (WARN_ON(dc->external_connector))
> + return -EINVAL;
> +
> + dc->external_connector = connector;
> + connector_funcs = devm_kzalloc(dev->dev, sizeof(*connector_funcs),
> + GFP_KERNEL);
> + if (!connector_funcs)
> + return -ENOMEM;
> +
> + /*
> + * connector->helper_private always contains the struct
> + * connector_helper_funcs pointer. For the atmel-hlcdc crtc
> + * to have a say if a specific mode is Ok, we install our
> + * own helper functions. In our helper functions we copy
> + * everything else but use our own mode_valid() (above).
> + */
> + if (connector->helper_private) {
> + dc->connector_funcs = connector->helper_private;
> + *connector_funcs = *dc->connector_funcs;
> + } else {
> + dc->connector_funcs = ERR_PTR(-ENOENT);
> + }
> + connector_funcs->mode_valid = atmel_hlcdc_external_mode_valid;
> + drm_connector_helper_add(connector, connector_funcs);
> +
> + dev_dbg(dev->dev, "External connector '%s' connected\n",
> + connector->name);
> +
> + return 0;
> +}
> +
> +static struct drm_connector *
> +atmel_hlcdc_encoder_find_connector(struct drm_device *dev,
> + struct drm_encoder *encoder)
> +{
> + struct drm_connector *connector;
> + int i;
> +
> + list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> + if (connector->encoder_ids[i] == encoder->base.id)
> + return connector;
> +
> + dev_err(dev->dev, "No connector found for %s encoder (id %d)\n",
> + encoder->name, encoder->base.id);
> +
> + return NULL;
> +}
> +
> +int atmel_hlcdc_add_component_encoder(struct drm_device *dev)
> +{
> + struct atmel_hlcdc_dc *dc = dev->dev_private;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> +
> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> + if (encoder->possible_crtcs & (1 << dc->crtc->index))
> + break;
> + }
> +
> + if (!encoder) {
> + dev_err(dev->dev, "%s: No suitable encoder found\n", __func__);
> + return -ENODEV;
> + }
> +
> + connector = atmel_hlcdc_encoder_find_connector(dev, encoder);
> + if (!connector)
> + return -ENODEV;
> +
> + return atmel_hlcdc_add_external_connector(dev, connector);
> +}
> +
> +static int dev_match_of(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +int atmel_hlcdc_get_external_components(struct device *dev,
> + struct component_match **match)
> +{
> + struct device_node *node;
> +
> + node = of_graph_get_remote_node(dev->of_node, 0, 0);
> +
> + if (!of_device_is_compatible(node, "nxp,tda998x")) {
> + of_node_put(node);
> + return 0;
> + }
> +
> + if (match)
> + drm_of_component_match_add(dev, match, dev_match_of, node);
> + of_node_put(node);
> + return 1;
> +}
