[PATCH 1/2 v2] drm: bridge: Add bindings for TI THS8134

[PATCH 1/2 v2] drm: bridge: Add bindings for TI THS8134

[Linus Walleij]

This adds device tree bindings for the Texas Instruments
THS8134A and THS8134B VGA DACs by extending and renaming the
existing bindings for THS8135.

These DACs are used for the VGA outputs on the ARM reference
designs such as Integrator, Versatile and RealView.

Cc: devicetree at vger.kernel.org
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Introduce specific-to-general compatible string:
  compatible = "ti,ths8134a", "ti,ths813x";
  so drivers can handle the whole family the same way.
- Collected Rob's ACK.
---
 .../display/bridge/{ti,ths8135.txt => ti,ths813x.txt}      | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
 rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt => ti,ths813x.txt} (65%)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
similarity index 65%
rename from Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
rename to Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
index 6ec1a880ac18..97ef5eb57052 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
@@ -1,11 +1,15 @@
-THS8135 Video DAC
------------------
+THS8134 and THS8135 Video DAC
+-----------------------------
 
-This is the binding for Texas Instruments THS8135 Video DAC bridge.
+This is the binding for Texas Instruments THS8134A, THS8134B and THS8135
+Video DAC bridge.
 
 Required properties:
 
-- compatible: Must be "ti,ths8135"
+- compatible: Must be one of
+  "ti,ths8134a", "ti,ths813x"
+  "ti,ths8134b", "ti,ths813x"
+  "ti,ths8135", "ti,ths813x"
 
 Required nodes:
 
@@ -19,7 +23,7 @@ Example
 -------
 
 vga-bridge {
-	compatible = "ti,ths8135";
+	compatible = "ti,ths8135", "ti,ths813x";
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-- 
2.13.5

[PATCH 2/2 v2] drm: bridge: Add THS8134A/B support to dumb VGA DAC

[Linus Walleij]

This extends the dumb VGA DAC bridge to handle the THS8134A
and THS8134B VGA DACs in addition to those already handled.

The THS8134A, THS8134B and as it turns out also THS8135 need to
have data clocked out at the negative edge of the clock pulse,
since they clock it into the DAC at the positive edge (so by
then it needs to be stable) so we need some extra logic to flag
this on the connector to the driver.

The semantics of the flag DRM_BUS_FLAG_PIXDATA_NEGEDGE in
<drm/drm_connector.h> clearly indicates that this flag tells
when to *drive* the data, not when the receiver *reads* it,
so the TI variants needs to be handled like this.

Introduce a variant struct and contain the information there,
and add a bit of helpful comments about how this works so
people will get it right when adding new DACs or connectiong
new display drivers to DACs.

The fact that THS8135 might be working on some systems today
is probably due to the fact that the display driver cannot
configure when the data is clocked out and the electronics
have simply been designed around it so it works anyways.

The phenomenon is very real on the ARM reference designs using
PL111 where the hardware can control which edge to push out
the data.

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Alphabetize includes
- Use a u32 with the bus polarity flags and just encode the
  polarity using the DRM define directly.
- Rename vendor_data to vendor_info.
- Simplify assignment of the flag as it is just a simple
  u32 now.
- Probe all TI variants on the "ti,ths813x" wildcard for now,
  we only need to know that the device is in this family to
  set the clock edge flag right.
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 51 ++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 831a606c4706..9cd19e4c33c9 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 
@@ -19,9 +20,18 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 
+/**
+ * struct vga_dac_info - characteristics of the DAC
+ * @clk_edge_latch: this defines the clock edge latch for the variant
+ */
+struct vga_dac_info {
+	u32 clk_edge_latch;
+};
+
 struct dumb_vga {
 	struct drm_bridge	bridge;
 	struct drm_connector	connector;
+	struct vga_dac_info const *variant;
 
 	struct i2c_adapter	*ddc;
 	struct regulator	*vdd;
@@ -55,7 +65,9 @@ static int dumb_vga_get_modes(struct drm_connector *connector)
 	}
 
 	drm_mode_connector_update_edid_property(connector, edid);
-	return drm_add_edid_modes(connector, edid);
+	ret = drm_add_edid_modes(connector, edid);
+	connector->display_info.bus_flags |= vga->variant->clk_edge_latch;
+	return ret;
 
 fallback:
 	/*
@@ -67,6 +79,8 @@ static int dumb_vga_get_modes(struct drm_connector *connector)
 	/* And prefer a mode pretty much anyone can handle */
 	drm_set_preferred_mode(connector, 1024, 768);
 
+	connector->display_info.bus_flags |= vga->variant->clk_edge_latch;
+
 	return ret;
 }
 
