Re: [PATCH 1/4] drm/bridge: Repurpose lvds-encoder.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 04/11/2019 11:42, Fabrizio Castro wrote:
> Hi Neil,
> 
> Thank you for your feedback!
> 
>> From: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> Sent: 04 November 2019 09:18
>> Subject: Re: [PATCH 1/4] drm/bridge: Repurpose lvds-encoder.c
>>
>> Hi,
>>
>> On 30/10/2019 14:43, Fabrizio Castro wrote:
>>> lvds-encoder.c implementation is also suitable for LVDS decoders,
>>> not just LVDS encoders.
>>> Instead of creating a new driver for addressing support for
>>> transparent LVDS decoders, repurpose lvds-encoder.c for the greater
>>> good.
>>>
>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig        |   8 +-
>>>  drivers/gpu/drm/bridge/Makefile       |   2 +-
>>>  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
>>>  4 files changed, 174 insertions(+), 160 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
>>>  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3436297..9e75ca4e 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
>>>  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
>>>  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
>>>
>>> -config DRM_LVDS_ENCODER
>>> -	tristate "Transparent parallel to LVDS encoder support"
>>> +config DRM_LVDS_CODEC
>>
>>
>> I'm not CCed on other patches, but I'm pretty sure you should fix other
>> Kconfig and defconfigs selecting this config in this patch to avoid breaking bisect.
> 
> My understanding is that no defconfig and no other option refer directly to DRM_LVDS_ENCODER, do you mind being a little bit more specific here?
> 

Indeed, you are right.

Neil

