Re: [PATCH 4/4] drm/tidss: Add OLDI bridge support

[Aradhya Bhatia <a-bhatia1@ti.com>]

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@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

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@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? 

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