Re: [PATCH 1/8] drm: bridge: Add LVDS encoder driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Archit,

On Friday 21 Oct 2016 10:51:59 Archit Taneja wrote:
> On 10/19/2016 07:55 PM, Laurent Pinchart wrote:
> > The LVDS encoder driver is a DRM bridge driver that supports the
> > parallel to LVDS encoders that don't require any configuration. The
> > driver thus doesn't interact with the device, but creates an LVDS
> > connector for the panel and exposes its size and timing based on
> > information retrieved from DT.
> > 
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../bindings/display/bridge/lvds-transmitter.txt   |  64 +++++++
> >  drivers/gpu/drm/bridge/Kconfig                     |   8 +
> >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >  drivers/gpu/drm/bridge/lvds-encoder.c              | 203 ++++++++++++++++
> >  4 files changed, 276 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > new file mode 100644
> > index 000000000000..fd39ad34c383
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > @@ -0,0 +1,64 @@
> > +Parallel to LVDS Encoder
> > +------------------------
> > +
> > +This binding supports the parallel to LVDS encoders that don't require
> > any
> > +configuration.
> > +
> > +LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A.
> > Multiple +incompatible data link layers have been used over time to
> > transmit image data +to LVDS panels. This binding targets devices
> > compatible with the following +specifications only.
> > +
> > +[JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999,
> > February
> > +1999 (Version 1.0), Japan Electronic Industry Development Association
> > (JEIDA) +[LDI] "Open LVDS Display Interface", May 1999 (Version 0.95),
> > National +Semiconductor
> > +[VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > +Electronics Standards Association (VESA)
> > +
> > +Those devices have been marketed under the FPD-Link and FlatLink brand
> > names +among others.
> > +
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "lvds-encoder"
> > +
> > +Required nodes:
> > +
> > +This device has two video ports. Their connections are modeled using the
> > OF +graph bindings specified in
> > Documentation/devicetree/bindings/graph.txt. +
> > +- Video port 0 for parallel input
> > +- Video port 1 for LVDS output
> > +
> > +
> > +Example
> > +-------
> > +
> > +lvds-encoder {
> > +	compatible = "lvds-encoder";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +
> > +			lvds_enc_in: endpoint {
> > +				remote-endpoint = <&display_out_rgb>;
> > +			};
> > +		};
> > +
> > +		port@1 {
> > +			reg = <1>;
> > +
> > +			lvds_enc_out: endpoint {
> > +				remote-endpoint = <&lvds_panel_in>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/drivers/gpu/drm/bridge/Kconfig
> > b/drivers/gpu/drm/bridge/Kconfig index 10e12e74fc9f..5dafad7817ad 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -24,6 +24,14 @@ config DRM_DUMB_VGA_DAC
> > 
> >  	help
> >  	
> >  	  Support for RGB to VGA DAC based bridges
> > 
> > +config DRM_LVDS_ENCODER
> > +	tristate "Transparent parallel to LVDS encoder support"
> > +	depends on OF
> > +	select DRM_KMS_HELPER
> > +	help
> > +	  Support for transparent parallel to LVDS encoders that don't require
> > +	  any configuration.
> > +
> > 
> >  config DRM_DW_HDMI
> >  
> >  	tristate
> >  	select DRM_KMS_HELPER
> > 
> > diff --git a/drivers/gpu/drm/bridge/Makefile
> > b/drivers/gpu/drm/bridge/Makefile index cdf3a3cf765d..bbaf583581ac 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -2,6 +2,7 @@ ccflags-y := -Iinclude/drm
> > 
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > 
> > +obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > 
> >  obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
> >  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
> >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> > 
> > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
> > b/drivers/gpu/drm/bridge/lvds-encoder.c new file mode 100644
> > index 000000000000..33e8025c8a6d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> > @@ -0,0 +1,203 @@
> > +/*
> > + * Copyright (C) 2016 Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_encoder.h>
> > +#include <drm/drm_modeset_helper_vtables.h>
> > +
> > +#include <linux/of_graph.h>
> > +
> > +#include <video/display_timing.h>
> > +#include <video/of_display_timing.h>
> > +#include <video/videomode.h>
> > +
> > +struct lvds_encoder {
> > +	struct device *dev;
> > +
> > +	struct drm_bridge bridge;
> > +	struct drm_connector connector;
> > +
> > +	struct {
> > +		unsigned int width_mm;
> > +		unsigned int height_mm;
> > +		struct videomode mode;
> > +	} panel;
> 
> I don't see why we need create our own panel instance
> in the encoder driver. We should use the drm_panel
> API here rather than managing panel specific stuff in
> this driver.

It's not a panel instance but a structure that holds information about the 
connected panel. This is temporary until the LVDS panel DT bindings I've 
submitted get accepted.

> The parade-ps8622 and nxp-ptn3460 bridge drivers already
> do this.
> 
> Thanks,
> Archit
> 
> > +};
> > +
> > +static inline struct lvds_encoder *
> > +drm_bridge_to_lvds_encoder(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct lvds_encoder, bridge);
> > +}
> > +
> > +static inline struct lvds_encoder *
> > +drm_connector_to_lvds_encoder(struct drm_connector *connector)
> > +{
> > +	return container_of(connector, struct lvds_encoder, connector);
> > +}
> > +
> > +static int lvds_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector);
> > +	struct drm_display_mode *mode;
> > +
> > +	mode = drm_mode_create(connector->dev);
> > +	if (mode == NULL)
> > +		return 0;
> > +
> > +	mode->type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER;
> > +
> > +	drm_display_mode_from_videomode(&lvds->panel.mode, mode);
> > +
> > +	drm_mode_probed_add(connector, mode);
> > +
> > +	return 1;
> > +}
> > +
> > +static const struct drm_connector_helper_funcs
> > lvds_connector_helper_funcs = { +	.get_modes = lvds_connector_get_modes,
> > +};
> > +
> > +static enum drm_connector_status
> > +lvds_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +	return connector_status_connected;
> > +}
> > +
> > +static const struct drm_connector_funcs lvds_connector_funcs = {
> > +	.dpms = drm_atomic_helper_connector_dpms,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.detect = lvds_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int lvds_encoder_attach(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
> > +	struct drm_connector *connector = &lvds->connector;
> > +	int ret;
> > +
> > +	if (!bridge->encoder) {
> > +		DRM_ERROR("Missing encoder\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	connector->display_info.width_mm = lvds->panel.width_mm;
> > +	connector->display_info.height_mm = lvds->panel.height_mm;
> > +
> > +	drm_connector_helper_add(connector, &lvds_connector_helper_funcs);
> > +
> > +	ret = drm_connector_init(bridge->dev, connector, 
&lvds_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_LVDS);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize connector\n");
> > +		return ret;
> > +	}
> > +
> > +	drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = {
> > +	.attach = lvds_encoder_attach,
> > +};
> > +
> > +static int lvds_encoder_probe(struct platform_device *pdev)
> > +{
> > +	struct lvds_encoder *lvds;
> > +	struct display_timing timing;
> > +	struct device_node *port;
> > +	struct device_node *endpoint;
> > +	struct device_node *panel;
> > +	int ret;
> > +
> > +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> > +	if (!lvds)
> > +		return -ENOMEM;
> > +
> > +	lvds->dev = &pdev->dev;
> > +	platform_set_drvdata(pdev, lvds);
> > +
> > +	lvds->bridge.funcs = &lvds_encoder_bridge_funcs;
> > +	lvds->bridge.of_node = pdev->dev.of_node;
> > +	lvds->bridge.encoder_type = DRM_MODE_ENCODER_LVDS;
> > +
> > +	/* Locate the panel DT node. */
> > +	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
> > +	if (!port) {
> > +		dev_dbg(&pdev->dev, "port 1 not found\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	endpoint = of_get_child_by_name(port, "endpoint");
> > +	of_node_put(port);
> > +	if (!endpoint) {
> > +		dev_dbg(&pdev->dev, "no endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	panel = of_graph_get_remote_port_parent(endpoint);
> > +	of_node_put(endpoint);
> > +	if (!panel) {
> > +		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	/* Parse and store panel information. */
> > +	ret = of_get_display_timing(panel, "panel-timing", &timing);
> > +	of_node_put(panel);
> > +	if (ret < 0) {
> > +		dev_dbg(&pdev->dev, "unable to parse panel timings\n");
> > +		return ret;
> > +	}
> > +
> > +	videomode_from_timing(&timing, &lvds->panel.mode);
> > +
> > +	of_property_read_u32(panel, "width-mm", &lvds->panel.width_mm);
> > +	of_property_read_u32(panel, "height-mm", &lvds->panel.height_mm);
> > +
> > +	/* Register the bridge. */
> > +	return drm_bridge_add(&lvds->bridge);
> > +}
> > +
> > +static int lvds_encoder_remove(struct platform_device *pdev)
> > +{
> > +	struct lvds_encoder *encoder = platform_get_drvdata(pdev);
> > +
> > +	drm_bridge_remove(&encoder->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id lvds_encoder_match[] = {
> > +	{ .compatible = "lvds-encoder" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> > +
> > +struct platform_driver lvds_encoder_driver = {
> > +	.probe	= lvds_encoder_probe,
> > +	.remove	= lvds_encoder_remove,
> > +	.driver		= {
> > +		.name		= "lvds-encoder",
> > +		.of_match_table	= lvds_encoder_match,
> > +	},
> > +};
> > +module_platform_driver(lvds_encoder_driver);
> > +
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> > +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart