Hi Francesco,
On 12/05/24 17:18, Francesco Dolcini wrote:
> Hello Aradhya, thanks for you patch, I should be able to test your patch on my
> hardware in the coming days.
That's appreciated. Thank you! =)
>
> 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-bhatia1@xxxxxx>
>> ---
>> 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
Got it. Will update in all the 4 places.
>
> 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.
There hasn't been any real logic behind the prints, except that I have
added them whenever there was something (specifc) to be explained. Other
times, for example, if the error is -ENOMEM, or any other systemic API
failure, there isn't any print required.
If this function does exit in an error, however, tidss_probe will always
throw a print (except in the case of -EPROBE_DEFER).
>
>> + 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-bhati1@xxxxxx>
>> + */
>> +
>> +#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?
So that struct tidss_device can store pointers to struct tidss_oldi
instances.
>
>> +
>> +/* 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.
Apart from struct tidss_device being able to access struct tidss_oldi,
there is no direct access to any of the above.
Perhaps I can move the idle clock definition into the C file.
However, before tidss_oldi.h, all the register definitions have been
stored in tidss_dispc_regs.h. It just seemed right to keep them out in
the header file and maintain the status quo.
Regards
Aradhya
