[PATCH v17 1/8] drm: bridge: Cadence: Create mhdp helper driver

[Dmitry Baryshkov]

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 at nxp.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