linux-kernel - Re: [PATCH v18 6/8] phy: freescale: Add DisplayPort/HDMI Combo-PHY driver for i.MX8MQ

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: <ux55oif6kyp6thojj55ymlcjjgsmqbrc35wtmmfbaun7qlxfuw@k6fi3ea5laty>
Date: Sat, 30 Nov 2024 10:28:04 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Sandor Yu <sandor.yu@....com>
Cc: "andrzej.hajda@...el.com" <andrzej.hajda@...el.com>, 
	"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	"jonas@...boo.se" <jonas@...boo.se>, "jernej.skrabec@...il.com" <jernej.skrabec@...il.com>, 
	"airlied@...il.com" <airlied@...il.com>, "daniel@...ll.ch" <daniel@...ll.ch>, 
	"robh+dt@...nel.org" <robh+dt@...nel.org>, 
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>, "shawnguo@...nel.org" <shawnguo@...nel.org>, 
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>, 
	"vkoul@...nel.org" <vkoul@...nel.org>, 
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>, "mripard@...nel.org" <mripard@...nel.org>, 
	"kernel@...gutronix.de" <kernel@...gutronix.de>, dl-linux-imx <linux-imx@....com>, 
	Oliver Brown <oliver.brown@....com>, 
	"alexander.stein@...tq-group.com" <alexander.stein@...tq-group.com>, "sam@...nborg.org" <sam@...nborg.org>
Subject: Re: [PATCH v18 6/8] phy: freescale: Add DisplayPort/HDMI Combo-PHY
 driver for i.MX8MQ

On Tue, Nov 26, 2024 at 02:12:04PM +0000, Sandor Yu wrote:
> > 
> > 
> > On 5 November 2024 14:05:51 GMT, Sandor Yu <sandor.yu@....com> wrote:
> > >>
> > >> On Tue, Oct 29, 2024 at 02:02:14PM +0800, Sandor Yu wrote:
> > >> > Add Cadence HDP-TX DisplayPort and HDMI PHY driver for i.MX8MQ.
> > >> >
> > >> > Cadence HDP-TX PHY could be put in either DP mode or HDMI mode
> > base
> > >> > on the configuration chosen.
> > >> > DisplayPort or HDMI PHY mode is configured in the driver.
> > >> >
> > >> > Signed-off-by: Sandor Yu <Sandor.yu@....com>
> > >> > Signed-off-by: Alexander Stein <alexander.stein@...tq-group.com>
> > >> > ---
> > >> > v17->v18:
> > >> > - fix build error as code rebase to latest kernel version.
> > >> >
> > >> >  drivers/phy/freescale/Kconfig                |   10 +
> > >> >  drivers/phy/freescale/Makefile               |    1 +
> > >> >  drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c | 1337
> > >> ++++++++++++++++++
> > >> >  3 files changed, 1348 insertions(+)  create mode 100644
> > >> > drivers/phy/freescale/phy-fsl-imx8mq-hdptx.c
> > >> >
> > >> > diff --git a/drivers/phy/freescale/Kconfig
> > >> > b/drivers/phy/freescale/Kconfig index dcd9acff6d01a..2b1210367b31c
> > >> > 100644
> > >> > --- a/drivers/phy/freescale/Kconfig
> > >> > +++ b/drivers/phy/freescale/Kconfig
> > >> > @@ -35,6 +35,16 @@ config PHY_FSL_IMX8M_PCIE
> > >> >         Enable this to add support for the PCIE PHY as found on
> > >> >         i.MX8M family of SOCs.
> > >> >
> > >> > +config PHY_FSL_IMX8MQ_HDPTX
> > >> > +     tristate "Freescale i.MX8MQ DP/HDMI PHY support"
> > >> > +     depends on OF && HAS_IOMEM
> > >> > +     depends on COMMON_CLK
> > >> > +     select GENERIC_PHY
> > >> > +     select CDNS_MHDP_HELPER
> > >>
> > >> This can have problems with being satisfied on randconfig builds,
> > >> because CDNS_MHDP_HELPER is deep inside the DRM tree.
> > >
> > >Yes, it should be. Change it to "depend on CDNS_MHDP_HELPER" will
> > eliminate this problem.
> > 
> > No, depending on a non-user-selectable symbol is a bad idea. You should
> > either depend/select all necessary symbols or, better in my opinion, move
> > your helpers out of the DRM tree.
> 
> How about change CDNS_MHDP_HELPER to user selectable? such as
> 
>  config CDNS_MHDP_HELPER
>          tristate "Cadence MHDP Helper driver"
>          help
>            Enable Cadence MHDP helpers for mailbox, HDMI and DP.
>            This driver provides a foundational layer of mailbox communication for
>            various Cadence MHDP IP implementations, such as HDMI and DisplayPort

I'd say, it's a bad idea. Helpers should be automatically selected.

> 
> Finding a suitable location for the helper code is challenging. 
> It needs to be shared among various IP versions (essentially different SoCs) 
> and across different driver types to facilitate mailbox access.
> I've searched the kernel code but haven't found a good precedent. 
> Placing this helper in either drivers/gpu/drm/bridge/cadence or drivers/soc/ (as you previously suggested) has its drawbacks. 
> drivers/gpu/drm/bridge/cadence at least provides better context for readers.

Yes, I understand the issue. However you might as well use drivers/phy/
for the helpers: your DRM driver already depends on GENERIC_PHY, but
there is no dependency from the PHY onto the DRM.

> 
> > 
> > 
> > >
> > >>
> > >> > +     help
> > >> > +       Enable this to support the Cadence HDPTX DP/HDMI PHY
> > driver
> > >> > +       on i.MX8MQ SOC.
> > >> > +
> > >> >  config PHY_FSL_IMX8QM_HSIO
> > >> >       tristate "Freescale i.MX8QM HSIO PHY"
> > >> >       depends on OF && HAS_IOMEM

-- 
With best wishes
Dmitry