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@xxxxxxxxxxxxxxxx>
> Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> + },
> + {
> + .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);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
