| Message ID | 20160930143709.1388-3-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maxime, On 09/30/2016 08:07 PM, Maxime Ripard wrote: > Some boards have an entirely passive RGB to VGA bridge, based on either > DACs or resistor ladders. > > Those might or might not have an i2c bus routed to the VGA connector in > order to access the screen EDIDs. > > Add a bridge that doesn't do anything but expose the modes available on the > screen, either based on the EDIDs if available, or based on the XGA > standards. > > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > drivers/gpu/drm/bridge/Kconfig | 7 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++++++++ > 4 files changed, 285 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > > diff --git a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > new file mode 100644 > index 000000000000..a8375bc1f9cb > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > @@ -0,0 +1,48 @@ > +Dumb RGB to VGA bridge > +---------------------- > + > +This binding is aimed for dumb RGB to VGA bridges that do not require > +any configuration. > + > +Required properties: > + > +- compatible: Must be "rgb-to-vga-bridge" I'd talked to Laurent on IRC if he's okay with this. And I guess you to had discussed it during XDC too. He's suggested that it'd be better to have the compatible string as "simple-vga-dac". Some of the reasons behind having this: - We don't need to specify "rgb" in the compatible string since most simple VGA DACs can only work with an RGB input. - Also, with "dac" specified in the string, we don't need to specifically mention "bridge" in the string. Also, bridge is a drm specific term. - "simple" is considered because it's an unconfigurable bridge, and it might be misleading for other VGA DACs to not use "vga-dac". What do you think about this? If you think it's good, would it be possible for you to change this? I guess it's okay for the rest of the patch to stay the same. Sorry about the churn. Thanks, Archit > + > +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 RGB input > +- Video port 1 for VGA output > + > + > +Example > +------- > + > +bridge { > + compatible = "rgb-to-vga-bridge"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + > + vga_bridge_in: endpoint { > + remote-endpoint = <&tcon0_out_vga>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + > + vga_bridge_out: endpoint { > + remote-endpoint = <&vga_con_in>; > + }; > + }; > + }; > +}; > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index b590e678052d..d690398c541c 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -17,6 +17,13 @@ config DRM_ANALOGIX_ANX78XX > the HDMI output of an application processor to MyDP > or DisplayPort. > > +config DRM_RGB_TO_VGA > + tristate "Dumb RGB to VGA Bridge support" > + depends on OF > + select DRM_KMS_HELPER > + help > + Support for passive RGB to VGA bridges > + > config DRM_DW_HDMI > tristate > select DRM_KMS_HELPER > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index efdb07e878f5..3bb8cbe09fe9 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -1,6 +1,7 @@ > ccflags-y := -Iinclude/drm > > obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o > +obj-$(CONFIG_DRM_RGB_TO_VGA) += rgb-to-vga.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/rgb-to-vga.c b/drivers/gpu/drm/bridge/rgb-to-vga.c > new file mode 100644 > index 000000000000..5ff4d4f3598f > --- /dev/null > +++ b/drivers/gpu/drm/bridge/rgb-to-vga.c > @@ -0,0 +1,229 @@ > +/* > + * Copyright (C) 2015-2016 Free Electrons > + * Copyright (C) 2015-2016 NextThing Co > + * > + * Maxime Ripard <maxime.ripard@free-electrons.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 <linux/module.h> > +#include <linux/of_graph.h> > + > +#include <drm/drmP.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > + > +struct dumb_vga { > + struct drm_bridge bridge; > + struct drm_connector connector; > + > + struct i2c_adapter *ddc; > +}; > + > +static inline struct dumb_vga * > +drm_bridge_to_dumb_vga(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct dumb_vga, bridge); > +} > + > +static inline struct dumb_vga * > +drm_connector_to_dumb_vga(struct drm_connector *connector) > +{ > + return container_of(connector, struct dumb_vga, connector); > +} > + > +static int dumb_vga_get_modes(struct drm_connector *connector) > +{ > + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); > + struct edid *edid; > + int ret; > + > + if (IS_ERR(vga->ddc)) > + goto fallback; > + > + edid = drm_get_edid(connector, vga->ddc); > + if (!edid) { > + DRM_INFO("EDID readout failed, falling back to standard modes\n"); > + goto fallback; > + } > + > + drm_mode_connector_update_edid_property(connector, edid); > + return drm_add_edid_modes(connector, edid); > + > +fallback: > + /* > + * In case we cannot retrieve the EDIDs (broken or missing i2c > + * bus), fallback on the XGA standards > + */ > + ret = drm_add_modes_noedid(connector, 1920, 1200); > + > + /* And prefer a mode pretty much anyone can handle */ > + drm_set_preferred_mode(connector, 1024, 768); > + > + return ret; > +} > + > +static const struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = { > + .get_modes = dumb_vga_get_modes, > +}; > + > +static enum drm_connector_status > +dumb_vga_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); > + > + /* > + * Even if we have an I2C bus, we can't assume that the cable > + * is disconnected if drm_probe_ddc fails. Some cables don't > + * wire the DDC pins, or the I2C bus might not be working at > + * all. > + */ > + if (!IS_ERR(vga->ddc) && drm_probe_ddc(vga->ddc)) > + return connector_status_connected; > + > + return connector_status_unknown; > +} > + > +static void > +dumb_vga_connector_destroy(struct drm_connector *connector) > +{ > + drm_connector_cleanup(connector); > +} > + > +static const struct drm_connector_funcs dumb_vga_con_funcs = { > + .dpms = drm_atomic_helper_connector_dpms, > + .detect = dumb_vga_connector_detect, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = dumb_vga_connector_destroy, > + .reset = drm_atomic_helper_connector_reset, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static int dumb_vga_attach(struct drm_bridge *bridge) > +{ > + struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge); > + int ret; > + > + if (!bridge->encoder) { > + DRM_ERROR("Missing encoder\n"); > + return -ENODEV; > + } > + > + drm_connector_helper_add(&vga->connector, > + &dumb_vga_con_helper_funcs); > + ret = drm_connector_init(bridge->dev, &vga->connector, > + &dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA); > + if (ret) { > + DRM_ERROR("Failed to initialize connector\n"); > + return ret; > + } > + > + drm_mode_connector_attach_encoder(&vga->connector, > + bridge->encoder); > + > + return 0; > +} > + > +static const struct drm_bridge_funcs dumb_vga_bridge_funcs = { > + .attach = dumb_vga_attach, > +}; > + > +static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) > +{ > + struct device_node *end_node, *phandle, *remote; > + struct i2c_adapter *ddc; > + > + end_node = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); > + if (!end_node) { > + dev_err(dev, "Missing connector endpoint\n"); > + return ERR_PTR(-ENODEV); > + } > + > + remote = of_graph_get_remote_port_parent(end_node); > + of_node_put(end_node); > + if (!remote) { > + dev_err(dev, "Enable to parse remote node\n"); > + return ERR_PTR(-EINVAL); > + } > + > + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0); > + of_node_put(remote); > + if (!phandle) > + return ERR_PTR(-ENODEV); > + > + ddc = of_get_i2c_adapter_by_node(phandle); > + of_node_put(phandle); > + if (!ddc) > + return ERR_PTR(-EPROBE_DEFER); > + > + return ddc; > +} > + > +static int dumb_vga_probe(struct platform_device *pdev) > +{ > + struct dumb_vga *vga; > + int ret; > + > + vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL); > + if (!vga) > + return -ENOMEM; > + platform_set_drvdata(pdev, vga); > + > + vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev); > + if (IS_ERR(vga->ddc)) { > + if (PTR_ERR(vga->ddc) == -ENODEV) { > + dev_info(&pdev->dev, > + "No i2c bus specified... Disabling EDID readout\n"); > + } else { > + dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n"); > + return PTR_ERR(vga->ddc); > + } > + } > + > + vga->bridge.funcs = &dumb_vga_bridge_funcs; > + vga->bridge.of_node = pdev->dev.of_node; > + > + ret = drm_bridge_add(&vga->bridge); > + if (ret && !IS_ERR(vga->ddc)) > + i2c_put_adapter(vga->ddc); > + > + return ret; > +} > + > +static int dumb_vga_remove(struct platform_device *pdev) > +{ > + struct dumb_vga *vga = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&vga->bridge); > + > + if (!IS_ERR(vga->ddc)) > + i2c_put_adapter(vga->ddc); > + > + return 0; > +} > + > +static const struct of_device_id dumb_vga_match[] = { > + { .compatible = "rgb-to-vga-bridge" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, dumb_vga_match); > + > +struct platform_driver dumb_vga_driver = { > + .probe = dumb_vga_probe, > + .remove = dumb_vga_remove, > + .driver = { > + .name = "rgb-to-vga-bridge", > + .of_match_table = dumb_vga_match, > + }, > +}; > +module_platform_driver(dumb_vga_driver); > + > +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>"); > +MODULE_DESCRIPTION("Dumb RGB to VGA bridge driver"); > +MODULE_LICENSE("GPL"); >
Hi Archit, On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: > Hi Maxime, > > On 09/30/2016 08:07 PM, Maxime Ripard wrote: > >Some boards have an entirely passive RGB to VGA bridge, based on either > >DACs or resistor ladders. > > > >Those might or might not have an i2c bus routed to the VGA connector in > >order to access the screen EDIDs. > > > >Add a bridge that doesn't do anything but expose the modes available on the > >screen, either based on the EDIDs if available, or based on the XGA > >standards. > > > >Acked-by: Rob Herring <robh@kernel.org> > >Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >--- > > .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > > drivers/gpu/drm/bridge/Kconfig | 7 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++++++++ > > 4 files changed, 285 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > > create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > > > >diff --git a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > >new file mode 100644 > >index 000000000000..a8375bc1f9cb > >--- /dev/null > >+++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > >@@ -0,0 +1,48 @@ > >+Dumb RGB to VGA bridge > >+---------------------- > >+ > >+This binding is aimed for dumb RGB to VGA bridges that do not require > >+any configuration. > >+ > >+Required properties: > >+ > >+- compatible: Must be "rgb-to-vga-bridge" > > I'd talked to Laurent on IRC if he's okay with this. And I guess you to > had discussed it during XDC too. He's suggested that it'd be better to > have the compatible string as "simple-vga-dac". I just wished this bikeshedding had taken place publicly and be actually part of that discussion, but yeah, ok. > Some of the reasons behind having this: > > - We don't need to specify "rgb" in the compatible string since most > simple VGA DACs can only work with an RGB input. Ok. > - Also, with "dac" specified in the string, we don't need to > specifically mention "bridge" in the string. Also, bridge is a drm > specific term. > > - "simple" is considered because it's an unconfigurable bridge, and it > might be misleading for other VGA DACs to not use "vga-dac". All those "simple" bindings are just the biggest lie we ever told. It's simple when you introduce it, and then grows into something much more complicated than a non-simple implementation. > What do you think about this? If you think it's good, would it be > possible for you to change this? I guess it's okay for the rest of > the patch to stay the same. I'll update and respin the serie. Thanks, Maxime
On 10/06/2016 12:51 PM, Maxime Ripard wrote: > Hi Archit, > > On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: >> Hi Maxime, >> >> On 09/30/2016 08:07 PM, Maxime Ripard wrote: >>> Some boards have an entirely passive RGB to VGA bridge, based on either >>> DACs or resistor ladders. >>> >>> Those might or might not have an i2c bus routed to the VGA connector in >>> order to access the screen EDIDs. >>> >>> Add a bridge that doesn't do anything but expose the modes available on the >>> screen, either based on the EDIDs if available, or based on the XGA >>> standards. >>> >>> Acked-by: Rob Herring <robh@kernel.org> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>> --- >>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ >>> drivers/gpu/drm/bridge/Kconfig | 7 + >>> drivers/gpu/drm/bridge/Makefile | 1 + >>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++++++++ >>> 4 files changed, 285 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt >>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c >>> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt >>> new file mode 100644 >>> index 000000000000..a8375bc1f9cb >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt >>> @@ -0,0 +1,48 @@ >>> +Dumb RGB to VGA bridge >>> +---------------------- >>> + >>> +This binding is aimed for dumb RGB to VGA bridges that do not require >>> +any configuration. >>> + >>> +Required properties: >>> + >>> +- compatible: Must be "rgb-to-vga-bridge" >> >> I'd talked to Laurent on IRC if he's okay with this. And I guess you to >> had discussed it during XDC too. He's suggested that it'd be better to >> have the compatible string as "simple-vga-dac". > > I just wished this bikeshedding had taken place publicly and be > actually part of that discussion, but yeah, ok. Sorry about that. I'd pinged him for an Ack, the discussion went more than that :) > >> Some of the reasons behind having this: >> >> - We don't need to specify "rgb" in the compatible string since most >> simple VGA DACs can only work with an RGB input. > > Ok. > >> - Also, with "dac" specified in the string, we don't need to >> specifically mention "bridge" in the string. Also, bridge is a drm >> specific term. >> >> - "simple" is considered because it's an unconfigurable bridge, and it >> might be misleading for other VGA DACs to not use "vga-dac". > > All those "simple" bindings are just the biggest lie we ever > told. It's simple when you introduce it, and then grows into something > much more complicated than a non-simple implementation. "simple" here is supposed to mean that it's an unconfigurable RGB to VGA DAC. This isn't supposed to follow the simple-panel model, where you add the "simple-panel" string in the compatible node, along with you chip specific compatible string. In other words, this driver shouldn't be touched again in the future :) If someone wants to write a RGB to VGA driver which is even slightly configurable, they'll need to write a new bridge driver. Thanks, Archit > >> What do you think about this? If you think it's good, would it be >> possible for you to change this? I guess it's okay for the rest of >> the patch to stay the same. > > I'll update and respin the serie. > > Thanks, > Maxime >
Hello, On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: > On 10/06/2016 12:51 PM, Maxime Ripard wrote: > > On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: > >> On 09/30/2016 08:07 PM, Maxime Ripard wrote: > >>> Some boards have an entirely passive RGB to VGA bridge, based on either > >>> DACs or resistor ladders. > >>> > >>> Those might or might not have an i2c bus routed to the VGA connector in > >>> order to access the screen EDIDs. > >>> > >>> Add a bridge that doesn't do anything but expose the modes available on > >>> the screen, either based on the EDIDs if available, or based on the XGA > >>> standards. > >>> > >>> Acked-by: Rob Herring <robh@kernel.org> > >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>> --- > >>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > >>> drivers/gpu/drm/bridge/Kconfig | 7 + > >>> drivers/gpu/drm/bridge/Makefile | 1 + > >>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++ > >>> 4 files changed, 285 insertions(+) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > >>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > >>> > >>> diff --git > >>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx > >>> t > >>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx > >>> t new file mode 100644 > >>> index 000000000000..a8375bc1f9cb > >>> --- /dev/null > >>> +++ > >>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx > >>> t @@ -0,0 +1,48 @@ > >>> +Dumb RGB to VGA bridge > >>> +---------------------- > >>> + > >>> +This binding is aimed for dumb RGB to VGA bridges that do not require > >>> +any configuration. > >>> + > >>> +Required properties: > >>> + > >>> +- compatible: Must be "rgb-to-vga-bridge" > >> > >> I'd talked to Laurent on IRC if he's okay with this. And I guess you to > >> had discussed it during XDC too. He's suggested that it'd be better to > >> have the compatible string as "simple-vga-dac". > > > > I just wished this bikeshedding had taken place publicly and be > > actually part of that discussion, but yeah, ok. > > Sorry about that. I'd pinged him for an Ack, the discussion went > more than that :) > > >> Some of the reasons behind having this: > >> > >> - We don't need to specify "rgb" in the compatible string since most > >> simple VGA DACs can only work with an RGB input. > > > > Ok. > > > >> - Also, with "dac" specified in the string, we don't need to > >> specifically mention "bridge" in the string. Also, bridge is a drm > >> specific term. > >> > >> - "simple" is considered because it's an unconfigurable bridge, and it > >> might be misleading for other VGA DACs to not use "vga-dac". > > > > All those "simple" bindings are just the biggest lie we ever > > told. It's simple when you introduce it, and then grows into something > > much more complicated than a non-simple implementation. > > "simple" here is supposed to mean that it's an unconfigurable RGB to > VGA DAC. This isn't supposed to follow the simple-panel model, where > you add the "simple-panel" string in the compatible node, along with > you chip specific compatible string. I agree with Maxime, I don't like the word "simple". My preference would be "vga-dac" for a lack of a better qualifier than "simple" to describe the fact that the device requires no configuration. My only concern with "vga-dac" is that we would restrict usage of that compatible string for a subset of VGA DACs, with more complex devices not being compatible with "vga-dac" even though they are VGA DACs. That's a problem I can live with though. > In other words, this driver shouldn't be touched again in the future :) > If someone wants to write a RGB to VGA driver which is even > slightly configurable, they'll need to write a new bridge driver. I'm sure that won't be true. I can certainly foresee the addition of regulators support for instance. It's unfortunately never black and white. > >> What do you think about this? If you think it's good, would it be > >> possible for you to change this? I guess it's okay for the rest of > >> the patch to stay the same. > > > > I'll update and respin the serie.
On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: >> On 10/06/2016 12:51 PM, Maxime Ripard wrote: >> > On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: >> >> On 09/30/2016 08:07 PM, Maxime Ripard wrote: >> >>> Some boards have an entirely passive RGB to VGA bridge, based on either >> >>> DACs or resistor ladders. >> >>> >> >>> Those might or might not have an i2c bus routed to the VGA connector in >> >>> order to access the screen EDIDs. >> >>> >> >>> Add a bridge that doesn't do anything but expose the modes available on >> >>> the screen, either based on the EDIDs if available, or based on the XGA >> >>> standards. >> >>> >> >>> Acked-by: Rob Herring <robh@kernel.org> >> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> >>> --- >> >>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ >> >>> drivers/gpu/drm/bridge/Kconfig | 7 + >> >>> drivers/gpu/drm/bridge/Makefile | 1 + >> >>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++ >> >>> 4 files changed, 285 insertions(+) >> >>> create mode 100644 >> >>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt >> >>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c >> >>> >> >>> diff --git >> >>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx >> >>> t >> >>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx >> >>> t new file mode 100644 >> >>> index 000000000000..a8375bc1f9cb >> >>> --- /dev/null >> >>> +++ >> >>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx >> >>> t @@ -0,0 +1,48 @@ >> >>> +Dumb RGB to VGA bridge >> >>> +---------------------- >> >>> + >> >>> +This binding is aimed for dumb RGB to VGA bridges that do not require >> >>> +any configuration. >> >>> + >> >>> +Required properties: >> >>> + >> >>> +- compatible: Must be "rgb-to-vga-bridge" >> >> >> >> I'd talked to Laurent on IRC if he's okay with this. And I guess you to >> >> had discussed it during XDC too. He's suggested that it'd be better to >> >> have the compatible string as "simple-vga-dac". >> > >> > I just wished this bikeshedding had taken place publicly and be >> > actually part of that discussion, but yeah, ok. >> >> Sorry about that. I'd pinged him for an Ack, the discussion went >> more than that :) >> >> >> Some of the reasons behind having this: >> >> >> >> - We don't need to specify "rgb" in the compatible string since most >> >> simple VGA DACs can only work with an RGB input. >> > >> > Ok. >> > >> >> - Also, with "dac" specified in the string, we don't need to >> >> specifically mention "bridge" in the string. Also, bridge is a drm >> >> specific term. >> >> >> >> - "simple" is considered because it's an unconfigurable bridge, and it >> >> might be misleading for other VGA DACs to not use "vga-dac". >> > >> > All those "simple" bindings are just the biggest lie we ever >> > told. It's simple when you introduce it, and then grows into something >> > much more complicated than a non-simple implementation. >> >> "simple" here is supposed to mean that it's an unconfigurable RGB to >> VGA DAC. This isn't supposed to follow the simple-panel model, where >> you add the "simple-panel" string in the compatible node, along with >> you chip specific compatible string. > > I agree with Maxime, I don't like the word "simple". My preference would be > "vga-dac" for a lack of a better qualifier than "simple" to describe the fact > that the device requires no configuration. My only concern with "vga-dac" is > that we would restrict usage of that compatible string for a subset of VGA > DACs, with more complex devices not being compatible with "vga-dac" even > though they are VGA DACs. That's a problem I can live with though. While we're bikeshedding (feel free to ignore my input on this), I think Maxime's initial "dumb" qualifier was better than "simple". I think "passive" also gets the point across better than "simple", which we've already established as something else in drm. Now that I've gotten that out of the way, this patch looks good to me regardless of the name. Reviewed-by: Sean Paul <seanpaul@chromium.org> Sean > >> In other words, this driver shouldn't be touched again in the future :) >> If someone wants to write a RGB to VGA driver which is even >> slightly configurable, they'll need to write a new bridge driver. > > I'm sure that won't be true. I can certainly foresee the addition of > regulators support for instance. It's unfortunately never black and white. > >> >> What do you think about this? If you think it's good, would it be >> >> possible for you to change this? I guess it's okay for the rest of >> >> the patch to stay the same. >> > >> > I'll update and respin the serie. > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sean, On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote: > On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote: > > On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: > >> On 10/06/2016 12:51 PM, Maxime Ripard wrote: > >>> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: > >>>> On 09/30/2016 08:07 PM, Maxime Ripard wrote: > >>>>> Some boards have an entirely passive RGB to VGA bridge, based on > >>>>> either DACs or resistor ladders. > >>>>> > >>>>> Those might or might not have an i2c bus routed to the VGA connector > >>>>> in order to access the screen EDIDs. > >>>>> > >>>>> Add a bridge that doesn't do anything but expose the modes available > >>>>> on the screen, either based on the EDIDs if available, or based on > >>>>> the XGA standards. > >>>>> > >>>>> Acked-by: Rob Herring <robh@kernel.org> > >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>>>> --- > >>>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > >>>>> drivers/gpu/drm/bridge/Kconfig | 7 + > >>>>> drivers/gpu/drm/bridge/Makefile | 1 + > >>>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++ > >>>>> 4 files changed, 285 insertions(+) > >>>>> create mode 100644 > >>>>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx > >>>>> t > >>>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > >>>>> > >>>>> diff --git > >>>>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>> txt > >>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>> txt > >>>>> new file mode 100644 > >>>>> index 000000000000..a8375bc1f9cb > >>>>> --- /dev/null > >>>>> +++ > >>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>> tx > >>>>> t @@ -0,0 +1,48 @@ > >>>>> +Dumb RGB to VGA bridge > >>>>> +---------------------- > >>>>> + > >>>>> +This binding is aimed for dumb RGB to VGA bridges that do not > >>>>> require > >>>>> +any configuration. > >>>>> + > >>>>> +Required properties: > >>>>> + > >>>>> +- compatible: Must be "rgb-to-vga-bridge" > >>>> > >>>> I'd talked to Laurent on IRC if he's okay with this. And I guess you > >>>> to had discussed it during XDC too. He's suggested that it'd be better > >>>> to have the compatible string as "simple-vga-dac". > >>> > >>> I just wished this bikeshedding had taken place publicly and be > >>> actually part of that discussion, but yeah, ok. > >> > >> Sorry about that. I'd pinged him for an Ack, the discussion went > >> more than that :) > >> > >>>> Some of the reasons behind having this: > >>>> > >>>> - We don't need to specify "rgb" in the compatible string since most > >>>> simple VGA DACs can only work with an RGB input. > >>> > >>> Ok. > >>> > >>>> - Also, with "dac" specified in the string, we don't need to > >>>> specifically mention "bridge" in the string. Also, bridge is a drm > >>>> specific term. > >>>> > >>>> - "simple" is considered because it's an unconfigurable bridge, and it > >>>> might be misleading for other VGA DACs to not use "vga-dac". > >>> > >>> All those "simple" bindings are just the biggest lie we ever > >>> told. It's simple when you introduce it, and then grows into something > >>> much more complicated than a non-simple implementation. > >> > >> "simple" here is supposed to mean that it's an unconfigurable RGB to > >> VGA DAC. This isn't supposed to follow the simple-panel model, where > >> you add the "simple-panel" string in the compatible node, along with > >> you chip specific compatible string. > > > > I agree with Maxime, I don't like the word "simple". My preference would > > be "vga-dac" for a lack of a better qualifier than "simple" to describe > > the fact that the device requires no configuration. My only concern with > > "vga-dac" is that we would restrict usage of that compatible string for a > > subset of VGA DACs, with more complex devices not being compatible with > > "vga-dac" even though they are VGA DACs. That's a problem I can live with > > though. > > While we're bikeshedding (feel free to ignore my input on this), I > think Maxime's initial "dumb" qualifier was better than "simple". I think I agree. > I think "passive" also gets the point across better than "simple", which > we've already established as something else in drm. To my electrical engineer's ear, passive refers to a component or combination of components that is not capable of power gain. The resistors ladder VGA DAC that Maxime is trying to support is a passive system, but the ADV7123 VGA DAC that equally requires no configuration is an active component. > Now that I've gotten that out of the way, this patch looks good to me > regardless of the name. > > Reviewed-by: Sean Paul <seanpaul@chromium.org> > > Sean > > >> In other words, this driver shouldn't be touched again in the future :) > >> If someone wants to write a RGB to VGA driver which is even > >> slightly configurable, they'll need to write a new bridge driver. > > > > I'm sure that won't be true. I can certainly foresee the addition of > > regulators support for instance. It's unfortunately never black and white. > > > >>>> What do you think about this? If you think it's good, would it be > >>>> possible for you to change this? I guess it's okay for the rest of > >>>> the patch to stay the same. > >>> > >>> I'll update and respin the serie.
On 10/07/2016 02:34 AM, Laurent Pinchart wrote: > Hi Sean, > > On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote: >> On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote: >>> On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: >>>> On 10/06/2016 12:51 PM, Maxime Ripard wrote: >>>>> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: >>>>>> On 09/30/2016 08:07 PM, Maxime Ripard wrote: >>>>>>> Some boards have an entirely passive RGB to VGA bridge, based on >>>>>>> either DACs or resistor ladders. >>>>>>> >>>>>>> Those might or might not have an i2c bus routed to the VGA connector >>>>>>> in order to access the screen EDIDs. >>>>>>> >>>>>>> Add a bridge that doesn't do anything but expose the modes available >>>>>>> on the screen, either based on the EDIDs if available, or based on >>>>>>> the XGA standards. >>>>>>> >>>>>>> Acked-by: Rob Herring <robh@kernel.org> >>>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>>>>> --- >>>>>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ >>>>>>> drivers/gpu/drm/bridge/Kconfig | 7 + >>>>>>> drivers/gpu/drm/bridge/Makefile | 1 + >>>>>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++ >>>>>>> 4 files changed, 285 insertions(+) >>>>>>> create mode 100644 >>>>>>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx >>>>>>> t >>>>>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>> txt >>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>> txt >>>>>>> new file mode 100644 >>>>>>> index 000000000000..a8375bc1f9cb >>>>>>> --- /dev/null >>>>>>> +++ >>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>> tx >>>>>>> t @@ -0,0 +1,48 @@ >>>>>>> +Dumb RGB to VGA bridge >>>>>>> +---------------------- >>>>>>> + >>>>>>> +This binding is aimed for dumb RGB to VGA bridges that do not >>>>>>> require >>>>>>> +any configuration. >>>>>>> + >>>>>>> +Required properties: >>>>>>> + >>>>>>> +- compatible: Must be "rgb-to-vga-bridge" >>>>>> >>>>>> I'd talked to Laurent on IRC if he's okay with this. And I guess you >>>>>> to had discussed it during XDC too. He's suggested that it'd be better >>>>>> to have the compatible string as "simple-vga-dac". >>>>> >>>>> I just wished this bikeshedding had taken place publicly and be >>>>> actually part of that discussion, but yeah, ok. >>>> >>>> Sorry about that. I'd pinged him for an Ack, the discussion went >>>> more than that :) >>>> >>>>>> Some of the reasons behind having this: >>>>>> >>>>>> - We don't need to specify "rgb" in the compatible string since most >>>>>> simple VGA DACs can only work with an RGB input. >>>>> >>>>> Ok. >>>>> >>>>>> - Also, with "dac" specified in the string, we don't need to >>>>>> specifically mention "bridge" in the string. Also, bridge is a drm >>>>>> specific term. >>>>>> >>>>>> - "simple" is considered because it's an unconfigurable bridge, and it >>>>>> might be misleading for other VGA DACs to not use "vga-dac". >>>>> >>>>> All those "simple" bindings are just the biggest lie we ever >>>>> told. It's simple when you introduce it, and then grows into something >>>>> much more complicated than a non-simple implementation. >>>> >>>> "simple" here is supposed to mean that it's an unconfigurable RGB to >>>> VGA DAC. This isn't supposed to follow the simple-panel model, where >>>> you add the "simple-panel" string in the compatible node, along with >>>> you chip specific compatible string. >>> >>> I agree with Maxime, I don't like the word "simple". My preference would >>> be "vga-dac" for a lack of a better qualifier than "simple" to describe >>> the fact that the device requires no configuration. My only concern with >>> "vga-dac" is that we would restrict usage of that compatible string for a >>> subset of VGA DACs, with more complex devices not being compatible with >>> "vga-dac" even though they are VGA DACs. That's a problem I can live with >>> though. >> >> While we're bikeshedding (feel free to ignore my input on this), I >> think Maxime's initial "dumb" qualifier was better than "simple". > > I think I agree. > >> I think "passive" also gets the point across better than "simple", which >> we've already established as something else in drm. > > To my electrical engineer's ear, passive refers to a component or combination > of components that is not capable of power gain. The resistors ladder VGA DAC > that Maxime is trying to support is a passive system, but the ADV7123 VGA DAC > that equally requires no configuration is an active component. If no one has any more objections within the next day, I'll pull in Maxime's v5 RGB to VGA bridge driver, and change the compatible to "dumb-vga-dac". Thanks, Archit > >> Now that I've gotten that out of the way, this patch looks good to me >> regardless of the name. >> >> Reviewed-by: Sean Paul <seanpaul@chromium.org> >> >> Sean >> >>>> In other words, this driver shouldn't be touched again in the future :) >>>> If someone wants to write a RGB to VGA driver which is even >>>> slightly configurable, they'll need to write a new bridge driver. >>> >>> I'm sure that won't be true. I can certainly foresee the addition of >>> regulators support for instance. It's unfortunately never black and white. >>> >>>>>> What do you think about this? If you think it's good, would it be >>>>>> possible for you to change this? I guess it's okay for the rest of >>>>>> the patch to stay the same. >>>>> >>>>> I'll update and respin the serie. >
On Fri, Oct 07, 2016 at 10:27:31AM +0530, Archit Taneja wrote: > > > On 10/07/2016 02:34 AM, Laurent Pinchart wrote: > >Hi Sean, > > > >On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote: > >>On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote: > >>>On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: > >>>>On 10/06/2016 12:51 PM, Maxime Ripard wrote: > >>>>>On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: > >>>>>>On 09/30/2016 08:07 PM, Maxime Ripard wrote: > >>>>>>>Some boards have an entirely passive RGB to VGA bridge, based on > >>>>>>>either DACs or resistor ladders. > >>>>>>> > >>>>>>>Those might or might not have an i2c bus routed to the VGA connector > >>>>>>>in order to access the screen EDIDs. > >>>>>>> > >>>>>>>Add a bridge that doesn't do anything but expose the modes available > >>>>>>>on the screen, either based on the EDIDs if available, or based on > >>>>>>>the XGA standards. > >>>>>>> > >>>>>>>Acked-by: Rob Herring <robh@kernel.org> > >>>>>>>Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>>>>>>--- > >>>>>>>.../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > >>>>>>>drivers/gpu/drm/bridge/Kconfig | 7 + > >>>>>>>drivers/gpu/drm/bridge/Makefile | 1 + > >>>>>>>drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++ > >>>>>>>4 files changed, 285 insertions(+) > >>>>>>>create mode 100644 > >>>>>>>Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx > >>>>>>>t > >>>>>>>create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > >>>>>>> > >>>>>>>diff --git > >>>>>>>a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>>>>txt > >>>>>>>b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>>>>txt > >>>>>>>new file mode 100644 > >>>>>>>index 000000000000..a8375bc1f9cb > >>>>>>>--- /dev/null > >>>>>>>+++ > >>>>>>>b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. > >>>>>>>tx > >>>>>>>t @@ -0,0 +1,48 @@ > >>>>>>>+Dumb RGB to VGA bridge > >>>>>>>+---------------------- > >>>>>>>+ > >>>>>>>+This binding is aimed for dumb RGB to VGA bridges that do not > >>>>>>>require > >>>>>>>+any configuration. > >>>>>>>+ > >>>>>>>+Required properties: > >>>>>>>+ > >>>>>>>+- compatible: Must be "rgb-to-vga-bridge" > >>>>>> > >>>>>>I'd talked to Laurent on IRC if he's okay with this. And I guess you > >>>>>>to had discussed it during XDC too. He's suggested that it'd be better > >>>>>>to have the compatible string as "simple-vga-dac". > >>>>> > >>>>>I just wished this bikeshedding had taken place publicly and be > >>>>>actually part of that discussion, but yeah, ok. > >>>> > >>>>Sorry about that. I'd pinged him for an Ack, the discussion went > >>>>more than that :) > >>>> > >>>>>>Some of the reasons behind having this: > >>>>>> > >>>>>>- We don't need to specify "rgb" in the compatible string since most > >>>>>>simple VGA DACs can only work with an RGB input. > >>>>> > >>>>>Ok. > >>>>> > >>>>>>- Also, with "dac" specified in the string, we don't need to > >>>>>>specifically mention "bridge" in the string. Also, bridge is a drm > >>>>>>specific term. > >>>>>> > >>>>>>- "simple" is considered because it's an unconfigurable bridge, and it > >>>>>>might be misleading for other VGA DACs to not use "vga-dac". > >>>>> > >>>>>All those "simple" bindings are just the biggest lie we ever > >>>>>told. It's simple when you introduce it, and then grows into something > >>>>>much more complicated than a non-simple implementation. > >>>> > >>>>"simple" here is supposed to mean that it's an unconfigurable RGB to > >>>>VGA DAC. This isn't supposed to follow the simple-panel model, where > >>>>you add the "simple-panel" string in the compatible node, along with > >>>>you chip specific compatible string. > >>> > >>>I agree with Maxime, I don't like the word "simple". My preference would > >>>be "vga-dac" for a lack of a better qualifier than "simple" to describe > >>>the fact that the device requires no configuration. My only concern with > >>>"vga-dac" is that we would restrict usage of that compatible string for a > >>>subset of VGA DACs, with more complex devices not being compatible with > >>>"vga-dac" even though they are VGA DACs. That's a problem I can live with > >>>though. > >> > >>While we're bikeshedding (feel free to ignore my input on this), I > >>think Maxime's initial "dumb" qualifier was better than "simple". > > > >I think I agree. > > > >>I think "passive" also gets the point across better than "simple", which > >>we've already established as something else in drm. > > > >To my electrical engineer's ear, passive refers to a component or combination > >of components that is not capable of power gain. The resistors ladder VGA DAC > >that Maxime is trying to support is a passive system, but the ADV7123 VGA DAC > >that equally requires no configuration is an active component. > > If no one has any more objections within the next day, I'll pull in > Maxime's v5 RGB to VGA bridge driver, and change the compatible to > "dumb-vga-dac". That works for me. You'll probably want to update the Kconfig and file name to match though. Maxime
Hi Archit, On Friday 07 Oct 2016 10:27:31 Archit Taneja wrote: > On 10/07/2016 02:34 AM, Laurent Pinchart wrote: > > On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote: > >> On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote: > >>> On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: > >>>> On 10/06/2016 12:51 PM, Maxime Ripard wrote: > >>>>> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: > >>>>>> On 09/30/2016 08:07 PM, Maxime Ripard wrote: > >>>>>>> Some boards have an entirely passive RGB to VGA bridge, based on > >>>>>>> either DACs or resistor ladders. > >>>>>>> > >>>>>>> Those might or might not have an i2c bus routed to the VGA connector > >>>>>>> in order to access the screen EDIDs. > >>>>>>> > >>>>>>> Add a bridge that doesn't do anything but expose the modes available > >>>>>>> on the screen, either based on the EDIDs if available, or based on > >>>>>>> the XGA standards. > >>>>>>> > >>>>>>> Acked-by: Rob Herring <robh@kernel.org> > >>>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>>>>>> --- > >>>>>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > >>>>>>> drivers/gpu/drm/bridge/Kconfig | 7 + > >>>>>>> drivers/gpu/drm/bridge/Makefile | 1 + > >>>>>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++ > >>>>>>> 4 files changed, 285 insertions(+) > >>>>>>> create mode 100644 > >>>>>>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.t > >>>>>>> xt > >>>>>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > >>>>>>> > >>>>>>> diff --git > >>>>>>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge > >>>>>>> . > >>>>>>> txt > >>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge > >>>>>>> . > >>>>>>> txt > >>>>>>> new file mode 100644 > >>>>>>> index 000000000000..a8375bc1f9cb > >>>>>>> --- /dev/null > >>>>>>> +++ > >>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge > >>>>>>> . > >>>>>>> tx > >>>>>>> t @@ -0,0 +1,48 @@ > >>>>>>> +Dumb RGB to VGA bridge > >>>>>>> +---------------------- > >>>>>>> + > >>>>>>> +This binding is aimed for dumb RGB to VGA bridges that do not > >>>>>>> require > >>>>>>> +any configuration. > >>>>>>> + > >>>>>>> +Required properties: > >>>>>>> + > >>>>>>> +- compatible: Must be "rgb-to-vga-bridge" > >>>>>> > >>>>>> I'd talked to Laurent on IRC if he's okay with this. And I guess you > >>>>>> to had discussed it during XDC too. He's suggested that it'd be > >>>>>> better > >>>>>> to have the compatible string as "simple-vga-dac". > >>>>> > >>>>> I just wished this bikeshedding had taken place publicly and be > >>>>> actually part of that discussion, but yeah, ok. > >>>> > >>>> Sorry about that. I'd pinged him for an Ack, the discussion went > >>>> more than that :) > >>>> > >>>>>> Some of the reasons behind having this: > >>>>>> > >>>>>> - We don't need to specify "rgb" in the compatible string since most > >>>>>> simple VGA DACs can only work with an RGB input. > >>>>> > >>>>> Ok. > >>>>> > >>>>>> - Also, with "dac" specified in the string, we don't need to > >>>>>> specifically mention "bridge" in the string. Also, bridge is a drm > >>>>>> specific term. > >>>>>> > >>>>>> - "simple" is considered because it's an unconfigurable bridge, and > >>>>>> it might be misleading for other VGA DACs to not use "vga-dac". > >>>>> > >>>>> All those "simple" bindings are just the biggest lie we ever > >>>>> told. It's simple when you introduce it, and then grows into something > >>>>> much more complicated than a non-simple implementation. > >>>> > >>>> "simple" here is supposed to mean that it's an unconfigurable RGB to > >>>> VGA DAC. This isn't supposed to follow the simple-panel model, where > >>>> you add the "simple-panel" string in the compatible node, along with > >>>> you chip specific compatible string. > >>> > >>> I agree with Maxime, I don't like the word "simple". My preference would > >>> be "vga-dac" for a lack of a better qualifier than "simple" to describe > >>> the fact that the device requires no configuration. My only concern with > >>> "vga-dac" is that we would restrict usage of that compatible string for > >>> a subset of VGA DACs, with more complex devices not being compatible > >>> with "vga-dac" even though they are VGA DACs. That's a problem I can > >>> live with though. > >> > >> While we're bikeshedding (feel free to ignore my input on this), I > >> think Maxime's initial "dumb" qualifier was better than "simple". > > > > I think I agree. > > > >> I think "passive" also gets the point across better than "simple", which > >> we've already established as something else in drm. > > > > To my electrical engineer's ear, passive refers to a component or > > combination of components that is not capable of power gain. The > > resistors ladder VGA DAC that Maxime is trying to support is a passive > > system, but the ADV7123 VGA DAC that equally requires no configuration is > > an active component. > > If no one has any more objections within the next day, I'll pull in > Maxime's v5 RGB to VGA bridge driver, I'm testing the patch with rcar-du-drm and will provide results today. > and change the compatible to "dumb-vga-dac". Feel free to ignore the bikeshedding, but "transparent" could be a candidate to replace "dumb" (either as "vga-dac-transparent" or "transparent-vga-dac").
Hi Maxime, Thank you for the patch. On Friday 30 Sep 2016 16:37:06 Maxime Ripard wrote: > Some boards have an entirely passive RGB to VGA bridge, based on either > DACs or resistor ladders. A resistor ladder is a DAC :-) > Those might or might not have an i2c bus routed to the VGA connector in > order to access the screen EDIDs. > > Add a bridge that doesn't do anything but expose the modes available on the > screen, either based on the EDIDs if available, or based on the XGA > standards. > > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Please see below for a few comments. > --- > .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ > drivers/gpu/drm/bridge/Kconfig | 7 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/rgb-to-vga.c | 229 ++++++++++++++++++ > 4 files changed, 285 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c > > diff --git > a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > new file mode 100644 > index 000000000000..a8375bc1f9cb > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt > @@ -0,0 +1,48 @@ > +Dumb RGB to VGA bridge > +---------------------- > + > +This binding is aimed for dumb RGB to VGA bridges that do not require > +any configuration. > + > +Required properties: > + > +- compatible: Must be "rgb-to-vga-bridge" > + > +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 RGB input > +- Video port 1 for VGA output > + > + > +Example > +------- > + > +bridge { > + compatible = "rgb-to-vga-bridge"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + > + vga_bridge_in: endpoint { > + remote-endpoint = <&tcon0_out_vga>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + > + vga_bridge_out: endpoint { > + remote-endpoint = <&vga_con_in>; > + }; > + }; > + }; > +}; > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index b590e678052d..d690398c541c 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -17,6 +17,13 @@ config DRM_ANALOGIX_ANX78XX > the HDMI output of an application processor to MyDP > or DisplayPort. > > +config DRM_RGB_TO_VGA > + tristate "Dumb RGB to VGA Bridge support" > + depends on OF > + select DRM_KMS_HELPER > + help > + Support for passive RGB to VGA bridges > + > config DRM_DW_HDMI > tristate > select DRM_KMS_HELPER > diff --git a/drivers/gpu/drm/bridge/Makefile > b/drivers/gpu/drm/bridge/Makefile index efdb07e878f5..3bb8cbe09fe9 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -1,6 +1,7 @@ > ccflags-y := -Iinclude/drm > > obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o > +obj-$(CONFIG_DRM_RGB_TO_VGA) += rgb-to-vga.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/rgb-to-vga.c > b/drivers/gpu/drm/bridge/rgb-to-vga.c new file mode 100644 > index 000000000000..5ff4d4f3598f > --- /dev/null > +++ b/drivers/gpu/drm/bridge/rgb-to-vga.c > @@ -0,0 +1,229 @@ > +/* > + * Copyright (C) 2015-2016 Free Electrons > + * Copyright (C) 2015-2016 NextThing Co > + * > + * Maxime Ripard <maxime.ripard@free-electrons.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 <linux/module.h> > +#include <linux/of_graph.h> > + > +#include <drm/drmP.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > + > +struct dumb_vga { > + struct drm_bridge bridge; > + struct drm_connector connector; > + > + struct i2c_adapter *ddc; > +}; > + > +static inline struct dumb_vga * > +drm_bridge_to_dumb_vga(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct dumb_vga, bridge); > +} > + > +static inline struct dumb_vga * > +drm_connector_to_dumb_vga(struct drm_connector *connector) > +{ > + return container_of(connector, struct dumb_vga, connector); > +} > + > +static int dumb_vga_get_modes(struct drm_connector *connector) > +{ > + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); > + struct edid *edid; > + int ret; > + > + if (IS_ERR(vga->ddc)) > + goto fallback; > + > + edid = drm_get_edid(connector, vga->ddc); > + if (!edid) { > + DRM_INFO("EDID readout failed, falling back to standard modes\n"); > + goto fallback; > + } > + > + drm_mode_connector_update_edid_property(connector, edid); > + return drm_add_edid_modes(connector, edid); > + > +fallback: > + /* > + * In case we cannot retrieve the EDIDs (broken or missing i2c > + * bus), fallback on the XGA standards > + */ > + ret = drm_add_modes_noedid(connector, 1920, 1200); > + > + /* And prefer a mode pretty much anyone can handle */ > + drm_set_preferred_mode(connector, 1024, 768); > + > + return ret; > +} > + > +static const struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = > { + .get_modes = dumb_vga_get_modes, > +}; > + > +static enum drm_connector_status > +dumb_vga_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); > + > + /* > + * Even if we have an I2C bus, we can't assume that the cable > + * is disconnected if drm_probe_ddc fails. Some cables don't > + * wire the DDC pins, or the I2C bus might not be working at > + * all. > + */ > + if (!IS_ERR(vga->ddc) && drm_probe_ddc(vga->ddc)) > + return connector_status_connected; > + > + return connector_status_unknown; > +} > + > +static void > +dumb_vga_connector_destroy(struct drm_connector *connector) > +{ > + drm_connector_cleanup(connector); > +} > + > +static const struct drm_connector_funcs dumb_vga_con_funcs = { > + .dpms = drm_atomic_helper_connector_dpms, > + .detect = dumb_vga_connector_detect, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = dumb_vga_connector_destroy, You can use drm_connector_cleanup directly here. > + .reset = drm_atomic_helper_connector_reset, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static int dumb_vga_attach(struct drm_bridge *bridge) > +{ > + struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge); > + int ret; > + > + if (!bridge->encoder) { > + DRM_ERROR("Missing encoder\n"); > + return -ENODEV; > + } > + > + drm_connector_helper_add(&vga->connector, > + &dumb_vga_con_helper_funcs); > + ret = drm_connector_init(bridge->dev, &vga->connector, > + &dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA); > + if (ret) { > + DRM_ERROR("Failed to initialize connector\n"); > + return ret; > + } > + > + drm_mode_connector_attach_encoder(&vga->connector, > + bridge->encoder); > + > + return 0; > +} > + > +static const struct drm_bridge_funcs dumb_vga_bridge_funcs = { > + .attach = dumb_vga_attach, > +}; > + > +static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) > +{ > + struct device_node *end_node, *phandle, *remote; > + struct i2c_adapter *ddc; > + > + end_node = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); > + if (!end_node) { > + dev_err(dev, "Missing connector endpoint\n"); > + return ERR_PTR(-ENODEV); > + } > + > + remote = of_graph_get_remote_port_parent(end_node); > + of_node_put(end_node); > + if (!remote) { > + dev_err(dev, "Enable to parse remote node\n"); > + return ERR_PTR(-EINVAL); > + } > + > + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0); > + of_node_put(remote); > + if (!phandle) > + return ERR_PTR(-ENODEV); > + > + ddc = of_get_i2c_adapter_by_node(phandle); > + of_node_put(phandle); > + if (!ddc) > + return ERR_PTR(-EPROBE_DEFER); > + > + return ddc; > +} > + > +static int dumb_vga_probe(struct platform_device *pdev) > +{ > + struct dumb_vga *vga; > + int ret; > + > + vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL); > + if (!vga) > + return -ENOMEM; > + platform_set_drvdata(pdev, vga); > + > + vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev); > + if (IS_ERR(vga->ddc)) { > + if (PTR_ERR(vga->ddc) == -ENODEV) { > + dev_info(&pdev->dev, > + "No i2c bus specified... Disabling EDID readout\n"); Nitpicking, there's no need for an ellipsis ("..."). I'd write the message as "DDC not available, disabling EDID readout". You could also turn it into a dev_dbg() message as I'm not sure it's really crucial information, that's up to you. > + } else { > + dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n"); > + return PTR_ERR(vga->ddc); > + } > + } > + > + vga->bridge.funcs = &dumb_vga_bridge_funcs; > + vga->bridge.of_node = pdev->dev.of_node; > + > + ret = drm_bridge_add(&vga->bridge); > + if (ret && !IS_ERR(vga->ddc)) > + i2c_put_adapter(vga->ddc); > + > + return ret; > +} > + > +static int dumb_vga_remove(struct platform_device *pdev) > +{ > + struct dumb_vga *vga = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&vga->bridge); > + > + if (!IS_ERR(vga->ddc)) > + i2c_put_adapter(vga->ddc); > + > + return 0; > +} > + > +static const struct of_device_id dumb_vga_match[] = { > + { .compatible = "rgb-to-vga-bridge" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, dumb_vga_match); > + > +struct platform_driver dumb_vga_driver = { > + .probe = dumb_vga_probe, > + .remove = dumb_vga_remove, > + .driver = { > + .name = "rgb-to-vga-bridge", If we changing the compatible string to "dumb-vga-dac" as proposed by Archit, let's not forget to rename the driver (dumb-vga-dac.ko seems a good match) as well as the description string below ("Dumb VGA DAC bridge driver" ?). Both The DT binding and Kconfig texts need to be updated as well. I would also rename struct dumb_vga to dumb_vga_dac, that's up to you. > + .of_match_table = dumb_vga_match, > + }, > +}; > +module_platform_driver(dumb_vga_driver); > + > +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>"); > +MODULE_DESCRIPTION("Dumb RGB to VGA bridge driver"); > +MODULE_LICENSE("GPL");
On 10/07/2016 02:44 PM, Maxime Ripard wrote: > On Fri, Oct 07, 2016 at 10:27:31AM +0530, Archit Taneja wrote: >> >> >> On 10/07/2016 02:34 AM, Laurent Pinchart wrote: >>> Hi Sean, >>> >>> On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote: >>>> On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote: >>>>> On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote: >>>>>> On 10/06/2016 12:51 PM, Maxime Ripard wrote: >>>>>>> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote: >>>>>>>> On 09/30/2016 08:07 PM, Maxime Ripard wrote: >>>>>>>>> Some boards have an entirely passive RGB to VGA bridge, based on >>>>>>>>> either DACs or resistor ladders. >>>>>>>>> >>>>>>>>> Those might or might not have an i2c bus routed to the VGA connector >>>>>>>>> in order to access the screen EDIDs. >>>>>>>>> >>>>>>>>> Add a bridge that doesn't do anything but expose the modes available >>>>>>>>> on the screen, either based on the EDIDs if available, or based on >>>>>>>>> the XGA standards. >>>>>>>>> >>>>>>>>> Acked-by: Rob Herring <robh@kernel.org> >>>>>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>>>>>>> --- >>>>>>>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++ >>>>>>>>> drivers/gpu/drm/bridge/Kconfig | 7 + >>>>>>>>> drivers/gpu/drm/bridge/Makefile | 1 + >>>>>>>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++ >>>>>>>>> 4 files changed, 285 insertions(+) >>>>>>>>> create mode 100644 >>>>>>>>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx >>>>>>>>> t >>>>>>>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>>>> txt >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>>>> txt >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..a8375bc1f9cb >>>>>>>>> --- /dev/null >>>>>>>>> +++ >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge. >>>>>>>>> tx >>>>>>>>> t @@ -0,0 +1,48 @@ >>>>>>>>> +Dumb RGB to VGA bridge >>>>>>>>> +---------------------- >>>>>>>>> + >>>>>>>>> +This binding is aimed for dumb RGB to VGA bridges that do not >>>>>>>>> require >>>>>>>>> +any configuration. >>>>>>>>> + >>>>>>>>> +Required properties: >>>>>>>>> + >>>>>>>>> +- compatible: Must be "rgb-to-vga-bridge" >>>>>>>> >>>>>>>> I'd talked to Laurent on IRC if he's okay with this. And I guess you >>>>>>>> to had discussed it during XDC too. He's suggested that it'd be better >>>>>>>> to have the compatible string as "simple-vga-dac". >>>>>>> >>>>>>> I just wished this bikeshedding had taken place publicly and be >>>>>>> actually part of that discussion, but yeah, ok. >>>>>> >>>>>> Sorry about that. I'd pinged him for an Ack, the discussion went >>>>>> more than that :) >>>>>> >>>>>>>> Some of the reasons behind having this: >>>>>>>> >>>>>>>> - We don't need to specify "rgb" in the compatible string since most >>>>>>>> simple VGA DACs can only work with an RGB input. >>>>>>> >>>>>>> Ok. >>>>>>> >>>>>>>> - Also, with "dac" specified in the string, we don't need to >>>>>>>> specifically mention "bridge" in the string. Also, bridge is a drm >>>>>>>> specific term. >>>>>>>> >>>>>>>> - "simple" is considered because it's an unconfigurable bridge, and it >>>>>>>> might be misleading for other VGA DACs to not use "vga-dac". >>>>>>> >>>>>>> All those "simple" bindings are just the biggest lie we ever >>>>>>> told. It's simple when you introduce it, and then grows into something >>>>>>> much more complicated than a non-simple implementation. >>>>>> >>>>>> "simple" here is supposed to mean that it's an unconfigurable RGB to >>>>>> VGA DAC. This isn't supposed to follow the simple-panel model, where >>>>>> you add the "simple-panel" string in the compatible node, along with >>>>>> you chip specific compatible string. >>>>> >>>>> I agree with Maxime, I don't like the word "simple". My preference would >>>>> be "vga-dac" for a lack of a better qualifier than "simple" to describe >>>>> the fact that the device requires no configuration. My only concern with >>>>> "vga-dac" is that we would restrict usage of that compatible string for a >>>>> subset of VGA DACs, with more complex devices not being compatible with >>>>> "vga-dac" even though they are VGA DACs. That's a problem I can live with >>>>> though. >>>> >>>> While we're bikeshedding (feel free to ignore my input on this), I >>>> think Maxime's initial "dumb" qualifier was better than "simple". >>> >>> I think I agree. >>> >>>> I think "passive" also gets the point across better than "simple", which >>>> we've already established as something else in drm. >>> >>> To my electrical engineer's ear, passive refers to a component or combination >>> of components that is not capable of power gain. The resistors ladder VGA DAC >>> that Maxime is trying to support is a passive system, but the ADV7123 VGA DAC >>> that equally requires no configuration is an active component. >> >> If no one has any more objections within the next day, I'll pull in >> Maxime's v5 RGB to VGA bridge driver, and change the compatible to >> "dumb-vga-dac". > > That works for me. You'll probably want to update the Kconfig and file > name to match though. Queued to drm-misc, with the changes suggested by you and Laurent. Thanks, Archit > > Maxime >
Hi Archit, On Monday 10 Oct 2016 11:05:10 Archit Taneja wrote: > On 10/07/2016 02:44 PM, Maxime Ripard wrote: > > On Fri, Oct 07, 2016 at 10:27:31AM +0530, Archit Taneja wrote: [snip] > >> If no one has any more objections within the next day, I'll pull in > >> Maxime's v5 RGB to VGA bridge driver, and change the compatible to > >> "dumb-vga-dac". > > > > That works for me. You'll probably want to update the Kconfig and file > > name to match though. > > Queued to drm-misc, with the changes suggested by you and Laurent. Those changes would have been worth a repost. I've had a look at the patch you've committed and it looks OK to me, so no harm done (the commit message is a bit inaccurate, but it's not the end of the world). Could you please make sure you repost patches in the future when you change them in non-trivial ways ?
On 10/10/2016 12:45 PM, Laurent Pinchart wrote: > Hi Archit, > > On Monday 10 Oct 2016 11:05:10 Archit Taneja wrote: >> On 10/07/2016 02:44 PM, Maxime Ripard wrote: >>> On Fri, Oct 07, 2016 at 10:27:31AM +0530, Archit Taneja wrote: > > [snip] > >>>> If no one has any more objections within the next day, I'll pull in >>>> Maxime's v5 RGB to VGA bridge driver, and change the compatible to >>>> "dumb-vga-dac". >>> >>> That works for me. You'll probably want to update the Kconfig and file >>> name to match though. >> >> Queued to drm-misc, with the changes suggested by you and Laurent. > > Those changes would have been worth a repost. I've had a look at the patch > you've committed and it looks OK to me, so no harm done (the commit message is > a bit inaccurate, but it's not the end of the world). Could you please make > sure you repost patches in the future when you change them in non-trivial ways > ? Sorry about that. Will repost from now onwards if the changes are too significant. Archit >
diff --git a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt new file mode 100644 index 000000000000..a8375bc1f9cb --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt @@ -0,0 +1,48 @@ +Dumb RGB to VGA bridge +---------------------- + +This binding is aimed for dumb RGB to VGA bridges that do not require +any configuration. + +Required properties: + +- compatible: Must be "rgb-to-vga-bridge" + +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 RGB input +- Video port 1 for VGA output + + +Example +------- + +bridge { + compatible = "rgb-to-vga-bridge"; + #address-cells = <1>; + #size-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + + vga_bridge_in: endpoint { + remote-endpoint = <&tcon0_out_vga>; + }; + }; + + port@1 { + reg = <1>; + + vga_bridge_out: endpoint { + remote-endpoint = <&vga_con_in>; + }; + }; + }; +}; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index b590e678052d..d690398c541c 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -17,6 +17,13 @@ config DRM_ANALOGIX_ANX78XX the HDMI output of an application processor to MyDP or DisplayPort. +config DRM_RGB_TO_VGA + tristate "Dumb RGB to VGA Bridge support" + depends on OF + select DRM_KMS_HELPER + help + Support for passive RGB to VGA bridges + config DRM_DW_HDMI tristate select DRM_KMS_HELPER diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index efdb07e878f5..3bb8cbe09fe9 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,6 +1,7 @@ ccflags-y := -Iinclude/drm obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o +obj-$(CONFIG_DRM_RGB_TO_VGA) += rgb-to-vga.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/rgb-to-vga.c b/drivers/gpu/drm/bridge/rgb-to-vga.c new file mode 100644 index 000000000000..5ff4d4f3598f --- /dev/null +++ b/drivers/gpu/drm/bridge/rgb-to-vga.c @@ -0,0 +1,229 @@ +/* + * Copyright (C) 2015-2016 Free Electrons + * Copyright (C) 2015-2016 NextThing Co + * + * Maxime Ripard <maxime.ripard@free-electrons.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 <linux/module.h> +#include <linux/of_graph.h> + +#include <drm/drmP.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> + +struct dumb_vga { + struct drm_bridge bridge; + struct drm_connector connector; + + struct i2c_adapter *ddc; +}; + +static inline struct dumb_vga * +drm_bridge_to_dumb_vga(struct drm_bridge *bridge) +{ + return container_of(bridge, struct dumb_vga, bridge); +} + +static inline struct dumb_vga * +drm_connector_to_dumb_vga(struct drm_connector *connector) +{ + return container_of(connector, struct dumb_vga, connector); +} + +static int dumb_vga_get_modes(struct drm_connector *connector) +{ + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); + struct edid *edid; + int ret; + + if (IS_ERR(vga->ddc)) + goto fallback; + + edid = drm_get_edid(connector, vga->ddc); + if (!edid) { + DRM_INFO("EDID readout failed, falling back to standard modes\n"); + goto fallback; + } + + drm_mode_connector_update_edid_property(connector, edid); + return drm_add_edid_modes(connector, edid); + +fallback: + /* + * In case we cannot retrieve the EDIDs (broken or missing i2c + * bus), fallback on the XGA standards + */ + ret = drm_add_modes_noedid(connector, 1920, 1200); + + /* And prefer a mode pretty much anyone can handle */ + drm_set_preferred_mode(connector, 1024, 768); + + return ret; +} + +static const struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = { + .get_modes = dumb_vga_get_modes, +}; + +static enum drm_connector_status +dumb_vga_connector_detect(struct drm_connector *connector, bool force) +{ + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); + + /* + * Even if we have an I2C bus, we can't assume that the cable + * is disconnected if drm_probe_ddc fails. Some cables don't + * wire the DDC pins, or the I2C bus might not be working at + * all. + */ + if (!IS_ERR(vga->ddc) && drm_probe_ddc(vga->ddc)) + return connector_status_connected; + + return connector_status_unknown; +} + +static void +dumb_vga_connector_destroy(struct drm_connector *connector) +{ + drm_connector_cleanup(connector); +} + +static const struct drm_connector_funcs dumb_vga_con_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .detect = dumb_vga_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = dumb_vga_connector_destroy, + .reset = drm_atomic_helper_connector_reset, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static int dumb_vga_attach(struct drm_bridge *bridge) +{ + struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge); + int ret; + + if (!bridge->encoder) { + DRM_ERROR("Missing encoder\n"); + return -ENODEV; + } + + drm_connector_helper_add(&vga->connector, + &dumb_vga_con_helper_funcs); + ret = drm_connector_init(bridge->dev, &vga->connector, + &dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA); + if (ret) { + DRM_ERROR("Failed to initialize connector\n"); + return ret; + } + + drm_mode_connector_attach_encoder(&vga->connector, + bridge->encoder); + + return 0; +} + +static const struct drm_bridge_funcs dumb_vga_bridge_funcs = { + .attach = dumb_vga_attach, +}; + +static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) +{ + struct device_node *end_node, *phandle, *remote; + struct i2c_adapter *ddc; + + end_node = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); + if (!end_node) { + dev_err(dev, "Missing connector endpoint\n"); + return ERR_PTR(-ENODEV); + } + + remote = of_graph_get_remote_port_parent(end_node); + of_node_put(end_node); + if (!remote) { + dev_err(dev, "Enable to parse remote node\n"); + return ERR_PTR(-EINVAL); + } + + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0); + of_node_put(remote); + if (!phandle) + return ERR_PTR(-ENODEV); + + ddc = of_get_i2c_adapter_by_node(phandle); + of_node_put(phandle); + if (!ddc) + return ERR_PTR(-EPROBE_DEFER); + + return ddc; +} + +static int dumb_vga_probe(struct platform_device *pdev) +{ + struct dumb_vga *vga; + int ret; + + vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL); + if (!vga) + return -ENOMEM; + platform_set_drvdata(pdev, vga); + + vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev); + if (IS_ERR(vga->ddc)) { + if (PTR_ERR(vga->ddc) == -ENODEV) { + dev_info(&pdev->dev, + "No i2c bus specified... Disabling EDID readout\n"); + } else { + dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n"); + return PTR_ERR(vga->ddc); + } + } + + vga->bridge.funcs = &dumb_vga_bridge_funcs; + vga->bridge.of_node = pdev->dev.of_node; + + ret = drm_bridge_add(&vga->bridge); + if (ret && !IS_ERR(vga->ddc)) + i2c_put_adapter(vga->ddc); + + return ret; +} + +static int dumb_vga_remove(struct platform_device *pdev) +{ + struct dumb_vga *vga = platform_get_drvdata(pdev); + + drm_bridge_remove(&vga->bridge); + + if (!IS_ERR(vga->ddc)) + i2c_put_adapter(vga->ddc); + + return 0; +} + +static const struct of_device_id dumb_vga_match[] = { + { .compatible = "rgb-to-vga-bridge" }, + {}, +}; +MODULE_DEVICE_TABLE(of, dumb_vga_match); + +struct platform_driver dumb_vga_driver = { + .probe = dumb_vga_probe, + .remove = dumb_vga_remove, + .driver = { + .name = "rgb-to-vga-bridge", + .of_match_table = dumb_vga_match, + }, +}; +module_platform_driver(dumb_vga_driver); + +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>"); +MODULE_DESCRIPTION("Dumb RGB to VGA bridge driver"); +MODULE_LICENSE("GPL");