[3/4,v5] drm/bridge: Add timing support to dumb VGA DAC

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

We assign the proper timing data to the pointer inside the
bridge struct so display controllers that need to align their
timings to the bridge can pick it up and work from there.

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 v4->v5:
- Rewrite the support using the new concept of defining
  fine-granular sampling (setup+hold) timing definitions
  stored in the bridge timings struct.
ChangeLog v3->v4:
- Actually have the code syntactically correct and compiling :(
  (Kconfig mistake.)
  (...)
  AS      usr/initramfs_data.o
  AR      usr/built-in.o
  CC      drivers/gpu/drm/bridge/dumb-vga-dac.o
  AR      drivers/gpu/drm/bridge/built-in.o
  AR      drivers/gpu/drm/built-in.o
  AR      drivers/gpu/built-in.o
  AR      drivers/built-in.o
  (...)
ChangeLog v2->v3:
- Move const specifier.
- Cut one line of code assigning bus flags.
- Preserve the "ti,ths8135" compatible for elder device trees.
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 | 61 +++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 3 deletions(-)

Hi Linus,

Thank you for the patch.

On Friday, 15 December 2017 14:10:46 EET Linus Walleij wrote:
> This extends the dumb VGA DAC bridge to handle the THS8134A
> and THS8134B VGA DACs in addition to those already handled.
> 
> We assign the proper timing data to the pointer inside the
> bridge struct so display controllers that need to align their
> timings to the bridge can pick it up and work from there.
> 
> 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 v4->v5:
> - Rewrite the support using the new concept of defining
>   fine-granular sampling (setup+hold) timing definitions
>   stored in the bridge timings struct.
> ChangeLog v3->v4:
> - Actually have the code syntactically correct and compiling :(
>   (Kconfig mistake.)
>   (...)
>   AS      usr/initramfs_data.o
>   AR      usr/built-in.o
>   CC      drivers/gpu/drm/bridge/dumb-vga-dac.o
>   AR      drivers/gpu/drm/bridge/built-in.o
>   AR      drivers/gpu/drm/built-in.o
>   AR      drivers/gpu/built-in.o
>   AR      drivers/built-in.o
>   (...)
> ChangeLog v2->v3:
> - Move const specifier.
> - Cut one line of code assigning bus flags.
> - Preserve the "ti,ths8135" compatible for elder device trees.
> 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 | 61 ++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index de5e7dee7ad6..34788783a90f
> 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>
> 
> @@ -176,11 +177,13 @@ static struct i2c_adapter
> *dumb_vga_retrieve_ddc(struct device *dev) static int dumb_vga_probe(struct
> platform_device *pdev)
>  {
>  	struct dumb_vga *vga;
> +	const struct drm_bridge_timings *timings;
> 
>  	vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
>  	if (!vga)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, vga);
> +	timings = of_device_get_match_data(&pdev->dev);
> 
>  	vga->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
>  	if (IS_ERR(vga->vdd)) {
> @@ -204,6 +207,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
> 
>  	vga->bridge.funcs = &dumb_vga_bridge_funcs;
>  	vga->bridge.of_node = pdev->dev.of_node;
> +	vga->bridge.timings = timings;

Do you need the intermediate timings variable ?

>  	drm_bridge_add(&vga->bridge);
> 
> @@ -222,10 +226,61 @@ static int dumb_vga_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> +/*
> + * We assume the ADV7123 DAC is the "default" for historical reasons
> + * Information taken from the ADV7123 datasheet, revision D.
> + * NOTE: the ADV7123EP seems to have other timings and need a new timings
> + * set if used.
> + */
> +static const struct drm_bridge_timings default_dac_timings = {
> +	/* Timing specifications, datasheet page 7 */
> +	.sampling_edge = true,
> +	.setup_time_ps = 500,
> +	.hold_time_ps = 1500,
> +};

You know what's lovely ? The setup time depends on the power supply voltage 
:-) Let's use 500ps for now, that's a conservative value that will work for 
both 5V and 3.3V. If anyone needs to lower it to 200ps later, they can always 
implement support for voltage-dependent timings.

> +/*
> + * Information taken from the THS8134, THS8134A, THS8134B datasheet named
> + * "SLVS205D", dated May 1990, revised March 2000.
> + */
> +static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> +	/* From timing diagram, datasheet page 9 */
> +	.sampling_edge = true,
> +	/* From datasheet, page 12 */
> +	.setup_time_ps = 3000,
> +	/* I guess this means latched input */
> +	.hold_time_ps = 0,
> +};
> +
> +/*
> + * Information taken from the THS8135 datasheet named "SLAS343B", dated
> + * May 2001, revised April 2013.
> + */
> +static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> +	/* From timing diagram, datasheet page 14 */
> +	.sampling_edge = true,
> +	/* From datasheet, page 16 */
> +	.setup_time_ps = 2000,
> +	.hold_time_ps = 500,
> +};
> +
>  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_timings,

Shouldn't we leave this NULL for dumb VGA DACs ? They're made of passive 
components and don't sample the signal, so there's no real timings that we can 
report.

Apart from that,

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

> +	},
> +	{
> +		.compatible = "adi,adv7123",
> +		.data = &default_dac_timings,
> +	},
> +	{
> +		.compatible = "ti,ths8135",
> +		.data = &ti_ths8135_dac_timings,
> +	},
> +	{
> +		.compatible = "ti,ths8134",
> +		.data = &ti_ths8134_dac_timings,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dumb_vga_match);