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

[laurent.pinchart@ideasonboard.com (Laurent Pinchart)]

Hi Linus,

Thank you for the patch.

On Friday, 1 September 2017 12:40:58 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>
> ---
>  drivers/gpu/drm/bridge/dumb-vga-dac.c | 62 ++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 831a606c4706..6c2fdcb4fde1
> 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -12,6 +12,7 @@
> 
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> +#include <linux/of_device.h>

Nitpicking, could you keep header files alphabetically sorted ?

>  #include <linux/regulator/consumer.h>
> 
>  #include <drm/drmP.h>
> @@ -19,9 +20,20 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> 
> +/**
> + * struct vga_dac_variant - characteristics of the DAC

Nitpicking again, I'd call that vga_dac_info, but it's up to you.

> + * @posedge_clk: this DAC latches data into the DAC on the positive
> + *	edge of the clock pulse, which means that display controllers
> + *	need to clock it out on the negative edge
> + */
> +struct vga_dac_variant {
> +	bool posedge_clk;

How about using DRM_BUS_FLAG_PIXDATA_NEGEDGE and DRM_BUS_FLAG_PIXDATA_POSEDGE 
explicitly instead of a bool ? For additional readability you could also 
rename the field to clk_edge_latch (or something similar), to remove the 
drive/latch ambiguity.

> +};
> +
>  struct dumb_vga {
>  	struct drm_bridge	bridge;
>  	struct drm_connector	connector;
> +	struct vga_dac_variant const *variant;
> 
>  	struct i2c_adapter	*ddc;
>  	struct regulator	*vdd;
> @@ -67,6 +79,18 @@ 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);
> 
> +	if (vga->variant->posedge_clk)
> +		/*
> +		 * If the DAC latches the data into its registers on the
> +		 * positive edge of the clock, the display driver needs to
> +		 * drive the data out on the negative edge so it is
> +		 * stable at the positive edge, so as to avoid flicker.

That's only true if there's no inverter on the board :-) I wonder how that 
could be modelled though.

> +		 * Tell the driver that we want data on the negative edge
> +		 */
> +		connector->display_info.bus_flags |=
> +			DRM_BUS_FLAG_PIXDATA_NEGEDGE;
> +
>  	return ret;
>  }
> 
> @@ -183,6 +207,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 +251,41 @@ static int dumb_vga_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> +static const struct vga_dac_variant 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.
> +	 */
> +	.posedge_clk = false,
> +};
> +
> +static const struct vga_dac_variant ti_ths_dac_variant = {
> +	/* The TI DACs read the data on the positive edge of the CLK */
> +	.posedge_clk = true,
> +};
> +
>  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,ths8134a",
> +		.data = &ti_ths_dac_variant,
> +	},
> +	{
> +		.compatible = "ti,ths8134b",
> +		.data = &ti_ths_dac_variant,
> +	},
> +	{
> +		.compatible = "ti,ths8135",
> +		.data = &ti_ths_dac_variant,
> +	},

I wonder whether a generic compatible string for the TI THS813x would make 
sense. I haven't checked the datasheets, but if they're compatible from a 
software point of view, it would be nice not to patch the driver for every 
model (especially if there's more than those three).

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dumb_vga_match);

-- 
Regards,

Laurent Pinchart