| Message ID | 1572443014-17335-2-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add LCD panel support to iwg20d | expand |
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@bp.renesas.com> > --- > 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. 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@ideasonboard.com> > + */ > + > +#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@ideasonboard.com>"); > +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@ideasonboard.com> > - */ > - > -#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@ideasonboard.com>"); > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder"); > -MODULE_LICENSE("GPL"); >
Hi Neil, Thank you for your feedback! > From: Neil Armstrong <narmstrong@baylibre.com> > 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@bp.renesas.com> > > --- > > 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? 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@ideasonboard.com> > > + */ > > + > > +#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@ideasonboard.com>"); > > +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@ideasonboard.com> > > - */ > > - > > -#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@ideasonboard.com>"); > > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder"); > > -MODULE_LICENSE("GPL"); > >
Hi, On 04/11/2019 11:42, Fabrizio Castro wrote: > Hi Neil, > > Thank you for your feedback! > >> From: Neil Armstrong <narmstrong@baylibre.com> >> 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@bp.renesas.com> >>> --- >>> 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@ideasonboard.com> >>> + */ >>> + >>> +#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@ideasonboard.com>"); >>> +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@ideasonboard.com> >>> - */ >>> - >>> -#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@ideasonboard.com>"); >>> -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder"); >>> -MODULE_LICENSE("GPL"); >>> >
Hi Fabrizio, On Mon, Nov 4, 2019 at 11:42 AM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > > From: Neil Armstrong <narmstrong@baylibre.com> > > Sent: 04 November 2019 09:18 > > Subject: Re: [PATCH 1/4] drm/bridge: Repurpose lvds-encoder.c > > > > 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@bp.renesas.com> > > > --- > > > 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? Sidenote: it looks like CONFIG_DRM_LVDS_ENCODER should have been enabled in shmobile_defconfig since commits 381ddfe478871588 ("drm: rcar-du: Hardcode encoders types to DRM_MODE_ENCODER_NONE") and 7160d57b6f81185c ("drm: bridge: lvds-encoder: Add thine,thc63lvdm83d compatible string"), to support thine,thc63lvdm83d in arch/arm/boot/dts/r8a7779-marzen.dts? Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for your feedback! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 08 November 2019 09:21 > Subject: Re: [PATCH 1/4] drm/bridge: Repurpose lvds-encoder.c > > Hi Fabrizio, > > On Mon, Nov 4, 2019 at 11:42 AM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > > From: Neil Armstrong <narmstrong@baylibre.com> > > > Sent: 04 November 2019 09:18 > > > Subject: Re: [PATCH 1/4] drm/bridge: Repurpose lvds-encoder.c > > > > > > 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@bp.renesas.com> > > > > --- > > > > 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? > > Sidenote: it looks like CONFIG_DRM_LVDS_ENCODER should have been enabled > in shmobile_defconfig since commits 381ddfe478871588 ("drm: rcar-du: > Hardcode encoders types to DRM_MODE_ENCODER_NONE") and 7160d57b6f81185c > ("drm: bridge: lvds-encoder: Add thine,thc63lvdm83d compatible string"), > to support thine,thc63lvdm83d in arch/arm/boot/dts/r8a7779-marzen.dts? Interesting, well the defconfig patch from this series should help then. Thanks, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
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 + 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@ideasonboard.com> + */ + +#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@ideasonboard.com>"); +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@ideasonboard.com> - */ - -#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@ideasonboard.com>"); -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder"); -MODULE_LICENSE("GPL");
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@bp.renesas.com> --- 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