[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <vlqcv7pdhyf6rjmkm2ccyvg3fv64gcp536zppns6nc3xpzuew6@j3qrwextsp33>
Date: Tue, 24 Sep 2024 12:31:02 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Alexander Stein <alexander.stein@...tq-group.com>
Cc: andrzej.hajda@...el.com, neil.armstrong@...aro.org,
Laurent.pinchart@...asonboard.com, jonas@...boo.se, jernej.skrabec@...il.com, airlied@...il.com,
daniel@...ll.ch, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com, vkoul@...nel.org,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org, mripard@...nel.org,
Sandor Yu <Sandor.yu@....com>, kernel@...gutronix.de, linux-imx@....com, oliver.brown@....com,
sam@...nborg.org
Subject: Re: [PATCH v17 1/8] drm: bridge: Cadence: Create mhdp helper driver
On Tue, Sep 24, 2024 at 10:41:26AM GMT, Alexander Stein wrote:
> Hi Sandor,
>
> Am Dienstag, 24. September 2024, 09:36:46 CEST schrieb Sandor Yu:
> > MHDP8546 mailbox access functions will be share to other mhdp driver
> > and Cadence HDP-TX HDMI/DP PHY drivers.
> > Create a new mhdp helper driver and move all those functions into.
> >
> > cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(),
> > because it use the DPTX command ID DPTX_WRITE_REGISTER.
> >
> > New cdns_mhdp_reg_write() is created with the general command ID
> > GENERAL_REGISTER_WRITE.
> >
> > Rewrite cdns_mhdp_set_firmware_active() in mhdp8546 core driver,
> > use cdns_mhdp_mailbox_send() to replace cdns_mhdp_mailbox_write()
> > same as the other mailbox access functions.
> >
> > Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex
> > to prevent race conditions in mailbox access by multi drivers.
> >
> > Signed-off-by: Sandor Yu <Sandor.yu@....com>
> > ---
> > v16->v17:
> > - Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex
> >
> > v12->v16:
> > *No change.
> >
> > V11->v12:
> > - Move status initialize out of mbox_mutex.
> > - Reorder API functions in alphabetical.
> > - Add notes for malibox access functions.
> > - Add year 2024 to copyright.
> >
> > drivers/gpu/drm/bridge/cadence/Kconfig | 4 +
> > drivers/gpu/drm/bridge/cadence/Makefile | 1 +
> > .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 307 +++++++++++++
> > .../drm/bridge/cadence/cdns-mhdp8546-core.c | 425 ++++--------------
> > .../drm/bridge/cadence/cdns-mhdp8546-core.h | 47 +-
> > .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 36 +-
> > include/drm/bridge/cdns-mhdp-helper.h | 94 ++++
> > 7 files changed, 507 insertions(+), 407 deletions(-)
> > create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
> >
> > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig b/drivers/gpu/drm/bridge/cadence/Kconfig
> > index cced81633ddcd..e0973339e9e33 100644
> > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > @@ -21,6 +21,9 @@ config DRM_CDNS_DSI_J721E
> > the routing of the DSS DPI signal to the Cadence DSI.
> > endif
> >
> > +config CDNS_MHDP_HELPER
> > + tristate
> > +
> > config DRM_CDNS_MHDP8546
> > tristate "Cadence DPI/DP bridge"
> > select DRM_DISPLAY_DP_HELPER
> > @@ -28,6 +31,7 @@ config DRM_CDNS_MHDP8546
> > select DRM_DISPLAY_HELPER
> > select DRM_KMS_HELPER
> > select DRM_PANEL_BRIDGE
> > + select CDNS_MHDP_HELPER
> > depends on OF
> > help
> > Support Cadence DPI to DP bridge. This is an internal
> > diff --git a/drivers/gpu/drm/bridge/cadence/Makefile b/drivers/gpu/drm/bridge/cadence/Makefile
> > index c95fd5b81d137..087dc074820d7 100644
> > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > @@ -2,6 +2,7 @@
> > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > cdns-dsi-y := cdns-dsi-core.o
> > cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> > +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> > obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> > cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > new file mode 100644
> > index 0000000000000..c60a6b69a5343
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > @@ -0,0 +1,307 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023, 2024 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#include <drm/bridge/cdns-mhdp-helper.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/module.h>
> > +
> > +/* Protects mailbox communications with the firmware */
> > +DEFINE_MUTEX(mhdp_mailbox_mutex);
>
> This is not enough if the driver is built as a module:
>
> > ERROR: modpost: "mhdp_mailbox_mutex"
> > [drivers/gpu/drm/bridge/cadence/cdns-mhdp8546.ko] undefined! ERROR:
> > modpost: "mhdp_mailbox_mutex"
> > [drivers/gpu/drm/bridge/cadence/cdns-mhdp8501.ko] undefined!
>
> Not sure if EXPORT_SYMBOL_GPL() on a mutex is considered good style.
Absolutely no. The API should wrap register access, using the mutex
whether required, rather than exporting the mutex for everybody else to
use.
>
> Best regards,
> Alexander
>
> > [snip]
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists