Hello Aradhya, thanks for you patch, I should be able to test your patch on my
hardware in the coming days.
On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote:
> Up till now, the OLDI support in tidss was integrated within the tidss dispc.
> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds /
> lvds-clone
> modes.
>
> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
> configurations.
>
> Signed-off-by: Aradhya Bhatia <a-bhat...@ti.com>
> ---
> drivers/gpu/drm/tidss/Makefile | 3 +-
> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +-
> drivers/gpu/drm/tidss/tidss_dispc.h | 4 +
> drivers/gpu/drm/tidss/tidss_drv.c | 13 +-
> drivers/gpu/drm/tidss/tidss_drv.h | 4 +
> drivers/gpu/drm/tidss/tidss_oldi.c | 568 ++++++++++++++++++++++++++++
> drivers/gpu/drm/tidss/tidss_oldi.h | 73 ++++
> 7 files changed, 673 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c
> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h
>
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c
> b/drivers/gpu/drm/tidss/tidss_drv.c
> index d15f836dca95..fd90e8498cc2 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -23,6 +23,7 @@
> #include "tidss_drv.h"
> #include "tidss_kms.h"
> #include "tidss_irq.h"
> +#include "tidss_oldi.h"
>
> /* Power management */
>
> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
>
> spin_lock_init(&tidss->wait_lock);
>
> + ret = tidss_oldi_init(tidss);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to init OLDI (%d)\n", ret);
> + return ret;
> + }
return dev_err_probe()
> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c
> b/drivers/gpu/drm/tidss/tidss_oldi.c
> new file mode 100644
> index 000000000000..fd96ca815542
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
> @@ -0,0 +1,568 @@
...
> + ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
> + &panel, &bridge);
> + if (ret) {
> + /*
> + * Either there was no OLDI sink in the devicetree, or
> + * the OLDI sink has not been added yet. In any case,
> + * return.
> + * We don't want to have an OLDI node connected to DSS
> + * but not to any sink.
> + */
> + if (ret != -EPROBE_DEFER)
> + dev_err(tidss->dev,
> + "no panel/bridge for OLDI%d. Error
> %d\n",
> + oldi_instance, ret);
just dev_err_probe
> + goto err_put_node;
> + }
...
> + if (IS_ERR(oldi->io_ctrl)) {
> + dev_err(oldi->dev,
> + "%s: oldi%d syscon_regmap_lookup_by_phandle
> failed %ld\n",
> + __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
> + ret = PTR_ERR(oldi->io_ctrl);
dev_err_probe
> + goto err_put_node;
> + }
> +
> + oldi->s_clk = of_clk_get_by_name(child, "s_clk");
> + if (IS_ERR(oldi->s_clk)) {
> + dev_err(oldi->dev,
> + "%s: oldi%d Failed to get s_clk: %ld\n",
> + __func__, oldi_instance, PTR_ERR(oldi->s_clk));
> + ret = PTR_ERR(oldi->s_clk);
dev_err_probe
In general, in this function, sometime you print an error and goto
err_put_node, sometime you just goto err_put_node. Not sure what's the
rationale on this.
> + goto err_put_node;
> + }
> +
> + /* Register the bridge. */
> + oldi->bridge.of_node = child;
> + oldi->bridge.driver_private = oldi;
> + oldi->bridge.funcs = &tidss_oldi_bridge_funcs;
> + oldi->bridge.timings = &default_tidss_oldi_timings;
> +
> + tidss->oldis[tidss->num_oldis++] = oldi;
> + oldi->tidss = tidss;
> +
> + drm_bridge_add(&oldi->bridge);
> + }
> +
> +err_put_node:
> + of_node_put(child);
> + of_node_put(oldi_parent);
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h
> b/drivers/gpu/drm/tidss/tidss_oldi.h
> new file mode 100644
> index 000000000000..5ad02ddea11a
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2023 - Texas Instruments Incorporated
> + *
> + * Aradhya Bhatia <a-bha...@ti.com>
> + */
> +
> +#ifndef __TIDSS_OLDI_H__
> +#define __TIDSS_OLDI_H__
> +
> +#include <linux/media-bus-format.h>
> +
> +#include "tidss_drv.h"
> +#include "tidss_dispc.h"
> +
> +struct tidss_oldi;
why do you need this here?
> +
> +/* OLDI Instances */
> +#define OLDI(n) n
> +
> +/* OLDI PORTS */
> +#define OLDI_INPUT_PORT 0
> +#define OLDI_OURPUT_PORT 1
> +
> +/* OLDI Config Bits */
> +#define OLDI_ENABLE BIT(0)
> +#define OLDI_MAP (BIT(1) | BIT(2) | BIT(3))
> +#define OLDI_SRC BIT(4)
> +#define OLDI_CLONE_MODE BIT(5)
> +#define OLDI_MASTERSLAVE BIT(6)
> +#define OLDI_DEPOL BIT(7)
> +#define OLDI_MSB BIT(8)
> +#define OLDI_LBEN BIT(9)
> +#define OLDI_LBDATA BIT(10)
> +#define OLDI_DUALMODESYNC BIT(11)
> +#define OLDI_SOFTRST BIT(12)
> +#define OLDI_TPATCFG BIT(13)
> +
> +/* Control MMR Register */
> +
> +/* Register offsets */
> +#define OLDI_PD_CTRL 0x100
> +#define OLDI_LB_CTRL 0x104
> +
> +/* Power control bits */
> +#define OLDI_PWRDN_TX(n) BIT(n)
> +
> +/* LVDS Bandgap reference Enable/Disable */
> +#define OLDI_PWRDN_BG BIT(8)
> +
> +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */
this is used only on a single C files, move it there?
I would consider this comment in general for this header file,
from a quick check most of this is used only in tidss_oldi.c.
Francesco