> Thanks,
> Fab
> 
>>
>> Neil
>>
>>> +	tristate "Transparent LVDS encoders and decoders support"
>>>  	depends on OF
>>>  	select DRM_KMS_HELPER
>>>  	select DRM_PANEL_BRIDGE
>>>  	help
>>> -	  Support for transparent parallel to LVDS encoders that don't require
>>> -	  any configuration.
>>> +	  Support for transparent LVDS encoders and LVDS decoders that don't
>>> +	  require any configuration.
>>>
>>>  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>>>  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>>> index 4934fcf..8a9178a 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -2,7 +2,7 @@
>>>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>>>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>>>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>>> -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>>> +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>>>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>>>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
>>> new file mode 100644
>>> index 0000000..8a1979c
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>>> @@ -0,0 +1,169 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> + */
>>> +
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_graph.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include <drm/drm_bridge.h>
>>> +#include <drm/drm_panel.h>
>>> +
>>> +struct lvds_codec {
>>> +	struct drm_bridge bridge;
>>> +	struct drm_bridge *panel_bridge;
>>> +	struct gpio_desc *powerdown_gpio;
>>> +	u32 connector_type;
>>> +};
>>> +
>>> +static int lvds_codec_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct lvds_codec *lvds_codec = container_of(bridge,
>>> +							 struct lvds_codec,
>>> +							 bridge);
>>> +
>>> +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
>>> +				 bridge);
>>> +}
>>> +
>>> +static void lvds_codec_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct lvds_codec *lvds_codec = container_of(bridge,
>>> +							 struct lvds_codec,
>>> +							 bridge);
>>> +
>>> +	if (lvds_codec->powerdown_gpio)
>>> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
>>> +}
>>> +
>>> +static void lvds_codec_disable(struct drm_bridge *bridge)
>>> +{
>>> +	struct lvds_codec *lvds_codec = container_of(bridge,
>>> +							 struct lvds_codec,
>>> +							 bridge);
>>> +
>>> +	if (lvds_codec->powerdown_gpio)
>>> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
>>> +}
>>> +
>>> +static struct drm_bridge_funcs funcs = {
>>> +	.attach = lvds_codec_attach,
>>> +	.enable = lvds_codec_enable,
>>> +	.disable = lvds_codec_disable,
>>> +};
>>> +
>>> +static int lvds_codec_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *port;
>>> +	struct device_node *endpoint;
>>> +	struct device_node *panel_node;
>>> +	struct drm_panel *panel;
>>> +	struct lvds_codec *lvds_codec;
>>> +
>>> +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
>>> +	if (!lvds_codec)
>>> +		return -ENOMEM;
>>> +
>>> +	lvds_codec->connector_type = (u32)
>>> +		of_device_get_match_data(&pdev->dev);
>>> +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>>> +							       GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
>>> +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
>>> +
>>> +		if (err != -EPROBE_DEFER)
>>> +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
>>> +		return err;
>>> +	}
>>> +
>>> +	/* Locate the panel DT node. */
>>> +	port = of_graph_get_port_by_id(dev->of_node, 1);
>>> +	if (!port) {
>>> +		dev_dbg(dev, "port 1 not found\n");
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	endpoint = of_get_child_by_name(port, "endpoint");
>>> +	of_node_put(port);
>>> +	if (!endpoint) {
>>> +		dev_dbg(dev, "no endpoint for port 1\n");
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	panel_node = of_graph_get_remote_port_parent(endpoint);
>>> +	of_node_put(endpoint);
>>> +	if (!panel_node) {
>>> +		dev_dbg(dev, "no remote endpoint for port 1\n");
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	panel = of_drm_find_panel(panel_node);
>>> +	of_node_put(panel_node);
>>> +	if (IS_ERR(panel)) {
>>> +		dev_dbg(dev, "panel not found, deferring probe\n");
>>> +		return PTR_ERR(panel);
>>> +	}
>>> +
>>> +	lvds_codec->panel_bridge =
>>> +		devm_drm_panel_bridge_add_typed(dev, panel,
>>> +						lvds_codec->connector_type);
>>> +	if (IS_ERR(lvds_codec->panel_bridge))
>>> +		return PTR_ERR(lvds_codec->panel_bridge);
>>> +
>>> +	/* The panel_bridge bridge is attached to the panel's of_node,
>>> +	 * but we need a bridge attached to our of_node for our user
>>> +	 * to look up.
>>> +	 */
>>> +	lvds_codec->bridge.of_node = dev->of_node;
>>> +	lvds_codec->bridge.funcs = &funcs;
>>> +	drm_bridge_add(&lvds_codec->bridge);
>>> +
>>> +	platform_set_drvdata(pdev, lvds_codec);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int lvds_codec_remove(struct platform_device *pdev)
>>> +{
>>> +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
>>> +
>>> +	drm_bridge_remove(&lvds_codec->bridge);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id lvds_codec_match[] = {
>>> +	{
>>> +		.compatible = "lvds-encoder",
>>> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS
>>> +	},
>>> +	{
>>> +		.compatible = "thine,thc63lvdm83d",
>>> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
>>> +	},
>>> +	{
>>> +		.compatible = "lvds-decoder",
>>> +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
>>> +	},
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, lvds_codec_match);
>>> +
>>> +static struct platform_driver lvds_codec_driver = {
>>> +	.probe	= lvds_codec_probe,
>>> +	.remove	= lvds_codec_remove,
>>> +	.driver		= {
>>> +		.name		= "lvds-codec",
>>> +		.of_match_table	= lvds_codec_match,
>>> +	},
>>> +};
>>> +module_platform_driver(lvds_codec_driver);
>>> +
>>> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
>>> deleted file mode 100644
>>> index e2132a8..0000000
>>> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
>>> +++ /dev/null
>>> @@ -1,155 +0,0 @@
>>> -// SPDX-License-Identifier: GPL-2.0-or-later
>>> -/*
>>> - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> - */
>>> -
>>> -#include <linux/gpio/consumer.h>
>>> -#include <linux/module.h>
>>> -#include <linux/of.h>
>>> -#include <linux/of_graph.h>
>>> -#include <linux/platform_device.h>
>>> -
>>> -#include <drm/drm_bridge.h>
>>> -#include <drm/drm_panel.h>
>>> -
>>> -struct lvds_encoder {
>>> -	struct drm_bridge bridge;
>>> -	struct drm_bridge *panel_bridge;
>>> -	struct gpio_desc *powerdown_gpio;
>>> -};
>>> -
>>> -static int lvds_encoder_attach(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
>>> -							 struct lvds_encoder,
>>> -							 bridge);
>>> -
>>> -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
>>> -				 bridge);
>>> -}
>>> -
>>> -static void lvds_encoder_enable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
>>> -							 struct lvds_encoder,
>>> -							 bridge);
>>> -
>>> -	if (lvds_encoder->powerdown_gpio)
>>> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
>>> -}
>>> -
>>> -static void lvds_encoder_disable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
>>> -							 struct lvds_encoder,
>>> -							 bridge);
>>> -
>>> -	if (lvds_encoder->powerdown_gpio)
>>> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
>>> -}
>>> -
>>> -static struct drm_bridge_funcs funcs = {
>>> -	.attach = lvds_encoder_attach,
>>> -	.enable = lvds_encoder_enable,
>>> -	.disable = lvds_encoder_disable,
>>> -};
>>> -
>>> -static int lvds_encoder_probe(struct platform_device *pdev)
>>> -{
>>> -	struct device *dev = &pdev->dev;
>>> -	struct device_node *port;
>>> -	struct device_node *endpoint;
>>> -	struct device_node *panel_node;
>>> -	struct drm_panel *panel;
>>> -	struct lvds_encoder *lvds_encoder;
>>> -
>>> -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
>>> -	if (!lvds_encoder)
>>> -		return -ENOMEM;
>>> -
>>> -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>>> -							       GPIOD_OUT_HIGH);
>>> -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
>>> -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
>>> -
>>> -		if (err != -EPROBE_DEFER)
>>> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
>>> -		return err;
>>> -	}
>>> -
>>> -	/* Locate the panel DT node. */
>>> -	port = of_graph_get_port_by_id(dev->of_node, 1);
>>> -	if (!port) {
>>> -		dev_dbg(dev, "port 1 not found\n");
>>> -		return -ENXIO;
>>> -	}
>>> -
>>> -	endpoint = of_get_child_by_name(port, "endpoint");
>>> -	of_node_put(port);
>>> -	if (!endpoint) {
>>> -		dev_dbg(dev, "no endpoint for port 1\n");
>>> -		return -ENXIO;
>>> -	}
>>> -
>>> -	panel_node = of_graph_get_remote_port_parent(endpoint);
>>> -	of_node_put(endpoint);
>>> -	if (!panel_node) {
>>> -		dev_dbg(dev, "no remote endpoint for port 1\n");
>>> -		return -ENXIO;
>>> -	}
>>> -
>>> -	panel = of_drm_find_panel(panel_node);
>>> -	of_node_put(panel_node);
>>> -	if (IS_ERR(panel)) {
>>> -		dev_dbg(dev, "panel not found, deferring probe\n");
>>> -		return PTR_ERR(panel);
>>> -	}
>>> -
>>> -	lvds_encoder->panel_bridge =
>>> -		devm_drm_panel_bridge_add_typed(dev, panel,
>>> -						DRM_MODE_CONNECTOR_LVDS);
>>> -	if (IS_ERR(lvds_encoder->panel_bridge))
>>> -		return PTR_ERR(lvds_encoder->panel_bridge);
>>> -
>>> -	/* The panel_bridge bridge is attached to the panel's of_node,
>>> -	 * but we need a bridge attached to our of_node for our user
>>> -	 * to look up.
>>> -	 */
>>> -	lvds_encoder->bridge.of_node = dev->of_node;
>>> -	lvds_encoder->bridge.funcs = &funcs;
>>> -	drm_bridge_add(&lvds_encoder->bridge);
>>> -
>>> -	platform_set_drvdata(pdev, lvds_encoder);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int lvds_encoder_remove(struct platform_device *pdev)
>>> -{
>>> -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
>>> -
>>> -	drm_bridge_remove(&lvds_encoder->bridge);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static const struct of_device_id lvds_encoder_match[] = {
>>> -	{ .compatible = "lvds-encoder" },
>>> -	{ .compatible = "thine,thc63lvdm83d" },
>>> -	{},
>>> -};
>>> -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
>>> -
>>> -static 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@xxxxxxxxxxxxxxxx>");
>>> -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
>>> -MODULE_LICENSE("GPL");
>>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux