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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZkDx94qEVv65FquF@gaggiata.pivistrello.it>
Date: Sun, 12 May 2024 18:44:39 +0200
From: Francesco Dolcini <francesco@...cini.it>
To: Aradhya Bhatia <a-bhatia1@...com>
Cc: Francesco Dolcini <francesco@...cini.it>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Jyri Sarha <jyri.sarha@....fi>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	DRI Development List <dri-devel@...ts.freedesktop.org>,
	Devicetree List <devicetree@...r.kernel.org>,
	Linux Kernel List <linux-kernel@...r.kernel.org>,
	Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
	Praneeth Bajjuri <praneeth@...com>, Udit Kumar <u-kumar1@...com>,
	Alexander Sverdlin <alexander.sverdlin@...mens.com>,
	Randolph Sapp <rs@...com>, Devarsh Thakkar <devarsht@...com>,
	Jayesh Choudhary <j-choudhary@...com>, Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH 4/4] drm/tidss: Add OLDI bridge support

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