@@ -183,6 +197,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
 	if (!vga)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, vga);
+	vga->variant = of_device_get_match_data(&pdev->dev);
 
 	vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
 	if (IS_ERR(vga->vdd)) {
@@ -226,10 +241,38 @@ static int dumb_vga_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct vga_dac_info default_dac_variant = {
+	/*
+	 * These DACs read data on the negative edge. For example in the
+	 * ADV7123 datasheet (revision D, page 8) there is a timing diagram
+	 * making this clear. So consequently we need to latch the data
+	 * on the positive edge.
+	 */
+	.clk_edge_latch = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+};
+
+static const struct vga_dac_info ti_ths_dac_variant = {
+	/*
+	 * The TI DACs read the data on the positive edge of the CLK,
+	 * so consequently we need to latch the data on the negative
+	 * edge.
+	 */
+	.clk_edge_latch = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
+};
+
 static const struct of_device_id dumb_vga_match[] = {
-	{ .compatible = "dumb-vga-dac" },
-	{ .compatible = "adi,adv7123" },
-	{ .compatible = "ti,ths8135" },
+	{
+		.compatible = "dumb-vga-dac",
+		.data = &default_dac_variant,
+	},
+	{
+		.compatible = "adi,adv7123",
+		.data = &default_dac_variant,
+	},
+	{
+		.compatible = "ti,ths813x",
+		.data = &ti_ths_dac_variant,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, dumb_vga_match);
-- 
2.13.5

[PATCH 2/2 v2] drm: bridge: Add THS8134A/B support to dumb VGA DAC

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Tuesday, 17 October 2017 13:19:04 EEST Linus Walleij wrote:
> This extends the dumb VGA DAC bridge to handle the THS8134A
> and THS8134B VGA DACs in addition to those already handled.
> 
> The THS8134A, THS8134B and as it turns out also THS8135 need to
> have data clocked out at the negative edge of the clock pulse,
> since they clock it into the DAC at the positive edge (so by
> then it needs to be stable) so we need some extra logic to flag
> this on the connector to the driver.
> 
> The semantics of the flag DRM_BUS_FLAG_PIXDATA_NEGEDGE in
> <drm/drm_connector.h> clearly indicates that this flag tells
> when to *drive* the data, not when the receiver *reads* it,
> so the TI variants needs to be handled like this.
> 
> Introduce a variant struct and contain the information there,
> and add a bit of helpful comments about how this works so
> people will get it right when adding new DACs or connectiong
> new display drivers to DACs.
> 
> The fact that THS8135 might be working on some systems today
> is probably due to the fact that the display driver cannot
> configure when the data is clocked out and the electronics
> have simply been designed around it so it works anyways.
> 
> The phenomenon is very real on the ARM reference designs using
> PL111 where the hardware can control which edge to push out
> the data.
> 
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Alphabetize includes
> - Use a u32 with the bus polarity flags and just encode the
>   polarity using the DRM define directly.
> - Rename vendor_data to vendor_info.
> - Simplify assignment of the flag as it is just a simple
>   u32 now.
> - Probe all TI variants on the "ti,ths813x" wildcard for now,
>   we only need to know that the device is in this family to
>   set the clock edge flag right.
> ---
>  drivers/gpu/drm/bridge/dumb-vga-dac.c | 51 +++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 831a606c4706..9cd19e4c33c9
> 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -11,6 +11,7 @@
>   */
> 
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
> 
> @@ -19,9 +20,18 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> 
> +/**
> + * struct vga_dac_info - characteristics of the DAC
> + * @clk_edge_latch: this defines the clock edge latch for the variant
> + */
> +struct vga_dac_info {
> +	u32 clk_edge_latch;
> +};
> +
>  struct dumb_vga {
>  	struct drm_bridge	bridge;
>  	struct drm_connector	connector;
> +	struct vga_dac_info const *variant;

Anything wrong with the usual order of keywords (const struct vga_dac_info 
*variant) ?

>  	struct i2c_adapter	*ddc;
>  	struct regulator	*vdd;
> @@ -55,7 +65,9 @@ static int dumb_vga_get_modes(struct drm_connector
> *connector) }
> 
>  	drm_mode_connector_update_edid_property(connector, edid);
> -	return drm_add_edid_modes(connector, edid);
> +	ret = drm_add_edid_modes(connector, edid);
> +	connector->display_info.bus_flags |= vga->variant->clk_edge_latch;

As far as I can tell drm_add_edid_modes() doesn't set bus_flags, so you could 
keep the return live as-is and set the clock edge flag just before it.

> +	return ret;
> 
>  fallback:
>  	/*
> @@ -67,6 +79,8 @@ static int dumb_vga_get_modes(struct drm_connector
> *connector) /* And prefer a mode pretty much anyone can handle */
>  	drm_set_preferred_mode(connector, 1024, 768);
> 
> +	connector->display_info.bus_flags |= vga->variant->clk_edge_latch;
> +
>  	return ret;
>  }
> 
> @@ -183,6 +197,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
>  	if (!vga)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, vga);
> +	vga->variant = of_device_get_match_data(&pdev->dev);
> 
>  	vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
>  	if (IS_ERR(vga->vdd)) {
> @@ -226,10 +241,38 @@ static int dumb_vga_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> +static const struct vga_dac_info default_dac_variant = {
> +	/*
> +	 * These DACs read data on the negative edge. For example in the
> +	 * ADV7123 datasheet (revision D, page 8) there is a timing diagram
> +	 * making this clear. So consequently we need to latch the data
> +	 * on the positive edge.
> +	 */
> +	.clk_edge_latch = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> +};
> +
> +static const struct vga_dac_info ti_ths_dac_variant = {
> +	/*
> +	 * The TI DACs read the data on the positive edge of the CLK,
> +	 * so consequently we need to latch the data on the negative
> +	 * edge.
> +	 */
> +	.clk_edge_latch = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
> +};
> +
>  static const struct of_device_id dumb_vga_match[] = {
> -	{ .compatible = "dumb-vga-dac" },
> -	{ .compatible = "adi,adv7123" },
> -	{ .compatible = "ti,ths8135" },

You need to keep support for this compatible string for backward 
compatibility.

> +	{
> +		.compatible = "dumb-vga-dac",
> +		.data = &default_dac_variant,
> +	},
> +	{
> +		.compatible = "adi,adv7123",
> +		.data = &default_dac_variant,
> +	},
> +	{
> +		.compatible = "ti,ths813x",
> +		.data = &ti_ths_dac_variant,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dumb_vga_match);

-- 
Regards,

Laurent Pinchart

[PATCH 1/2 v2] drm: bridge: Add bindings for TI THS8134

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Tuesday, 17 October 2017 13:19:03 EEST Linus Walleij wrote:
> This adds device tree bindings for the Texas Instruments
> THS8134A and THS8134B VGA DACs by extending and renaming the
> existing bindings for THS8135.
> 
> These DACs are used for the VGA outputs on the ARM reference
> designs such as Integrator, Versatile and RealView.
> 
> Cc: devicetree at vger.kernel.org
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> ChangeLog v1->v2:
> - Introduce specific-to-general compatible string:
>   compatible = "ti,ths8134a", "ti,ths813x";
>   so drivers can handle the whole family the same way.
> - Collected Rob's ACK.
> ---
>  .../display/bridge/{ti,ths8135.txt => ti,ths813x.txt}      | 14 +++++++----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>  rename Documentation/devicetree/bindings/display/bridge/{ti,ths8135.txt =>
> ti,ths813x.txt} (65%)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> similarity index 65%
> rename from Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> rename to Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> index 6ec1a880ac18..97ef5eb57052 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ths813x.txt
> @@ -1,11 +1,15 @@
> -THS8135 Video DAC
> ------------------
> +THS8134 and THS8135 Video DAC
> +-----------------------------
> 
> -This is the binding for Texas Instruments THS8135 Video DAC bridge.
> +This is the binding for Texas Instruments THS8134A, THS8134B and THS8135
> +Video DAC bridge.
> 
>  Required properties:
> 
> -- compatible: Must be "ti,ths8135"
> +- compatible: Must be one of
> +  "ti,ths8134a", "ti,ths813x"
> +  "ti,ths8134b", "ti,ths813x"
> +  "ti,ths8135", "ti,ths813x"
> 
>  Required nodes:
> 
> @@ -19,7 +23,7 @@ Example
>  -------
> 
>  vga-bridge {
> -	compatible = "ti,ths8135";
> +	compatible = "ti,ths8135", "ti,ths813x";
>  	#address-cells = <1>;
>  	#size-cells = <0>;

-- 
Regards,

Laurent Pinchart