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

[Francesco Dolcini]

Hello Aradhya,

On Sun, May 12, 2024 at 08:53:12PM +0530, Aradhya Bhatia wrote:
> On 12/05/24 17:18, Francesco Dolcini wrote:
> > 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 at 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_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 at 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.

on this and ...

> >> +#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.

... this they are probably more of a personal taste topic, just go
for whatever you and the actual maintainer (tomi?) prefer.

Thanks,
Francesco