[PATCH v3 00/10] drm/display: generic HDMI CEC helpers

[PATCH v3 00/10] drm/display: generic HDMI CEC helpers

[Dmitry Baryshkov]

Currently it is next to impossible to implement CEC handling for the
setup using drm_bridges and drm_bridge_connector: bridges don't have a
hold of the connector at the proper time to be able to route CEC events.

At the same time it not very easy and obvious to get the CEC physical
address handling correctly. Drivers handle it at various places, ending
up with the slight differences in behaviour.

Follow up the HDMI Connector and HDMI Audio series and implement generic
HDMI CEC set of helpers that link into the HDMI Connector and
drm_bridge_connector frameworks and provide a way to implement CEC
handling for HDMI bridges in an easy yet standard way.

Notes:
- the patchset was only compile-tested
- being an RFC some of the API functions and structures are left
  undocumented
- although the patchset provides drm_bridge / drm_bridge_connector API
  for working with CEC, there is no actual bridge that uses the API
  (yet)

- I'm pretty unhappy with the drm_bridge integration code, we end up
  duplicating wrappers for CEC functions instead of reusing the
  drm_connector wrapping functions. An easy way would be to map
  drm_bridge to the corresponding drm_connector, but that would
  either require a state or storing drm_connector inside a drm_bridge.
  Current code stores cec_adapter instead.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v3:
- Moved default available_las setting to
  drm_connector_hdmi_cec_register(), allowing drivers to pass 0 instead
  of CEC_MAX_LOG_ADDRS.
- Reworked drm_bridge interface to store opaque pointer and interpret it
  as drm_connector in CEC helpers code.
- Fixed EINVAL checks in drm_connector_hdmi_cec_register().
- Added the adv7511 patch, demonstrating CEC helpers for the DRM
  bridges.
- Link to v2: https://lore.kernel.org/r/20250110-drm-hdmi-connector-cec-v2-0-9067c8f34663@linaro.org

Changes in v2:
- Refactored CEC funcs, adding more wrappers to provide more consistent
  interface (Maxime)
- Removed export of drm_connector_cec_unregister() (Maxime)
- Restored and rephrased comment in vc4_hdmi (Maxime)
- Squashed vc4 patches
- Link to v1: https://lore.kernel.org/r/20241225-drm-hdmi-connector-cec-v1-0-b80380c67221@linaro.org

---
Dmitry Baryshkov (10):
      drm/connector: add CEC-related fields
      drm/display: add CEC helpers code
      drm/display: hdmi-state-helper: handle CEC physical address
      drm/vc4: hdmi: switch to generic CEC helpers
      drm/display: bridge-connector: hook in CEC notifier support
      drm/bridge: move private data to the end of the struct
      drm/display: hdmi-cec-helper: add bridge-related functions
      drm/display: bridge-connector: handle CEC adapters
      drm/bridge: allow limiting I2S formats
      drm/bridge: adv7511: switch to the HDMI connector helpers

 drivers/gpu/drm/bridge/adv7511/Kconfig          |   4 +
 drivers/gpu/drm/bridge/adv7511/adv7511.h        |  51 ++--
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c  |  77 +-----
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c    |  56 ++--
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c    | 342 ++++++++++--------------
 drivers/gpu/drm/bridge/adv7511/adv7533.c        |   4 -
 drivers/gpu/drm/bridge/lontium-lt9611.c         |   2 +-
 drivers/gpu/drm/display/Kconfig                 |   6 +
 drivers/gpu/drm/display/Makefile                |   2 +
 drivers/gpu/drm/display/drm_bridge_connector.c  | 105 +++++++-
 drivers/gpu/drm/display/drm_hdmi_audio_helper.c |   2 +
 drivers/gpu/drm/display/drm_hdmi_cec_helper.c   | 247 +++++++++++++++++
 drivers/gpu/drm/display/drm_hdmi_state_helper.c |   9 +-
 drivers/gpu/drm/drm_bridge.c                    |  15 ++
 drivers/gpu/drm/drm_connector.c                 |  13 +
 drivers/gpu/drm/vc4/Kconfig                     |   1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c                  | 140 ++++------
 drivers/gpu/drm/vc4/vc4_hdmi.h                  |   1 -
 include/drm/display/drm_hdmi_audio_helper.h     |   1 +
 include/drm/display/drm_hdmi_cec_helper.h       |  69 +++++
 include/drm/drm_bridge.h                        |  77 ++++--
 include/drm/drm_connector.h                     |  44 +++
 22 files changed, 834 insertions(+), 434 deletions(-)
---
base-commit: 7c3127e4814c304c7d5a2fa796a42e75a9b8606f
change-id: 20241223-drm-hdmi-connector-cec-34902ce847b7

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v3 01/10] drm/connector: add CEC-related fields

[Dmitry Baryshkov]

As a preparation to adding HDMI CEC helper code, add CEC-related fields
to the struct drm_connector. Include both cec_adapter and cec_notifier,
allowing drivers to select which one to use. The unregister callback
is provided to let drivers unregister CEC-related data in a generic way
without polluting drm_connector.c with dependencies on the CEC
functions.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_connector.c | 13 ++++++++++++
 include/drm/drm_connector.h     | 44 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->probed_modes);
 	INIT_LIST_HEAD(&connector->modes);
 	mutex_init(&connector->mutex);
+	mutex_init(&connector->cec.mutex);
 	mutex_init(&connector->eld_mutex);
 	mutex_init(&connector->edid_override_mutex);
 	mutex_init(&connector->hdmi.infoframes.lock);
@@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector,
 	drm_mode_destroy(connector->dev, mode);
 }
 
+static void drm_connector_cec_unregister(struct drm_connector *connector)
+{
+	mutex_lock(&connector->cec.mutex);
+
+	if (connector->cec.funcs->unregister)
+		connector->cec.funcs->unregister(connector);
+
+	mutex_unlock(&connector->cec.mutex);
+}
+
 /**
  * drm_connector_cleanup - cleans up an initialised connector
  * @connector: connector to cleanup
@@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
 
 	platform_device_unregister(connector->hdmi_audio.codec_pdev);
 
+	drm_connector_cec_unregister(connector);
+
 	if (connector->privacy_screen) {
 		drm_privacy_screen_put(connector->privacy_screen);
 		connector->privacy_screen = NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -46,6 +46,7 @@ struct drm_property_blob;
 struct drm_printer;
 struct drm_privacy_screen;
 struct drm_edid;
+struct cec_adapter;
 struct edid;
 struct hdmi_codec_daifmt;
 struct hdmi_codec_params;
@@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs {
 			   bool enable, int direction);
 };
 
+/**
+ * struct drm_connector_cec_funcs - drm_hdmi_connector control functions
+ */
+struct drm_connector_cec_funcs {
+	/**
+	 * @adap_unregister: unregister CEC adapter / notifier.
+	 *
+	 * The callback to unregister CEC adapter or notifier, so that the core
+	 * DRM layer doesn't depend on the CEC_CORE.
+	 */
+	void (*unregister)(struct drm_connector *connector);
+};
+
 /**
  * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
  */
@@ -1832,6 +1846,31 @@ struct drm_connector_hdmi {
 	} infoframes;
 };
 
+/**
+ * struct drm_connector_cec - DRM Connector CEC-related structure
+ */
+struct drm_connector_cec {
+	/**
+	 * @mutex: protects all CEC-related fields
+	 */
+	struct mutex mutex;
+
+	/**
+	 * @adap: CEC adapter corresponding to the DRM connector.
+	 */
+	struct cec_adapter *adapter;
+
+	/**
+	 * @notifier: CEC notifier corresponding to the DRM connector.
+	 */
+	struct cec_notifier *notifier;
+
+	/**
+	 * @funcs: CEC Control Functions
+	 */
+	const struct drm_connector_cec_funcs *funcs;
+};
+
 /**
  * struct drm_connector - central DRM connector control structure
  *
@@ -2253,6 +2292,11 @@ struct drm_connector {
 	 * @hdmi_audio: HDMI codec properties and non-DRM state.
 	 */
 	struct drm_connector_hdmi_audio hdmi_audio;
+
+	/**
+	 * @cec: CEC-related data.
+	 */
+	struct drm_connector_cec cec;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)

-- 
2.39.5

Re: [PATCH v3 01/10] drm/connector: add CEC-related fields

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]

On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote:
> As a preparation to adding HDMI CEC helper code, add CEC-related fields
> to the struct drm_connector. Include both cec_adapter and cec_notifier,
> allowing drivers to select which one to use. The unregister callback
> is provided to let drivers unregister CEC-related data in a generic way
> without polluting drm_connector.c with dependencies on the CEC
> functions.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 13 ++++++++++++
>  include/drm/drm_connector.h     | 44 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->probed_modes);
>  	INIT_LIST_HEAD(&connector->modes);
>  	mutex_init(&connector->mutex);
> +	mutex_init(&connector->cec.mutex);
>  	mutex_init(&connector->eld_mutex);
>  	mutex_init(&connector->edid_override_mutex);
>  	mutex_init(&connector->hdmi.infoframes.lock);
> @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector,
>  	drm_mode_destroy(connector->dev, mode);
>  }
>  
> +static void drm_connector_cec_unregister(struct drm_connector *connector)
> +{
> +	mutex_lock(&connector->cec.mutex);
> +
> +	if (connector->cec.funcs->unregister)
> +		connector->cec.funcs->unregister(connector);
> +
> +	mutex_unlock(&connector->cec.mutex);
> +}
> +
>  /**
>   * drm_connector_cleanup - cleans up an initialised connector
>   * @connector: connector to cleanup
> @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  
>  	platform_device_unregister(connector->hdmi_audio.codec_pdev);
>  
> +	drm_connector_cec_unregister(connector);
> +

This should either be in a separate patch, or mentioned in the commit title/log

>  	if (connector->privacy_screen) {
>  		drm_privacy_screen_put(connector->privacy_screen);
>  		connector->privacy_screen = NULL;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -46,6 +46,7 @@ struct drm_property_blob;
>  struct drm_printer;
>  struct drm_privacy_screen;
>  struct drm_edid;
> +struct cec_adapter;
>  struct edid;
>  struct hdmi_codec_daifmt;
>  struct hdmi_codec_params;
> @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs {
>  			   bool enable, int direction);
>  };
>  
> +/**
> + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions
> + */
> +struct drm_connector_cec_funcs {
> +	/**
> +	 * @adap_unregister: unregister CEC adapter / notifier.
> +	 *
> +	 * The callback to unregister CEC adapter or notifier, so that the core
> +	 * DRM layer doesn't depend on the CEC_CORE.
> +	 */
> +	void (*unregister)(struct drm_connector *connector);
> +};
> +
>  /**
>   * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
>   */
> @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi {
>  	} infoframes;
>  };
>  
> +/**
> + * struct drm_connector_cec - DRM Connector CEC-related structure
> + */
> +struct drm_connector_cec {
> +	/**
> +	 * @mutex: protects all CEC-related fields
> +	 */

All fields? Which fields require to be protected by a specific mutex
here?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 01/10] drm/connector: add CEC-related fields

[Dmitry Baryshkov]

On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote:
> On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote:
> > As a preparation to adding HDMI CEC helper code, add CEC-related fields
> > to the struct drm_connector. Include both cec_adapter and cec_notifier,
> > allowing drivers to select which one to use. The unregister callback
> > is provided to let drivers unregister CEC-related data in a generic way
> > without polluting drm_connector.c with dependencies on the CEC
> > functions.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 13 ++++++++++++
> >  include/drm/drm_connector.h     | 44 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev,
> >  	INIT_LIST_HEAD(&connector->probed_modes);
> >  	INIT_LIST_HEAD(&connector->modes);
> >  	mutex_init(&connector->mutex);
> > +	mutex_init(&connector->cec.mutex);
> >  	mutex_init(&connector->eld_mutex);
> >  	mutex_init(&connector->edid_override_mutex);
> >  	mutex_init(&connector->hdmi.infoframes.lock);
> > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector,
> >  	drm_mode_destroy(connector->dev, mode);
> >  }
> >  
> > +static void drm_connector_cec_unregister(struct drm_connector *connector)
> > +{
> > +	mutex_lock(&connector->cec.mutex);
> > +
> > +	if (connector->cec.funcs->unregister)
> > +		connector->cec.funcs->unregister(connector);
> > +
> > +	mutex_unlock(&connector->cec.mutex);
> > +}
> > +
> >  /**
> >   * drm_connector_cleanup - cleans up an initialised connector
> >   * @connector: connector to cleanup
> > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
> >  
> >  	platform_device_unregister(connector->hdmi_audio.codec_pdev);
> >  
> > +	drm_connector_cec_unregister(connector);
> > +
> 
> This should either be in a separate patch, or mentioned in the commit title/log

I'll mention it in the commit message.

> 
> >  	if (connector->privacy_screen) {
> >  		drm_privacy_screen_put(connector->privacy_screen);
> >  		connector->privacy_screen = NULL;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -46,6 +46,7 @@ struct drm_property_blob;
> >  struct drm_printer;
> >  struct drm_privacy_screen;
> >  struct drm_edid;
> > +struct cec_adapter;
> >  struct edid;
> >  struct hdmi_codec_daifmt;
> >  struct hdmi_codec_params;
> > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs {
> >  			   bool enable, int direction);
> >  };
> >  
> > +/**
> > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions
> > + */
> > +struct drm_connector_cec_funcs {
> > +	/**
> > +	 * @adap_unregister: unregister CEC adapter / notifier.
> > +	 *
> > +	 * The callback to unregister CEC adapter or notifier, so that the core
> > +	 * DRM layer doesn't depend on the CEC_CORE.
> > +	 */
> > +	void (*unregister)(struct drm_connector *connector);
> > +};
> > +
> >  /**
> >   * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
> >   */
> > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi {
> >  	} infoframes;
> >  };
> >  
> > +/**
> > + * struct drm_connector_cec - DRM Connector CEC-related structure
> > + */
> > +struct drm_connector_cec {
> > +	/**
> > +	 * @mutex: protects all CEC-related fields
> > +	 */
> 
> All fields? Which fields require to be protected by a specific mutex
> here?

Yes, all the fields. adapter, notifier and funcs are all protected by
the mutex. See the drm_connector_cec_unregister() implementation (and
corresponding unregister() callbacks implementations.

-- 
With best wishes
Dmitry

Re: [PATCH v3 01/10] drm/connector: add CEC-related fields

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

On Tue, Jan 28, 2025 at 03:12:41PM +0200, Dmitry Baryshkov wrote:
> On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote:
> > On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote:
> > > As a preparation to adding HDMI CEC helper code, add CEC-related fields
> > > to the struct drm_connector. Include both cec_adapter and cec_notifier,
> > > allowing drivers to select which one to use. The unregister callback
> > > is provided to let drivers unregister CEC-related data in a generic way
> > > without polluting drm_connector.c with dependencies on the CEC
> > > functions.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 13 ++++++++++++
> > >  include/drm/drm_connector.h     | 44 +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 57 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev,
> > >  	INIT_LIST_HEAD(&connector->probed_modes);
> > >  	INIT_LIST_HEAD(&connector->modes);
> > >  	mutex_init(&connector->mutex);
> > > +	mutex_init(&connector->cec.mutex);
> > >  	mutex_init(&connector->eld_mutex);
> > >  	mutex_init(&connector->edid_override_mutex);
> > >  	mutex_init(&connector->hdmi.infoframes.lock);
> > > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector,
> > >  	drm_mode_destroy(connector->dev, mode);
> > >  }
> > >  
> > > +static void drm_connector_cec_unregister(struct drm_connector *connector)
> > > +{
> > > +	mutex_lock(&connector->cec.mutex);
> > > +
> > > +	if (connector->cec.funcs->unregister)
> > > +		connector->cec.funcs->unregister(connector);
> > > +
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +}
> > > +
> > >  /**
> > >   * drm_connector_cleanup - cleans up an initialised connector
> > >   * @connector: connector to cleanup
> > > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > >  
> > >  	platform_device_unregister(connector->hdmi_audio.codec_pdev);
> > >  
> > > +	drm_connector_cec_unregister(connector);
> > > +
> > 
> > This should either be in a separate patch, or mentioned in the commit title/log
> 
> I'll mention it in the commit message.
> 
> > 
> > >  	if (connector->privacy_screen) {
> > >  		drm_privacy_screen_put(connector->privacy_screen);
> > >  		connector->privacy_screen = NULL;
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > >  struct drm_printer;
> > >  struct drm_privacy_screen;
> > >  struct drm_edid;
> > > +struct cec_adapter;
> > >  struct edid;
> > >  struct hdmi_codec_daifmt;
> > >  struct hdmi_codec_params;
> > > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs {
> > >  			   bool enable, int direction);
> > >  };
> > >  
> > > +/**
> > > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions
> > > + */
> > > +struct drm_connector_cec_funcs {
> > > +	/**
> > > +	 * @adap_unregister: unregister CEC adapter / notifier.
> > > +	 *
> > > +	 * The callback to unregister CEC adapter or notifier, so that the core
> > > +	 * DRM layer doesn't depend on the CEC_CORE.
> > > +	 */
> > > +	void (*unregister)(struct drm_connector *connector);
> > > +};
> > > +
> > >  /**
> > >   * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
> > >   */
> > > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi {
> > >  	} infoframes;
> > >  };
> > >  
> > > +/**
> > > + * struct drm_connector_cec - DRM Connector CEC-related structure
> > > + */
> > > +struct drm_connector_cec {
> > > +	/**
> > > +	 * @mutex: protects all CEC-related fields
> > > +	 */
> > 
> > All fields? Which fields require to be protected by a specific mutex
> > here?
> 
> Yes, all the fields. adapter, notifier and funcs are all protected by
> the mutex. See the drm_connector_cec_unregister() implementation (and
> corresponding unregister() callbacks implementations.

That's still surprising to me. Like, what concurrency source / code path
will need to make sure funcs is updated properly? or the adapter and
notifier?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 01/10] drm/connector: add CEC-related fields

[Dmitry Baryshkov]

On Wed, 5 Feb 2025 at 16:28, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Jan 28, 2025 at 03:12:41PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote:
> > > On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote:
> > > > As a preparation to adding HDMI CEC helper code, add CEC-related fields
> > > > to the struct drm_connector. Include both cec_adapter and cec_notifier,
> > > > allowing drivers to select which one to use. The unregister callback
> > > > is provided to let drivers unregister CEC-related data in a generic way
> > > > without polluting drm_connector.c with dependencies on the CEC
> > > > functions.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_connector.c | 13 ++++++++++++
> > > >  include/drm/drm_connector.h     | 44 +++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 57 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev,
> > > >   INIT_LIST_HEAD(&connector->probed_modes);
> > > >   INIT_LIST_HEAD(&connector->modes);
> > > >   mutex_init(&connector->mutex);
> > > > + mutex_init(&connector->cec.mutex);
> > > >   mutex_init(&connector->eld_mutex);
> > > >   mutex_init(&connector->edid_override_mutex);
> > > >   mutex_init(&connector->hdmi.infoframes.lock);
> > > > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector,
> > > >   drm_mode_destroy(connector->dev, mode);
> > > >  }
> > > >
> > > > +static void drm_connector_cec_unregister(struct drm_connector *connector)
> > > > +{
> > > > + mutex_lock(&connector->cec.mutex);
> > > > +
> > > > + if (connector->cec.funcs->unregister)
> > > > +         connector->cec.funcs->unregister(connector);
> > > > +
> > > > + mutex_unlock(&connector->cec.mutex);
> > > > +}
> > > > +
> > > >  /**
> > > >   * drm_connector_cleanup - cleans up an initialised connector
> > > >   * @connector: connector to cleanup
> > > > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > >
> > > >   platform_device_unregister(connector->hdmi_audio.codec_pdev);
> > > >
> > > > + drm_connector_cec_unregister(connector);
> > > > +
> > >
> > > This should either be in a separate patch, or mentioned in the commit title/log
> >
> > I'll mention it in the commit message.
> >
> > >
> > > >   if (connector->privacy_screen) {
> > > >           drm_privacy_screen_put(connector->privacy_screen);
> > > >           connector->privacy_screen = NULL;
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > > >  struct drm_printer;
> > > >  struct drm_privacy_screen;
> > > >  struct drm_edid;
> > > > +struct cec_adapter;
> > > >  struct edid;
> > > >  struct hdmi_codec_daifmt;
> > > >  struct hdmi_codec_params;
> > > > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs {
> > > >                      bool enable, int direction);
> > > >  };
> > > >
> > > > +/**
> > > > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions
> > > > + */
> > > > +struct drm_connector_cec_funcs {
> > > > + /**
> > > > +  * @adap_unregister: unregister CEC adapter / notifier.
> > > > +  *
> > > > +  * The callback to unregister CEC adapter or notifier, so that the core
> > > > +  * DRM layer doesn't depend on the CEC_CORE.
> > > > +  */
> > > > + void (*unregister)(struct drm_connector *connector);
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
> > > >   */
> > > > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi {
> > > >   } infoframes;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct drm_connector_cec - DRM Connector CEC-related structure
> > > > + */
> > > > +struct drm_connector_cec {
> > > > + /**
> > > > +  * @mutex: protects all CEC-related fields
> > > > +  */
> > >
> > > All fields? Which fields require to be protected by a specific mutex
> > > here?
> >
> > Yes, all the fields. adapter, notifier and funcs are all protected by
> > the mutex. See the drm_connector_cec_unregister() implementation (and
> > corresponding unregister() callbacks implementations.
>
> That's still surprising to me. Like, what concurrency source / code path
> will need to make sure funcs is updated properly? or the adapter and
> notifier?

I'll have to check, maybe we can lift that and really protect only the
adapter and the notifier. I was using ->funcs != NULL as a way to
check that CEC is registered at all (and thus unsetting it on CEC
unregistration). But maybe that's not that necessary.

>
> Maxime



-- 
With best wishes
Dmitry

[PATCH v3 02/10] drm/display: add CEC helpers code

[Dmitry Baryshkov]

Add generic CEC helpers to be used by HDMI drivers. Both notifier and
and adapter are supported for registration. Once registered, the driver
can call common set of functions to update physical address, to
invalidate it or to unregister CEC data.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/Kconfig               |   5 +
 drivers/gpu/drm/display/Makefile              |   2 +
 drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
 include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
 4 files changed, 277 insertions(+)

diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
--- a/drivers/gpu/drm/display/Kconfig
+++ b/drivers/gpu/drm/display/Kconfig
@@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
 	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
 	  implementation).
 
+config DRM_DISPLAY_HDMI_CEC_HELPER
+	bool
+	help
+	  DRM display helpers for HDMI CEC implementation.
+
 config DRM_DISPLAY_HDMI_HELPER
 	bool
 	help
diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
--- a/drivers/gpu/drm/display/Makefile
+++ b/drivers/gpu/drm/display/Makefile
@@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
 drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
 drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
 	drm_hdmi_audio_helper.o
+drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
+	drm_hdmi_cec_helper.o
 drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
 	drm_hdmi_helper.o \
 	drm_scdc_helper.o
diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
new file mode 100644
index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
--- /dev/null
+++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright (c) 2024 Linaro Ltd
+ */
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_connector.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
+
+#include <linux/mutex.h>
+
+#include <media/cec.h>
+#include <media/cec-notifier.h>
+
+void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
+{
+	cec_unregister_adapter(connector->cec.adapter);
+	connector->cec.adapter = NULL;
+
+	cec_notifier_conn_unregister(connector->cec.notifier);
+	connector->cec.notifier = NULL;
+
+	connector->cec.funcs = NULL;
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
+
+static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
+	.unregister = drm_connector_hdmi_cec_unregister,
+};
+
+int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
+					     const char *port_name,
+					     struct device *dev)
+{
+	struct cec_connector_info conn_info;
+	struct cec_notifier *notifier;
+	int ret;
+
+	mutex_lock(&connector->cec.mutex);
+
+	if (connector->cec.funcs) {
+		ret = -EBUSY;
+		goto err_unlock;
+	}
+
+	cec_fill_conn_info_from_drm(&conn_info, connector);
+
+	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
+	if (!notifier) {
+		ret = -ENOMEM;
+		goto err_unlock;
+	}
+
+	connector->cec.notifier = notifier;
+	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
+
+	mutex_unlock(&connector->cec.mutex);
+
+	return 0;
+
+err_unlock:
+	mutex_unlock(&connector->cec.mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
+
+#define to_hdmi_cec_adapter_ops(ops) \
+	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
+
+static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct drm_connector *connector = cec_get_drvdata(adap);
+	struct drm_connector_hdmi_cec_adapter_ops *ops =
+		to_hdmi_cec_adapter_ops(connector->cec.funcs);
+
+	return ops->enable(connector, enable);
+}
+
+static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+	struct drm_connector *connector = cec_get_drvdata(adap);
+	struct drm_connector_hdmi_cec_adapter_ops *ops =
+		to_hdmi_cec_adapter_ops(connector->cec.funcs);
+
+	return ops->log_addr(connector, logical_addr);
+}
+
+static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+						u32 signal_free_time, struct cec_msg *msg)
+{
+	struct drm_connector *connector = cec_get_drvdata(adap);
+	struct drm_connector_hdmi_cec_adapter_ops *ops =
+		to_hdmi_cec_adapter_ops(connector->cec.funcs);
+
+	return ops->transmit(connector, attempts, signal_free_time, msg);
+}
+
+static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
+	.adap_enable = drm_connector_hdmi_cec_adap_enable,
+	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
+	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
+};
+
+int drm_connector_hdmi_cec_register(struct drm_connector *connector,
+				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
+				    const char *name,
+				    u8 available_las,
+				    struct device *dev)
+{
+	struct cec_connector_info conn_info;
+	struct cec_adapter *cec_adap;
+	int ret;
+
+	if (!ops->base.unregister ||
+	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
+		return -EINVAL;
+
+	mutex_lock(&connector->cec.mutex);
+
+	if (connector->cec.funcs) {
+		ret = -EBUSY;
+		goto err_unlock;
+	}
+
+	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
+					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
+					available_las ? : CEC_MAX_LOG_ADDRS);
+	ret = PTR_ERR_OR_ZERO(cec_adap);
+	if (ret < 0)
+		goto err_unlock;
+
+	cec_fill_conn_info_from_drm(&conn_info, connector);
+	cec_s_conn_info(cec_adap, &conn_info);
+
+	connector->cec.adapter = cec_adap;
+	connector->cec.funcs = &ops->base;
+
+	ret = ops->init(connector);
+	if (ret < 0)
+		goto err_delete_adapter;
+
+	ret = cec_register_adapter(cec_adap, dev);
+	if (ret < 0)
+		goto err_delete_adapter;
+
+	mutex_unlock(&connector->cec.mutex);
+
+	return 0;
+
+err_delete_adapter:
+	cec_delete_adapter(cec_adap);
+
+	connector->cec.adapter = NULL;
+
+err_unlock:
+	mutex_unlock(&connector->cec.mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
+
+void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
+					 struct cec_msg *msg)
+{
+	cec_received_msg(connector->cec.adapter, msg);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
+
+void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
+						  u8 status)
+{
+	cec_transmit_attempt_done(connector->cec.adapter, status);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
+
+void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
+					  u8 status,
+					  u8 arb_lost_cnt, u8 nack_cnt,
+					  u8 low_drive_cnt, u8 error_cnt)
+{
+	cec_transmit_done(connector->cec.adapter, status,
+			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
+
+void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
+{
+	mutex_lock(&connector->cec.mutex);
+
+	cec_phys_addr_invalidate(connector->cec.adapter);
+	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
+
+	mutex_unlock(&connector->cec.mutex);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
+
+void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
+{
+	mutex_lock(&connector->cec.mutex);
+
+	cec_s_phys_addr(connector->cec.adapter,
+			connector->display_info.source_physical_address, false);
+	cec_notifier_set_phys_addr(connector->cec.notifier,
+				   connector->display_info.source_physical_address);
+
+	mutex_unlock(&connector->cec.mutex);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
new file mode 100644
index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
--- /dev/null
+++ b/include/drm/display/drm_hdmi_cec_helper.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
+#define DRM_DISPLAY_HDMI_CEC_HELPER
+
+#include <drm/drm_connector.h>
+
+#include <linux/types.h>
+
+struct drm_connector;
+
+struct cec_msg;
+struct device;
+
+struct drm_connector_hdmi_cec_adapter_ops {
+	struct drm_connector_cec_funcs base;
+
+	int (*init)(struct drm_connector *connector);
+	void (*uninit)(struct drm_connector *connector);
+
+	int (*enable)(struct drm_connector *connector, bool enable);
+	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
+	int (*transmit)(struct drm_connector *connector, u8 attempts,
+			u32 signal_free_time, struct cec_msg *msg);
+};
+
+int drm_connector_hdmi_cec_register(struct drm_connector *connector,
+				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
+				    const char *name,
+				    u8 available_las,
+				    struct device *dev);
+
+int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
+					     const char *port_name,
+					     struct device *dev);
+
+void drm_connector_hdmi_cec_unregister(struct drm_connector *connector);
+
+void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
+					 struct cec_msg *msg);
+
+void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
+					  u8 status,
+					  u8 arb_lost_cnt, u8 nack_cnt,
+					  u8 low_drive_cnt, u8 error_cnt);
+
+void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
+						  u8 status);
+/*
+ * These functions are used by the state helper, so we end up linking to the
+ * same module. Define stubs to simplify the code.
+ */
+#ifdef CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER
+void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector);
+void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector);
+#else
+static inline void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector) {}
+static inline void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector) {}
+#endif
+
+#endif

-- 
2.39.5

Re: [PATCH v3 02/10] drm/display: add CEC helpers code

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10131 bytes --]

On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote:
> Add generic CEC helpers to be used by HDMI drivers. Both notifier and
> and adapter are supported for registration. Once registered, the driver
> can call common set of functions to update physical address, to
> invalidate it or to unregister CEC data.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/Kconfig               |   5 +
>  drivers/gpu/drm/display/Makefile              |   2 +
>  drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
>  include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
>  4 files changed, 277 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
> --- a/drivers/gpu/drm/display/Kconfig
> +++ b/drivers/gpu/drm/display/Kconfig
> @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
>  	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
>  	  implementation).
>  
> +config DRM_DISPLAY_HDMI_CEC_HELPER
> +	bool
> +	help
> +	  DRM display helpers for HDMI CEC implementation.
> +
>  config DRM_DISPLAY_HDMI_HELPER
>  	bool
>  	help
> diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
> --- a/drivers/gpu/drm/display/Makefile
> +++ b/drivers/gpu/drm/display/Makefile
> @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
>  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
>  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
>  	drm_hdmi_audio_helper.o
> +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
> +	drm_hdmi_cec_helper.o
>  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
>  	drm_hdmi_helper.o \
>  	drm_scdc_helper.o
> diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
> --- /dev/null
> +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (c) 2024 Linaro Ltd
> + */
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_connector.h>
> +#include <drm/display/drm_hdmi_cec_helper.h>
> +
> +#include <linux/mutex.h>
> +
> +#include <media/cec.h>
> +#include <media/cec-notifier.h>
> +
> +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
> +{
> +	cec_unregister_adapter(connector->cec.adapter);
> +	connector->cec.adapter = NULL;
> +
> +	cec_notifier_conn_unregister(connector->cec.notifier);
> +	connector->cec.notifier = NULL;
> +
> +	connector->cec.funcs = NULL;
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
> +
> +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
> +	.unregister = drm_connector_hdmi_cec_unregister,
> +};
> +
> +int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
> +					     const char *port_name,
> +					     struct device *dev)
> +{
> +	struct cec_connector_info conn_info;
> +	struct cec_notifier *notifier;
> +	int ret;
> +
> +	mutex_lock(&connector->cec.mutex);
> +
> +	if (connector->cec.funcs) {
> +		ret = -EBUSY;
> +		goto err_unlock;
> +	}
> +
> +	cec_fill_conn_info_from_drm(&conn_info, connector);
> +
> +	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
> +	if (!notifier) {
> +		ret = -ENOMEM;
> +		goto err_unlock;
> +	}
> +
> +	connector->cec.notifier = notifier;
> +	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
> +
> +	mutex_unlock(&connector->cec.mutex);
> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&connector->cec.mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
> +
> +#define to_hdmi_cec_adapter_ops(ops) \
> +	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
> +
> +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct drm_connector *connector = cec_get_drvdata(adap);
> +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> +
> +	return ops->enable(connector, enable);
> +}
> +
> +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	struct drm_connector *connector = cec_get_drvdata(adap);
> +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> +
> +	return ops->log_addr(connector, logical_addr);
> +}
> +
> +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +						u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct drm_connector *connector = cec_get_drvdata(adap);
> +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> +
> +	return ops->transmit(connector, attempts, signal_free_time, msg);
> +}
> +
> +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
> +	.adap_enable = drm_connector_hdmi_cec_adap_enable,
> +	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
> +	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
> +};
> +
> +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> +				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
> +				    const char *name,
> +				    u8 available_las,
> +				    struct device *dev)
> +{
> +	struct cec_connector_info conn_info;
> +	struct cec_adapter *cec_adap;
> +	int ret;
> +
> +	if (!ops->base.unregister ||
> +	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
> +		return -EINVAL;
> +
> +	mutex_lock(&connector->cec.mutex);
> +
> +	if (connector->cec.funcs) {
> +		ret = -EBUSY;
> +		goto err_unlock;
> +	}
> +
> +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> +					available_las ? : CEC_MAX_LOG_ADDRS);
> +	ret = PTR_ERR_OR_ZERO(cec_adap);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	cec_fill_conn_info_from_drm(&conn_info, connector);
> +	cec_s_conn_info(cec_adap, &conn_info);
> +
> +	connector->cec.adapter = cec_adap;
> +	connector->cec.funcs = &ops->base;
> +
> +	ret = ops->init(connector);
> +	if (ret < 0)
> +		goto err_delete_adapter;
> +
> +	ret = cec_register_adapter(cec_adap, dev);
> +	if (ret < 0)
> +		goto err_delete_adapter;
> +
> +	mutex_unlock(&connector->cec.mutex);
> +
> +	return 0;
> +
> +err_delete_adapter:
> +	cec_delete_adapter(cec_adap);
> +
> +	connector->cec.adapter = NULL;
> +
> +err_unlock:
> +	mutex_unlock(&connector->cec.mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
> +
> +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
> +					 struct cec_msg *msg)
> +{
> +	cec_received_msg(connector->cec.adapter, msg);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
> +
> +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
> +						  u8 status)
> +{
> +	cec_transmit_attempt_done(connector->cec.adapter, status);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
> +
> +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
> +					  u8 status,
> +					  u8 arb_lost_cnt, u8 nack_cnt,
> +					  u8 low_drive_cnt, u8 error_cnt)
> +{
> +	cec_transmit_done(connector->cec.adapter, status,
> +			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
> +
> +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
> +{
> +	mutex_lock(&connector->cec.mutex);
> +
> +	cec_phys_addr_invalidate(connector->cec.adapter);
> +	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
> +
> +	mutex_unlock(&connector->cec.mutex);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
> +
> +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
> +{
> +	mutex_lock(&connector->cec.mutex);
> +
> +	cec_s_phys_addr(connector->cec.adapter,
> +			connector->display_info.source_physical_address, false);
> +	cec_notifier_set_phys_addr(connector->cec.notifier,
> +				   connector->display_info.source_physical_address);
> +
> +	mutex_unlock(&connector->cec.mutex);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
> diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
> --- /dev/null
> +++ b/include/drm/display/drm_hdmi_cec_helper.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
> +#define DRM_DISPLAY_HDMI_CEC_HELPER
> +
> +#include <drm/drm_connector.h>
> +
> +#include <linux/types.h>
> +
> +struct drm_connector;
> +
> +struct cec_msg;
> +struct device;
> +
> +struct drm_connector_hdmi_cec_adapter_ops {
> +	struct drm_connector_cec_funcs base;
> +
> +	int (*init)(struct drm_connector *connector);
> +	void (*uninit)(struct drm_connector *connector);
> +
> +	int (*enable)(struct drm_connector *connector, bool enable);
> +	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
> +	int (*transmit)(struct drm_connector *connector, u8 attempts,
> +			u32 signal_free_time, struct cec_msg *msg);
> +};

Why can't we merge drm_connector_cec_funcs and
drm_connector_cec_adapter_ops? They look equivalent to me?

We should also document those hooks.

Aside from the mutex discussion, the rest of the patch looks good to me.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 02/10] drm/display: add CEC helpers code

[Dmitry Baryshkov]

On Tue, Jan 28, 2025 at 11:36:06AM +0100, Maxime Ripard wrote:
> On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote:
> > Add generic CEC helpers to be used by HDMI drivers. Both notifier and
> > and adapter are supported for registration. Once registered, the driver
> > can call common set of functions to update physical address, to
> > invalidate it or to unregister CEC data.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/Kconfig               |   5 +
> >  drivers/gpu/drm/display/Makefile              |   2 +
> >  drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
> >  include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
> >  4 files changed, 277 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> > index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
> > --- a/drivers/gpu/drm/display/Kconfig
> > +++ b/drivers/gpu/drm/display/Kconfig
> > @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
> >  	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
> >  	  implementation).
> >  
> > +config DRM_DISPLAY_HDMI_CEC_HELPER
> > +	bool
> > +	help
> > +	  DRM display helpers for HDMI CEC implementation.
> > +
> >  config DRM_DISPLAY_HDMI_HELPER
> >  	bool
> >  	help
> > diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> > index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
> > --- a/drivers/gpu/drm/display/Makefile
> > +++ b/drivers/gpu/drm/display/Makefile
> > @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
> >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
> >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
> >  	drm_hdmi_audio_helper.o
> > +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
> > +	drm_hdmi_cec_helper.o
> >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
> >  	drm_hdmi_helper.o \
> >  	drm_scdc_helper.o
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > @@ -0,0 +1,209 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright (c) 2024 Linaro Ltd
> > + */
> > +
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_connector.h>
> > +#include <drm/display/drm_hdmi_cec_helper.h>
> > +
> > +#include <linux/mutex.h>
> > +
> > +#include <media/cec.h>
> > +#include <media/cec-notifier.h>
> > +
> > +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
> > +{
> > +	cec_unregister_adapter(connector->cec.adapter);
> > +	connector->cec.adapter = NULL;
> > +
> > +	cec_notifier_conn_unregister(connector->cec.notifier);
> > +	connector->cec.notifier = NULL;
> > +
> > +	connector->cec.funcs = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
> > +
> > +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
> > +	.unregister = drm_connector_hdmi_cec_unregister,
> > +};
> > +
> > +int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
> > +					     const char *port_name,
> > +					     struct device *dev)
> > +{
> > +	struct cec_connector_info conn_info;
> > +	struct cec_notifier *notifier;
> > +	int ret;
> > +
> > +	mutex_lock(&connector->cec.mutex);
> > +
> > +	if (connector->cec.funcs) {
> > +		ret = -EBUSY;
> > +		goto err_unlock;
> > +	}
> > +
> > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > +
> > +	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
> > +	if (!notifier) {
> > +		ret = -ENOMEM;
> > +		goto err_unlock;
> > +	}
> > +
> > +	connector->cec.notifier = notifier;
> > +	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
> > +
> > +	mutex_unlock(&connector->cec.mutex);
> > +
> > +	return 0;
> > +
> > +err_unlock:
> > +	mutex_unlock(&connector->cec.mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
> > +
> > +#define to_hdmi_cec_adapter_ops(ops) \
> > +	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
> > +
> > +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > +{
> > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > +
> > +	return ops->enable(connector, enable);
> > +}
> > +
> > +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > +{
> > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > +
> > +	return ops->log_addr(connector, logical_addr);
> > +}
> > +
> > +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > +						u32 signal_free_time, struct cec_msg *msg)
> > +{
> > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > +
> > +	return ops->transmit(connector, attempts, signal_free_time, msg);
> > +}
> > +
> > +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
> > +	.adap_enable = drm_connector_hdmi_cec_adap_enable,
> > +	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
> > +	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
> > +};
> > +
> > +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> > +				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
> > +				    const char *name,
> > +				    u8 available_las,
> > +				    struct device *dev)
> > +{
> > +	struct cec_connector_info conn_info;
> > +	struct cec_adapter *cec_adap;
> > +	int ret;
> > +
> > +	if (!ops->base.unregister ||
> > +	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&connector->cec.mutex);
> > +
> > +	if (connector->cec.funcs) {
> > +		ret = -EBUSY;
> > +		goto err_unlock;
> > +	}
> > +
> > +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> > +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> > +					available_las ? : CEC_MAX_LOG_ADDRS);
> > +	ret = PTR_ERR_OR_ZERO(cec_adap);
> > +	if (ret < 0)
> > +		goto err_unlock;
> > +
> > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > +	cec_s_conn_info(cec_adap, &conn_info);
> > +
> > +	connector->cec.adapter = cec_adap;
> > +	connector->cec.funcs = &ops->base;
> > +
> > +	ret = ops->init(connector);
> > +	if (ret < 0)
> > +		goto err_delete_adapter;
> > +
> > +	ret = cec_register_adapter(cec_adap, dev);
> > +	if (ret < 0)
> > +		goto err_delete_adapter;
> > +
> > +	mutex_unlock(&connector->cec.mutex);
> > +
> > +	return 0;
> > +
> > +err_delete_adapter:
> > +	cec_delete_adapter(cec_adap);
> > +
> > +	connector->cec.adapter = NULL;
> > +
> > +err_unlock:
> > +	mutex_unlock(&connector->cec.mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
> > +
> > +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
> > +					 struct cec_msg *msg)
> > +{
> > +	cec_received_msg(connector->cec.adapter, msg);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
> > +
> > +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
> > +						  u8 status)
> > +{
> > +	cec_transmit_attempt_done(connector->cec.adapter, status);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
> > +
> > +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
> > +					  u8 status,
> > +					  u8 arb_lost_cnt, u8 nack_cnt,
> > +					  u8 low_drive_cnt, u8 error_cnt)
> > +{
> > +	cec_transmit_done(connector->cec.adapter, status,
> > +			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
> > +
> > +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
> > +{
> > +	mutex_lock(&connector->cec.mutex);
> > +
> > +	cec_phys_addr_invalidate(connector->cec.adapter);
> > +	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
> > +
> > +	mutex_unlock(&connector->cec.mutex);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
> > +
> > +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
> > +{
> > +	mutex_lock(&connector->cec.mutex);
> > +
> > +	cec_s_phys_addr(connector->cec.adapter,
> > +			connector->display_info.source_physical_address, false);
> > +	cec_notifier_set_phys_addr(connector->cec.notifier,
> > +				   connector->display_info.source_physical_address);
> > +
> > +	mutex_unlock(&connector->cec.mutex);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
> > diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
> > --- /dev/null
> > +++ b/include/drm/display/drm_hdmi_cec_helper.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: MIT */
> > +
> > +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
> > +#define DRM_DISPLAY_HDMI_CEC_HELPER
> > +
> > +#include <drm/drm_connector.h>
> > +
> > +#include <linux/types.h>
> > +
> > +struct drm_connector;
> > +
> > +struct cec_msg;
> > +struct device;
> > +
> > +struct drm_connector_hdmi_cec_adapter_ops {
> > +	struct drm_connector_cec_funcs base;
> > +
> > +	int (*init)(struct drm_connector *connector);
> > +	void (*uninit)(struct drm_connector *connector);
> > +
> > +	int (*enable)(struct drm_connector *connector, bool enable);
> > +	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
> > +	int (*transmit)(struct drm_connector *connector, u8 attempts,
> > +			u32 signal_free_time, struct cec_msg *msg);
> > +};
> 
> Why can't we merge drm_connector_cec_funcs and
> drm_connector_cec_adapter_ops? They look equivalent to me?

Well, not exactly. The funcs is a generic interface. Notifiers do not
need the adapter_ops. And cec_pin (sun4i) would also require a different
set of callbacks. Thus I decided that it's easier to subclass funcs
instead of adding all possible callbacks there.

> 
> We should also document those hooks.


Ack, I forgot to add docs here.

> 
> Aside from the mutex discussion, the rest of the patch looks good to me.
> 
> Maxime



-- 
With best wishes
Dmitry

Re: [PATCH v3 02/10] drm/display: add CEC helpers code

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 12116 bytes --]

On Tue, Jan 28, 2025 at 02:17:19PM +0200, Dmitry Baryshkov wrote:
> On Tue, Jan 28, 2025 at 11:36:06AM +0100, Maxime Ripard wrote:
> > On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote:
> > > Add generic CEC helpers to be used by HDMI drivers. Both notifier and
> > > and adapter are supported for registration. Once registered, the driver
> > > can call common set of functions to update physical address, to
> > > invalidate it or to unregister CEC data.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/display/Kconfig               |   5 +
> > >  drivers/gpu/drm/display/Makefile              |   2 +
> > >  drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
> > >  include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
> > >  4 files changed, 277 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> > > index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
> > > --- a/drivers/gpu/drm/display/Kconfig
> > > +++ b/drivers/gpu/drm/display/Kconfig
> > > @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
> > >  	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
> > >  	  implementation).
> > >  
> > > +config DRM_DISPLAY_HDMI_CEC_HELPER
> > > +	bool
> > > +	help
> > > +	  DRM display helpers for HDMI CEC implementation.
> > > +
> > >  config DRM_DISPLAY_HDMI_HELPER
> > >  	bool
> > >  	help
> > > diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> > > index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
> > > --- a/drivers/gpu/drm/display/Makefile
> > > +++ b/drivers/gpu/drm/display/Makefile
> > > @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
> > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
> > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
> > >  	drm_hdmi_audio_helper.o
> > > +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
> > > +	drm_hdmi_cec_helper.o
> > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
> > >  	drm_hdmi_helper.o \
> > >  	drm_scdc_helper.o
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > @@ -0,0 +1,209 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright (c) 2024 Linaro Ltd
> > > + */
> > > +
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/display/drm_hdmi_cec_helper.h>
> > > +
> > > +#include <linux/mutex.h>
> > > +
> > > +#include <media/cec.h>
> > > +#include <media/cec-notifier.h>
> > > +
> > > +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
> > > +{
> > > +	cec_unregister_adapter(connector->cec.adapter);
> > > +	connector->cec.adapter = NULL;
> > > +
> > > +	cec_notifier_conn_unregister(connector->cec.notifier);
> > > +	connector->cec.notifier = NULL;
> > > +
> > > +	connector->cec.funcs = NULL;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
> > > +
> > > +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
> > > +	.unregister = drm_connector_hdmi_cec_unregister,
> > > +};
> > > +
> > > +int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
> > > +					     const char *port_name,
> > > +					     struct device *dev)
> > > +{
> > > +	struct cec_connector_info conn_info;
> > > +	struct cec_notifier *notifier;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&connector->cec.mutex);
> > > +
> > > +	if (connector->cec.funcs) {
> > > +		ret = -EBUSY;
> > > +		goto err_unlock;
> > > +	}
> > > +
> > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > +
> > > +	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
> > > +	if (!notifier) {
> > > +		ret = -ENOMEM;
> > > +		goto err_unlock;
> > > +	}
> > > +
> > > +	connector->cec.notifier = notifier;
> > > +	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
> > > +
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +
> > > +	return 0;
> > > +
> > > +err_unlock:
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
> > > +
> > > +#define to_hdmi_cec_adapter_ops(ops) \
> > > +	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
> > > +
> > > +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > > +{
> > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > +
> > > +	return ops->enable(connector, enable);
> > > +}
> > > +
> > > +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > > +{
> > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > +
> > > +	return ops->log_addr(connector, logical_addr);
> > > +}
> > > +
> > > +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > > +						u32 signal_free_time, struct cec_msg *msg)
> > > +{
> > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > +
> > > +	return ops->transmit(connector, attempts, signal_free_time, msg);
> > > +}
> > > +
> > > +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
> > > +	.adap_enable = drm_connector_hdmi_cec_adap_enable,
> > > +	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
> > > +	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
> > > +};
> > > +
> > > +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> > > +				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
> > > +				    const char *name,
> > > +				    u8 available_las,
> > > +				    struct device *dev)
> > > +{
> > > +	struct cec_connector_info conn_info;
> > > +	struct cec_adapter *cec_adap;
> > > +	int ret;
> > > +
> > > +	if (!ops->base.unregister ||
> > > +	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&connector->cec.mutex);
> > > +
> > > +	if (connector->cec.funcs) {
> > > +		ret = -EBUSY;
> > > +		goto err_unlock;
> > > +	}
> > > +
> > > +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> > > +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> > > +					available_las ? : CEC_MAX_LOG_ADDRS);
> > > +	ret = PTR_ERR_OR_ZERO(cec_adap);
> > > +	if (ret < 0)
> > > +		goto err_unlock;
> > > +
> > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > +	cec_s_conn_info(cec_adap, &conn_info);
> > > +
> > > +	connector->cec.adapter = cec_adap;
> > > +	connector->cec.funcs = &ops->base;
> > > +
> > > +	ret = ops->init(connector);
> > > +	if (ret < 0)
> > > +		goto err_delete_adapter;
> > > +
> > > +	ret = cec_register_adapter(cec_adap, dev);
> > > +	if (ret < 0)
> > > +		goto err_delete_adapter;
> > > +
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +
> > > +	return 0;
> > > +
> > > +err_delete_adapter:
> > > +	cec_delete_adapter(cec_adap);
> > > +
> > > +	connector->cec.adapter = NULL;
> > > +
> > > +err_unlock:
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
> > > +
> > > +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
> > > +					 struct cec_msg *msg)
> > > +{
> > > +	cec_received_msg(connector->cec.adapter, msg);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
> > > +
> > > +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
> > > +						  u8 status)
> > > +{
> > > +	cec_transmit_attempt_done(connector->cec.adapter, status);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
> > > +
> > > +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
> > > +					  u8 status,
> > > +					  u8 arb_lost_cnt, u8 nack_cnt,
> > > +					  u8 low_drive_cnt, u8 error_cnt)
> > > +{
> > > +	cec_transmit_done(connector->cec.adapter, status,
> > > +			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
> > > +
> > > +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
> > > +{
> > > +	mutex_lock(&connector->cec.mutex);
> > > +
> > > +	cec_phys_addr_invalidate(connector->cec.adapter);
> > > +	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
> > > +
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
> > > +
> > > +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
> > > +{
> > > +	mutex_lock(&connector->cec.mutex);
> > > +
> > > +	cec_s_phys_addr(connector->cec.adapter,
> > > +			connector->display_info.source_physical_address, false);
> > > +	cec_notifier_set_phys_addr(connector->cec.notifier,
> > > +				   connector->display_info.source_physical_address);
> > > +
> > > +	mutex_unlock(&connector->cec.mutex);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
> > > diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
> > > --- /dev/null
> > > +++ b/include/drm/display/drm_hdmi_cec_helper.h
> > > @@ -0,0 +1,61 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +
> > > +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
> > > +#define DRM_DISPLAY_HDMI_CEC_HELPER
> > > +
> > > +#include <drm/drm_connector.h>
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +struct drm_connector;
> > > +
> > > +struct cec_msg;
> > > +struct device;
> > > +
> > > +struct drm_connector_hdmi_cec_adapter_ops {
> > > +	struct drm_connector_cec_funcs base;
> > > +
> > > +	int (*init)(struct drm_connector *connector);
> > > +	void (*uninit)(struct drm_connector *connector);
> > > +
> > > +	int (*enable)(struct drm_connector *connector, bool enable);
> > > +	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
> > > +	int (*transmit)(struct drm_connector *connector, u8 attempts,
> > > +			u32 signal_free_time, struct cec_msg *msg);
> > > +};
> > 
> > Why can't we merge drm_connector_cec_funcs and
> > drm_connector_cec_adapter_ops? They look equivalent to me?
> 
> Well, not exactly. The funcs is a generic interface. Notifiers do not
> need the adapter_ops. And cec_pin (sun4i) would also require a different
> set of callbacks. Thus I decided that it's easier to subclass funcs
> instead of adding all possible callbacks there.

There's two things here: cec_pin and cec_adapter are equivalent. They
provide the same feature, but one relies on an hardware controller, the
other bitbangs a GPIO. They are also mutually exclusive.

So I'd very much expect a different set of hooks there.

Then we have the notifiers.

AFAIK, it's useful when you don't have a CEC hardware and need to
coordinate with an external block providing the feature. Again, I'm not
sure why we should share anything there.

Can't we just duplicate the function names?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 02/10] drm/display: add CEC helpers code

[Dmitry Baryshkov]

On Wed, Feb 05, 2025 at 03:27:05PM +0100, Maxime Ripard wrote:
> On Tue, Jan 28, 2025 at 02:17:19PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Jan 28, 2025 at 11:36:06AM +0100, Maxime Ripard wrote:
> > > On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote:
> > > > Add generic CEC helpers to be used by HDMI drivers. Both notifier and
> > > > and adapter are supported for registration. Once registered, the driver
> > > > can call common set of functions to update physical address, to
> > > > invalidate it or to unregister CEC data.
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/display/Kconfig               |   5 +
> > > >  drivers/gpu/drm/display/Makefile              |   2 +
> > > >  drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
> > > >  include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
> > > >  4 files changed, 277 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> > > > index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
> > > > --- a/drivers/gpu/drm/display/Kconfig
> > > > +++ b/drivers/gpu/drm/display/Kconfig
> > > > @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
> > > >  	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
> > > >  	  implementation).
> > > >  
> > > > +config DRM_DISPLAY_HDMI_CEC_HELPER
> > > > +	bool
> > > > +	help
> > > > +	  DRM display helpers for HDMI CEC implementation.
> > > > +
> > > >  config DRM_DISPLAY_HDMI_HELPER
> > > >  	bool
> > > >  	help
> > > > diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> > > > index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
> > > > --- a/drivers/gpu/drm/display/Makefile
> > > > +++ b/drivers/gpu/drm/display/Makefile
> > > > @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
> > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
> > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
> > > >  	drm_hdmi_audio_helper.o
> > > > +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
> > > > +	drm_hdmi_cec_helper.o
> > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
> > > >  	drm_hdmi_helper.o \
> > > >  	drm_scdc_helper.o
> > > > diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > > @@ -0,0 +1,209 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright (c) 2024 Linaro Ltd
> > > > + */
> > > > +
> > > > +#include <drm/drm_bridge.h>
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/display/drm_hdmi_cec_helper.h>
> > > > +
> > > > +#include <linux/mutex.h>
> > > > +
> > > > +#include <media/cec.h>
> > > > +#include <media/cec-notifier.h>
> > > > +
> > > > +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
> > > > +{
> > > > +	cec_unregister_adapter(connector->cec.adapter);
> > > > +	connector->cec.adapter = NULL;
> > > > +
> > > > +	cec_notifier_conn_unregister(connector->cec.notifier);
> > > > +	connector->cec.notifier = NULL;
> > > > +
> > > > +	connector->cec.funcs = NULL;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
> > > > +
> > > > +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
> > > > +	.unregister = drm_connector_hdmi_cec_unregister,
> > > > +};
> > > > +
> > > > +int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
> > > > +					     const char *port_name,
> > > > +					     struct device *dev)
> > > > +{
> > > > +	struct cec_connector_info conn_info;
> > > > +	struct cec_notifier *notifier;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&connector->cec.mutex);
> > > > +
> > > > +	if (connector->cec.funcs) {
> > > > +		ret = -EBUSY;
> > > > +		goto err_unlock;
> > > > +	}
> > > > +
> > > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > > +
> > > > +	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
> > > > +	if (!notifier) {
> > > > +		ret = -ENOMEM;
> > > > +		goto err_unlock;
> > > > +	}
> > > > +
> > > > +	connector->cec.notifier = notifier;
> > > > +	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
> > > > +
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_unlock:
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
> > > > +
> > > > +#define to_hdmi_cec_adapter_ops(ops) \
> > > > +	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
> > > > +
> > > > +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > > > +{
> > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > +
> > > > +	return ops->enable(connector, enable);
> > > > +}
> > > > +
> > > > +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > > > +{
> > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > +
> > > > +	return ops->log_addr(connector, logical_addr);
> > > > +}
> > > > +
> > > > +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > > > +						u32 signal_free_time, struct cec_msg *msg)
> > > > +{
> > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > +
> > > > +	return ops->transmit(connector, attempts, signal_free_time, msg);
> > > > +}
> > > > +
> > > > +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
> > > > +	.adap_enable = drm_connector_hdmi_cec_adap_enable,
> > > > +	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
> > > > +	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
> > > > +};
> > > > +
> > > > +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> > > > +				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
> > > > +				    const char *name,
> > > > +				    u8 available_las,
> > > > +				    struct device *dev)
> > > > +{
> > > > +	struct cec_connector_info conn_info;
> > > > +	struct cec_adapter *cec_adap;
> > > > +	int ret;
> > > > +
> > > > +	if (!ops->base.unregister ||
> > > > +	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&connector->cec.mutex);
> > > > +
> > > > +	if (connector->cec.funcs) {
> > > > +		ret = -EBUSY;
> > > > +		goto err_unlock;
> > > > +	}
> > > > +
> > > > +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> > > > +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> > > > +					available_las ? : CEC_MAX_LOG_ADDRS);
> > > > +	ret = PTR_ERR_OR_ZERO(cec_adap);
> > > > +	if (ret < 0)
> > > > +		goto err_unlock;
> > > > +
> > > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > > +	cec_s_conn_info(cec_adap, &conn_info);
> > > > +
> > > > +	connector->cec.adapter = cec_adap;
> > > > +	connector->cec.funcs = &ops->base;
> > > > +
> > > > +	ret = ops->init(connector);
> > > > +	if (ret < 0)
> > > > +		goto err_delete_adapter;
> > > > +
> > > > +	ret = cec_register_adapter(cec_adap, dev);
> > > > +	if (ret < 0)
> > > > +		goto err_delete_adapter;
> > > > +
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_delete_adapter:
> > > > +	cec_delete_adapter(cec_adap);
> > > > +
> > > > +	connector->cec.adapter = NULL;
> > > > +
> > > > +err_unlock:
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
> > > > +
> > > > +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
> > > > +					 struct cec_msg *msg)
> > > > +{
> > > > +	cec_received_msg(connector->cec.adapter, msg);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
> > > > +
> > > > +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
> > > > +						  u8 status)
> > > > +{
> > > > +	cec_transmit_attempt_done(connector->cec.adapter, status);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
> > > > +
> > > > +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
> > > > +					  u8 status,
> > > > +					  u8 arb_lost_cnt, u8 nack_cnt,
> > > > +					  u8 low_drive_cnt, u8 error_cnt)
> > > > +{
> > > > +	cec_transmit_done(connector->cec.adapter, status,
> > > > +			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
> > > > +
> > > > +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
> > > > +{
> > > > +	mutex_lock(&connector->cec.mutex);
> > > > +
> > > > +	cec_phys_addr_invalidate(connector->cec.adapter);
> > > > +	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
> > > > +
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
> > > > +
> > > > +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
> > > > +{
> > > > +	mutex_lock(&connector->cec.mutex);
> > > > +
> > > > +	cec_s_phys_addr(connector->cec.adapter,
> > > > +			connector->display_info.source_physical_address, false);
> > > > +	cec_notifier_set_phys_addr(connector->cec.notifier,
> > > > +				   connector->display_info.source_physical_address);
> > > > +
> > > > +	mutex_unlock(&connector->cec.mutex);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
> > > > diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
> > > > --- /dev/null
> > > > +++ b/include/drm/display/drm_hdmi_cec_helper.h
> > > > @@ -0,0 +1,61 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +
> > > > +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
> > > > +#define DRM_DISPLAY_HDMI_CEC_HELPER
> > > > +
> > > > +#include <drm/drm_connector.h>
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct drm_connector;
> > > > +
> > > > +struct cec_msg;
> > > > +struct device;
> > > > +
> > > > +struct drm_connector_hdmi_cec_adapter_ops {
> > > > +	struct drm_connector_cec_funcs base;
> > > > +
> > > > +	int (*init)(struct drm_connector *connector);
> > > > +	void (*uninit)(struct drm_connector *connector);
> > > > +
> > > > +	int (*enable)(struct drm_connector *connector, bool enable);
> > > > +	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
> > > > +	int (*transmit)(struct drm_connector *connector, u8 attempts,
> > > > +			u32 signal_free_time, struct cec_msg *msg);
> > > > +};
> > > 
> > > Why can't we merge drm_connector_cec_funcs and
> > > drm_connector_cec_adapter_ops? They look equivalent to me?
> > 
> > Well, not exactly. The funcs is a generic interface. Notifiers do not
> > need the adapter_ops. And cec_pin (sun4i) would also require a different
> > set of callbacks. Thus I decided that it's easier to subclass funcs
> > instead of adding all possible callbacks there.
> 
> There's two things here: cec_pin and cec_adapter are equivalent. They
> provide the same feature, but one relies on an hardware controller, the
> other bitbangs a GPIO. They are also mutually exclusive.
> 
> So I'd very much expect a different set of hooks there.

That's the point. Normal CEC adapters would need to implement these
callbacks. CEC pin adapter will have a different set of callbacks.
Notifiers do not need any extra callbacks at all.

Maybe I should restructure the patchset, pushing helpers closer to the
end of the series.

> Then we have the notifiers.
> 
> AFAIK, it's useful when you don't have a CEC hardware and need to
> coordinate with an external block providing the feature. Again, I'm not
> sure why we should share anything there.
> 
> Can't we just duplicate the function names?

Which function names?



-- 
With best wishes
Dmitry

Re: [PATCH v3 02/10] drm/display: add CEC helpers code

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 13648 bytes --]

On Wed, Feb 05, 2025 at 05:01:09PM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 05, 2025 at 03:27:05PM +0100, Maxime Ripard wrote:
> > On Tue, Jan 28, 2025 at 02:17:19PM +0200, Dmitry Baryshkov wrote:
> > > On Tue, Jan 28, 2025 at 11:36:06AM +0100, Maxime Ripard wrote:
> > > > On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote:
> > > > > Add generic CEC helpers to be used by HDMI drivers. Both notifier and
> > > > > and adapter are supported for registration. Once registered, the driver
> > > > > can call common set of functions to update physical address, to
> > > > > invalidate it or to unregister CEC data.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/display/Kconfig               |   5 +
> > > > >  drivers/gpu/drm/display/Makefile              |   2 +
> > > > >  drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 ++++++++++++++++++++++++++
> > > > >  include/drm/display/drm_hdmi_cec_helper.h     |  61 ++++++++
> > > > >  4 files changed, 277 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> > > > > index 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c 100644
> > > > > --- a/drivers/gpu/drm/display/Kconfig
> > > > > +++ b/drivers/gpu/drm/display/Kconfig
> > > > > @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER
> > > > >  	  DRM display helpers for HDMI Audio functionality (generic HDMI Codec
> > > > >  	  implementation).
> > > > >  
> > > > > +config DRM_DISPLAY_HDMI_CEC_HELPER
> > > > > +	bool
> > > > > +	help
> > > > > +	  DRM display helpers for HDMI CEC implementation.
> > > > > +
> > > > >  config DRM_DISPLAY_HDMI_HELPER
> > > > >  	bool
> > > > >  	help
> > > > > diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> > > > > index b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 100644
> > > > > --- a/drivers/gpu/drm/display/Makefile
> > > > > +++ b/drivers/gpu/drm/display/Makefile
> > > > > @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \
> > > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o
> > > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \
> > > > >  	drm_hdmi_audio_helper.o
> > > > > +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \
> > > > > +	drm_hdmi_cec_helper.o
> > > > >  drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
> > > > >  	drm_hdmi_helper.o \
> > > > >  	drm_scdc_helper.o
> > > > > diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
> > > > > @@ -0,0 +1,209 @@
> > > > > +// SPDX-License-Identifier: MIT
> > > > > +/*
> > > > > + * Copyright (c) 2024 Linaro Ltd
> > > > > + */
> > > > > +
> > > > > +#include <drm/drm_bridge.h>
> > > > > +#include <drm/drm_connector.h>
> > > > > +#include <drm/display/drm_hdmi_cec_helper.h>
> > > > > +
> > > > > +#include <linux/mutex.h>
> > > > > +
> > > > > +#include <media/cec.h>
> > > > > +#include <media/cec-notifier.h>
> > > > > +
> > > > > +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector)
> > > > > +{
> > > > > +	cec_unregister_adapter(connector->cec.adapter);
> > > > > +	connector->cec.adapter = NULL;
> > > > > +
> > > > > +	cec_notifier_conn_unregister(connector->cec.notifier);
> > > > > +	connector->cec.notifier = NULL;
> > > > > +
> > > > > +	connector->cec.funcs = NULL;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister);
> > > > > +
> > > > > +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = {
> > > > > +	.unregister = drm_connector_hdmi_cec_unregister,
> > > > > +};
> > > > > +
> > > > > +int drm_connector_hdmi_cec_notifier_register(struct drm_connector *connector,
> > > > > +					     const char *port_name,
> > > > > +					     struct device *dev)
> > > > > +{
> > > > > +	struct cec_connector_info conn_info;
> > > > > +	struct cec_notifier *notifier;
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&connector->cec.mutex);
> > > > > +
> > > > > +	if (connector->cec.funcs) {
> > > > > +		ret = -EBUSY;
> > > > > +		goto err_unlock;
> > > > > +	}
> > > > > +
> > > > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > > > +
> > > > > +	notifier = cec_notifier_conn_register(dev, port_name, &conn_info);
> > > > > +	if (!notifier) {
> > > > > +		ret = -ENOMEM;
> > > > > +		goto err_unlock;
> > > > > +	}
> > > > > +
> > > > > +	connector->cec.notifier = notifier;
> > > > > +	connector->cec.funcs = &drm_connector_hdmi_cec_funcs;
> > > > > +
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err_unlock:
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register);
> > > > > +
> > > > > +#define to_hdmi_cec_adapter_ops(ops) \
> > > > > +	container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base)
> > > > > +
> > > > > +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > > > > +{
> > > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > > +
> > > > > +	return ops->enable(connector, enable);
> > > > > +}
> > > > > +
> > > > > +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > > > > +{
> > > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > > +
> > > > > +	return ops->log_addr(connector, logical_addr);
> > > > > +}
> > > > > +
> > > > > +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > > > > +						u32 signal_free_time, struct cec_msg *msg)
> > > > > +{
> > > > > +	struct drm_connector *connector = cec_get_drvdata(adap);
> > > > > +	struct drm_connector_hdmi_cec_adapter_ops *ops =
> > > > > +		to_hdmi_cec_adapter_ops(connector->cec.funcs);
> > > > > +
> > > > > +	return ops->transmit(connector, attempts, signal_free_time, msg);
> > > > > +}
> > > > > +
> > > > > +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = {
> > > > > +	.adap_enable = drm_connector_hdmi_cec_adap_enable,
> > > > > +	.adap_log_addr = drm_connector_hdmi_cec_adap_log_addr,
> > > > > +	.adap_transmit = drm_connector_hdmi_cec_adap_transmit,
> > > > > +};
> > > > > +
> > > > > +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> > > > > +				    const struct drm_connector_hdmi_cec_adapter_ops *ops,
> > > > > +				    const char *name,
> > > > > +				    u8 available_las,
> > > > > +				    struct device *dev)
> > > > > +{
> > > > > +	struct cec_connector_info conn_info;
> > > > > +	struct cec_adapter *cec_adap;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!ops->base.unregister ||
> > > > > +	    !ops->init || !ops->enable || !ops->log_addr || !ops->transmit)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	mutex_lock(&connector->cec.mutex);
> > > > > +
> > > > > +	if (connector->cec.funcs) {
> > > > > +		ret = -EBUSY;
> > > > > +		goto err_unlock;
> > > > > +	}
> > > > > +
> > > > > +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> > > > > +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> > > > > +					available_las ? : CEC_MAX_LOG_ADDRS);
> > > > > +	ret = PTR_ERR_OR_ZERO(cec_adap);
> > > > > +	if (ret < 0)
> > > > > +		goto err_unlock;
> > > > > +
> > > > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > > > +	cec_s_conn_info(cec_adap, &conn_info);
> > > > > +
> > > > > +	connector->cec.adapter = cec_adap;
> > > > > +	connector->cec.funcs = &ops->base;
> > > > > +
> > > > > +	ret = ops->init(connector);
> > > > > +	if (ret < 0)
> > > > > +		goto err_delete_adapter;
> > > > > +
> > > > > +	ret = cec_register_adapter(cec_adap, dev);
> > > > > +	if (ret < 0)
> > > > > +		goto err_delete_adapter;
> > > > > +
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err_delete_adapter:
> > > > > +	cec_delete_adapter(cec_adap);
> > > > > +
> > > > > +	connector->cec.adapter = NULL;
> > > > > +
> > > > > +err_unlock:
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_register);
> > > > > +
> > > > > +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector,
> > > > > +					 struct cec_msg *msg)
> > > > > +{
> > > > > +	cec_received_msg(connector->cec.adapter, msg);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg);
> > > > > +
> > > > > +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector *connector,
> > > > > +						  u8 status)
> > > > > +{
> > > > > +	cec_transmit_attempt_done(connector->cec.adapter, status);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done);
> > > > > +
> > > > > +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector,
> > > > > +					  u8 status,
> > > > > +					  u8 arb_lost_cnt, u8 nack_cnt,
> > > > > +					  u8 low_drive_cnt, u8 error_cnt)
> > > > > +{
> > > > > +	cec_transmit_done(connector->cec.adapter, status,
> > > > > +			  arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done);
> > > > > +
> > > > > +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector *connector)
> > > > > +{
> > > > > +	mutex_lock(&connector->cec.mutex);
> > > > > +
> > > > > +	cec_phys_addr_invalidate(connector->cec.adapter);
> > > > > +	cec_notifier_phys_addr_invalidate(connector->cec.notifier);
> > > > > +
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate);
> > > > > +
> > > > > +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
> > > > > +{
> > > > > +	mutex_lock(&connector->cec.mutex);
> > > > > +
> > > > > +	cec_s_phys_addr(connector->cec.adapter,
> > > > > +			connector->display_info.source_physical_address, false);
> > > > > +	cec_notifier_set_phys_addr(connector->cec.notifier,
> > > > > +				   connector->display_info.source_physical_address);
> > > > > +
> > > > > +	mutex_unlock(&connector->cec.mutex);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
> > > > > diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19
> > > > > --- /dev/null
> > > > > +++ b/include/drm/display/drm_hdmi_cec_helper.h
> > > > > @@ -0,0 +1,61 @@
> > > > > +/* SPDX-License-Identifier: MIT */
> > > > > +
> > > > > +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER
> > > > > +#define DRM_DISPLAY_HDMI_CEC_HELPER
> > > > > +
> > > > > +#include <drm/drm_connector.h>
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +
> > > > > +struct drm_connector;
> > > > > +
> > > > > +struct cec_msg;
> > > > > +struct device;
> > > > > +
> > > > > +struct drm_connector_hdmi_cec_adapter_ops {
> > > > > +	struct drm_connector_cec_funcs base;
> > > > > +
> > > > > +	int (*init)(struct drm_connector *connector);
> > > > > +	void (*uninit)(struct drm_connector *connector);
> > > > > +
> > > > > +	int (*enable)(struct drm_connector *connector, bool enable);
> > > > > +	int (*log_addr)(struct drm_connector *connector, u8 logical_addr);
> > > > > +	int (*transmit)(struct drm_connector *connector, u8 attempts,
> > > > > +			u32 signal_free_time, struct cec_msg *msg);
> > > > > +};
> > > > 
> > > > Why can't we merge drm_connector_cec_funcs and
> > > > drm_connector_cec_adapter_ops? They look equivalent to me?
> > > 
> > > Well, not exactly. The funcs is a generic interface. Notifiers do not
> > > need the adapter_ops. And cec_pin (sun4i) would also require a different
> > > set of callbacks. Thus I decided that it's easier to subclass funcs
> > > instead of adding all possible callbacks there.
> > 
> > There's two things here: cec_pin and cec_adapter are equivalent. They
> > provide the same feature, but one relies on an hardware controller, the
> > other bitbangs a GPIO. They are also mutually exclusive.
> > 
> > So I'd very much expect a different set of hooks there.
> 
> That's the point. Normal CEC adapters would need to implement these
> callbacks. CEC pin adapter will have a different set of callbacks.
> Notifiers do not need any extra callbacks at all.

Sure, and that's true for most of the KMS entities we support. What I
don't get is why that dual layer func structure is needed in the first
place. We have no other entity that works that way, and we typically
solve this using a helper.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH v3 03/10] drm/display: hdmi-state-helper: handle CEC physical address

[Dmitry Baryshkov]

Call HDMI CEC helpers in order to update physical address of the
adapter.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 2691e8b3e480131ac6e4e4b74b24947be55694bd..1e7ea9b387088d5f407b647a9a3fead7a2929a30 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -6,6 +6,7 @@
 #include <drm/drm_print.h>
 
 #include <drm/display/drm_hdmi_audio_helper.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
@@ -786,8 +787,11 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
 	const struct drm_edid *drm_edid;
 
 	if (status == connector_status_disconnected) {
-		// TODO: also handle CEC and scramber, HDMI sink disconnected.
+		// TODO: also handle scramber, HDMI sink disconnected.
 		drm_connector_hdmi_audio_plugged_notify(connector, false);
+		drm_edid_connector_update(connector, NULL);
+		drm_connector_hdmi_cec_phys_addr_invalidate(connector);
+		return;
 	}
 
 	if (connector->hdmi.funcs->read_edid)
@@ -800,8 +804,9 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
 	drm_edid_free(drm_edid);
 
 	if (status == connector_status_connected) {
-		// TODO: also handle CEC and scramber, HDMI sink is now connected.
+		// TODO: also handle scramber, HDMI sink is now connected.
 		drm_connector_hdmi_audio_plugged_notify(connector, true);
+		drm_connector_hdmi_cec_phys_addr_set(connector);
 	}
 }
 

-- 
2.39.5

Re: [PATCH v3 03/10] drm/display: hdmi-state-helper: handle CEC physical address

[Dmitry Baryshkov]

On Sun, Jan 26, 2025 at 03:29:08PM +0200, Dmitry Baryshkov wrote:
> Call HDMI CEC helpers in order to update physical address of the
> adapter.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..1e7ea9b387088d5f407b647a9a3fead7a2929a30 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -6,6 +6,7 @@
>  #include <drm/drm_print.h>
>  
>  #include <drm/display/drm_hdmi_audio_helper.h>
> +#include <drm/display/drm_hdmi_cec_helper.h>
>  #include <drm/display/drm_hdmi_helper.h>
>  #include <drm/display/drm_hdmi_state_helper.h>
>  
> @@ -786,8 +787,11 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>  	const struct drm_edid *drm_edid;
>  
>  	if (status == connector_status_disconnected) {
> -		// TODO: also handle CEC and scramber, HDMI sink disconnected.
> +		// TODO: also handle scramber, HDMI sink disconnected.
>  		drm_connector_hdmi_audio_plugged_notify(connector, false);
> +		drm_edid_connector_update(connector, NULL);
> +		drm_connector_hdmi_cec_phys_addr_invalidate(connector);

This also has a chunk from the commit 78a5acf5433d ("drm/display: hdmi:
Do not read EDID on disconnected connectors") present only in
drm-misc-next-fixes. In future I will make sure that that commit is
included into my tree history, but the series should probably wait for
-rc1, until all -fixes branches are again a part of the drm-misc-next.

On the other hand, if anybody would like to test the series on top of
drm-misc-next (wink-wink), there would be no need to pick up any other
changes.

> +		return;
>  	}
>  
>  	if (connector->hdmi.funcs->read_edid)
> @@ -800,8 +804,9 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>  	drm_edid_free(drm_edid);
>  
>  	if (status == connector_status_connected) {
> -		// TODO: also handle CEC and scramber, HDMI sink is now connected.
> +		// TODO: also handle scramber, HDMI sink is now connected.
>  		drm_connector_hdmi_audio_plugged_notify(connector, true);
> +		drm_connector_hdmi_cec_phys_addr_set(connector);
>  	}
>  }
>  
> 
> -- 
> 2.39.5
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 03/10] drm/display: hdmi-state-helper: handle CEC physical address

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

On Sun, Jan 26, 2025 at 03:29:08PM +0200, Dmitry Baryshkov wrote:
> Call HDMI CEC helpers in order to update physical address of the
> adapter.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH v3 04/10] drm/vc4: hdmi: switch to generic CEC helpers

[Dmitry Baryshkov]

Switch VC4 driver to using CEC helpers code, simplifying hotplug and
registration / cleanup. The existing vc4_hdmi_cec_release() is kept for
now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/vc4/Kconfig    |   1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c | 138 ++++++++++++++++-------------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |   1 -
 3 files changed, 56 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
index 123ab0ce178157c3b39466f87c7ac39c8470f329..bb8c40be325033632d3e94db87a16b03554ad3af 100644
--- a/drivers/gpu/drm/vc4/Kconfig
+++ b/drivers/gpu/drm/vc4/Kconfig
@@ -35,6 +35,7 @@ config DRM_VC4_HDMI_CEC
 	bool "Broadcom VC4 HDMI CEC Support"
 	depends on DRM_VC4
 	select CEC_CORE
+	select DRM_DISPLAY_HDMI_CEC_HELPER
 	help
 	  Choose this option if you have a Broadcom VC4 GPU
 	  and want to use CEC.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 47d9ada98430634cfd8c1e21c2a4d00d501bab7e..1108983c44858382cb9f09b686956903645ebe0a 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -32,6 +32,7 @@
  */
 
 #include <drm/display/drm_hdmi_audio_helper.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 #include <drm/display/drm_scdc_helper.h>
@@ -400,16 +401,8 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 	 * the lock for now.
 	 */
 
-	if (status == connector_status_disconnected) {
-		cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
-		return;
-	}
-
 	drm_atomic_helper_connector_hdmi_hotplug(connector, status);
 
-	cec_s_phys_addr(vc4_hdmi->cec_adap,
-			connector->display_info.source_physical_address, false);
-
 	if (status != connector_status_connected)
 		return;
 
@@ -2388,8 +2381,8 @@ static irqreturn_t vc4_cec_irq_handler_rx_thread(int irq, void *priv)
 	struct vc4_hdmi *vc4_hdmi = priv;
 
 	if (vc4_hdmi->cec_rx_msg.len)
-		cec_received_msg(vc4_hdmi->cec_adap,
-				 &vc4_hdmi->cec_rx_msg);
+		drm_connector_hdmi_cec_received_msg(&vc4_hdmi->connector,
+						    &vc4_hdmi->cec_rx_msg);
 
 	return IRQ_HANDLED;
 }
@@ -2399,15 +2392,17 @@ static irqreturn_t vc4_cec_irq_handler_tx_thread(int irq, void *priv)
 	struct vc4_hdmi *vc4_hdmi = priv;
 
 	if (vc4_hdmi->cec_tx_ok) {
-		cec_transmit_done(vc4_hdmi->cec_adap, CEC_TX_STATUS_OK,
-				  0, 0, 0, 0);
+		drm_connector_hdmi_cec_transmit_done(&vc4_hdmi->connector,
+						     CEC_TX_STATUS_OK,
+						     0, 0, 0, 0);
 	} else {
 		/*
 		 * This CEC implementation makes 1 retry, so if we
 		 * get a NACK, then that means it made 2 attempts.
 		 */
-		cec_transmit_done(vc4_hdmi->cec_adap, CEC_TX_STATUS_NACK,
-				  0, 2, 0, 0);
+		drm_connector_hdmi_cec_transmit_done(&vc4_hdmi->connector,
+						     CEC_TX_STATUS_NACK,
+						     0, 2, 0, 0);
 	}
 	return IRQ_HANDLED;
 }
@@ -2564,9 +2559,9 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
 	return ret;
 }
 
-static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
+static int vc4_hdmi_cec_enable(struct drm_connector *connector)
 {
-	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	/* clock period in microseconds */
 	const u32 usecs = 1000000 / CEC_CLOCK_FREQ;
@@ -2631,9 +2626,9 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 	return 0;
 }
 
-static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
+static int vc4_hdmi_cec_disable(struct drm_connector *connector)
 {
-	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	unsigned long flags;
 	int idx;
@@ -2667,17 +2662,17 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 	return 0;
 }
 
-static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+static int vc4_hdmi_cec_adap_enable(struct drm_connector *connector, bool enable)
 {
 	if (enable)
-		return vc4_hdmi_cec_enable(adap);
+		return vc4_hdmi_cec_enable(connector);
 	else
-		return vc4_hdmi_cec_disable(adap);
+		return vc4_hdmi_cec_disable(connector);
 }
 
-static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
+static int vc4_hdmi_cec_adap_log_addr(struct drm_connector *connector, u8 log_addr)
 {
-	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	struct drm_device *drm = vc4_hdmi->connector.dev;
 	unsigned long flags;
 	int idx;
@@ -2703,10 +2698,10 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 	return 0;
 }
 
-static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+static int vc4_hdmi_cec_adap_transmit(struct drm_connector *connector, u8 attempts,
 				      u32 signal_free_time, struct cec_msg *msg)
 {
-	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	struct drm_device *dev = vc4_hdmi->connector.dev;
 	unsigned long flags;
 	u32 val;
@@ -2749,84 +2744,66 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	return 0;
 }
 
-static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = {
-	.adap_enable = vc4_hdmi_cec_adap_enable,
-	.adap_log_addr = vc4_hdmi_cec_adap_log_addr,
-	.adap_transmit = vc4_hdmi_cec_adap_transmit,
-};
-
-static void vc4_hdmi_cec_release(void *ptr)
-{
-	struct vc4_hdmi *vc4_hdmi = ptr;
-
-	cec_unregister_adapter(vc4_hdmi->cec_adap);
-	vc4_hdmi->cec_adap = NULL;
-}
-
-static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
+static int vc4_hdmi_cec_init(struct drm_connector *connector)
 {
-	struct cec_connector_info conn_info;
+	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	struct platform_device *pdev = vc4_hdmi->pdev;
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	if (!of_property_present(dev->of_node, "interrupts")) {
-		dev_warn(dev, "'interrupts' DT property is missing, no CEC\n");
-		return 0;
-	}
-
-	vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
-						  vc4_hdmi,
-						  vc4_hdmi->variant->card_name,
-						  CEC_CAP_DEFAULTS |
-						  CEC_CAP_CONNECTOR_INFO, 1);
-	ret = PTR_ERR_OR_ZERO(vc4_hdmi->cec_adap);
-	if (ret < 0)
-		return ret;
-
-	cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector);
-	cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
-
 	if (vc4_hdmi->variant->external_irq_controller) {
 		ret = devm_request_threaded_irq(dev, platform_get_irq_byname(pdev, "cec-rx"),
 						vc4_cec_irq_handler_rx_bare,
 						vc4_cec_irq_handler_rx_thread, 0,
 						"vc4 hdmi cec rx", vc4_hdmi);
 		if (ret)
-			goto err_delete_cec_adap;
+			return ret;
 
 		ret = devm_request_threaded_irq(dev, platform_get_irq_byname(pdev, "cec-tx"),
 						vc4_cec_irq_handler_tx_bare,
 						vc4_cec_irq_handler_tx_thread, 0,
 						"vc4 hdmi cec tx", vc4_hdmi);
 		if (ret)
-			goto err_delete_cec_adap;
+			return ret;
 	} else {
 		ret = devm_request_threaded_irq(dev, platform_get_irq(pdev, 0),
 						vc4_cec_irq_handler,
 						vc4_cec_irq_handler_thread, 0,
 						"vc4 hdmi cec", vc4_hdmi);
 		if (ret)
-			goto err_delete_cec_adap;
+			return ret;
 	}
 
-	ret = cec_register_adapter(vc4_hdmi->cec_adap, &pdev->dev);
-	if (ret < 0)
-		goto err_delete_cec_adap;
+	return 0;
+}
+
+static const struct drm_connector_hdmi_cec_adapter_ops vc4_hdmi_cec_adap_ops = {
+	.base.unregister = drm_connector_hdmi_cec_unregister,
+	.init = vc4_hdmi_cec_init,
+	.enable = vc4_hdmi_cec_adap_enable,
+	.log_addr = vc4_hdmi_cec_adap_log_addr,
+	.transmit = vc4_hdmi_cec_adap_transmit,
+};
+
+static int vc4_hdmi_cec_register(struct vc4_hdmi *vc4_hdmi)
+{
+	struct platform_device *pdev = vc4_hdmi->pdev;
+	struct device *dev = &pdev->dev;
+
+	if (!of_property_present(dev->of_node, "interrupts")) {
+		dev_warn(dev, "'interrupts' DT property is missing, no CEC\n");
+		return 0;
+	}
 
 	/*
-	 * NOTE: Strictly speaking, we should probably use a DRM-managed
-	 * registration there to avoid removing the CEC adapter by the
-	 * time the DRM driver doesn't have any user anymore.
+	 * NOTE: the CEC adapter will be unregistered from
+	 * drm_connector_cleanup(), which is called from drm_dev_unplug()
+	 * during device unbind.
 	 *
 	 * However, the CEC framework already cleans up the CEC adapter
 	 * only when the last user has closed its file descriptor, so we
 	 * don't need to handle it in DRM.
 	 *
-	 * By the time the device-managed hook is executed, we will give
-	 * up our reference to the CEC adapter and therefore don't
-	 * really care when it's actually freed.
-	 *
 	 * There's still a problematic sequence: if we unregister our
 	 * CEC adapter, but the userspace keeps a handle on the CEC
 	 * adapter but not the DRM device for some reason. In such a
@@ -2837,19 +2814,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	 * the CEC framework already handles this too, by calling
 	 * cec_is_registered() in cec_ioctl() and cec_poll().
 	 */
-	ret = devm_add_action_or_reset(dev, vc4_hdmi_cec_release, vc4_hdmi);
-	if (ret)
-		return ret;
-
-	return 0;
-
-err_delete_cec_adap:
-	cec_delete_adapter(vc4_hdmi->cec_adap);
-
-	return ret;
+	return drm_connector_hdmi_cec_register(&vc4_hdmi->connector,
+					       &vc4_hdmi_cec_adap_ops,
+					       vc4_hdmi->variant->card_name,
+					       1,
+					       &pdev->dev);
 }
 #else
-static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
+static int vc4_hdmi_cec_register(struct vc4_hdmi *vc4_hdmi)
 {
 	return 0;
 }
@@ -3271,7 +3243,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		goto err_put_runtime_pm;
 
-	ret = vc4_hdmi_cec_init(vc4_hdmi);
+	ret = vc4_hdmi_cec_register(vc4_hdmi);
 	if (ret)
 		goto err_put_runtime_pm;
 
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..5acbe27fb57659d02f32ca571dd3ded4a1a0d9dc 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -146,7 +146,6 @@ struct vc4_hdmi {
 	 */
 	bool disable_wifi_frequencies;
 
-	struct cec_adapter *cec_adap;
 	struct cec_msg cec_rx_msg;
 	bool cec_tx_ok;
 	bool cec_irq_was_rx;

-- 
2.39.5

[PATCH v3 05/10] drm/display: bridge-connector: hook in CEC notifier support

[Dmitry Baryshkov]

Allow HDMI DRM bridges to create CEC notifier. Physical address is
handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
being called from .detect() path.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/lontium-lt9611.c        |  2 +-
 drivers/gpu/drm/display/Kconfig                |  1 +
 drivers/gpu/drm/display/drm_bridge_connector.c | 11 ++++++++++-
 include/drm/drm_bridge.h                       |  9 +++++++--
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 0df2c68e45897f2f2eed10a56dd670582526180b..158a4966ac00657074b11e93dc9bcab41c16db18 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -1252,7 +1252,7 @@ static int lt9611_probe(struct i2c_client *client)
 	lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
 	lt9611->bridge.vendor = "Lontium";
 	lt9611->bridge.product = "LT9611";
-	lt9611->bridge.hdmi_audio_dev = dev;
+	lt9611->bridge.hdmi_dev = dev;
 	lt9611->bridge.hdmi_audio_max_i2s_playback_channels = 8;
 	lt9611->bridge.hdmi_audio_dai_port = 2;
 
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
index 49da9b768acf3e5f84f2cefae4bb042cfd57a50c..d35d945a3811c30247a9f3e282a16c9eedd0d4e9 100644
--- a/drivers/gpu/drm/display/Kconfig
+++ b/drivers/gpu/drm/display/Kconfig
@@ -16,6 +16,7 @@ if DRM_DISPLAY_HELPER
 config DRM_BRIDGE_CONNECTOR
 	bool
 	select DRM_DISPLAY_HDMI_AUDIO_HELPER
+	select DRM_DISPLAY_HDMI_CEC_HELPER
 	select DRM_DISPLAY_HDMI_STATE_HELPER
 	help
 	  DRM connector implementation terminating DRM bridge chains.
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 0397e62f9cbc93321caeae99982f5e3c66d308c5..9f234bc647d5c0880d4c42aea130074b7fa54573 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -20,6 +20,7 @@
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/display/drm_hdmi_audio_helper.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
@@ -616,7 +617,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 				return ERR_PTR(-EINVAL);
 
 			ret = drm_connector_hdmi_audio_init(connector,
-							    bridge->hdmi_audio_dev,
+							    bridge->hdmi_dev,
 							    &drm_bridge_connector_hdmi_audio_funcs,
 							    bridge->hdmi_audio_max_i2s_playback_channels,
 							    bridge->hdmi_audio_spdif_playback,
@@ -624,6 +625,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			if (ret)
 				return ERR_PTR(ret);
 		}
+
+		if (bridge->hdmi_cec_notifier) {
+			ret = drm_connector_hdmi_cec_notifier_register(connector,
+								       NULL,
+								       bridge->hdmi_dev);
+			if (ret)
+				return ERR_PTR(ret);
+		}
 	} else {
 		ret = drmm_connector_init(drm, connector,
 					  &drm_bridge_connector_funcs,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4b84faf14e368310dd20aa964e8178ec80aa6fa7..1f1670e3c6aac39b8b891b0d5e7e91254eb0d3a1 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -914,9 +914,9 @@ struct drm_bridge {
 	unsigned int max_bpc;
 
 	/**
-	 * @hdmi_audio_dev: device to be used as a parent for the HDMI Codec
+	 * @hdmi_dev: device to be used as a parent for the HDMI Codec
 	 */
-	struct device *hdmi_audio_dev;
+	struct device *hdmi_dev;
 
 	/**
 	 * @hdmi_audio_max_i2s_playback_channels: maximum number of playback
@@ -933,6 +933,11 @@ struct drm_bridge {
 	 * @hdmi_audio_dai_port: sound DAI port, -1 if it is not enabled
 	 */
 	int hdmi_audio_dai_port;
+
+	/**
+	 * @hdmi_cec_notifier: use this bridge to register a CEC notifier
+	 */
+	bool hdmi_cec_notifier;
 };
 
 static inline struct drm_bridge *

-- 
2.39.5

Re: [PATCH v3 05/10] drm/display: bridge-connector: hook in CEC notifier support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On Sun, Jan 26, 2025 at 03:29:10PM +0200, Dmitry Baryshkov wrote:
> Allow HDMI DRM bridges to create CEC notifier. Physical address is
> handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
> being called from .detect() path.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/bridge/lontium-lt9611.c        |  2 +-
>  drivers/gpu/drm/display/Kconfig                |  1 +
>  drivers/gpu/drm/display/drm_bridge_connector.c | 11 ++++++++++-
>  include/drm/drm_bridge.h                       |  9 +++++++--
>  4 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index 0df2c68e45897f2f2eed10a56dd670582526180b..158a4966ac00657074b11e93dc9bcab41c16db18 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -1252,7 +1252,7 @@ static int lt9611_probe(struct i2c_client *client)
>  	lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>  	lt9611->bridge.vendor = "Lontium";
>  	lt9611->bridge.product = "LT9611";
> -	lt9611->bridge.hdmi_audio_dev = dev;
> +	lt9611->bridge.hdmi_dev = dev;
>  	lt9611->bridge.hdmi_audio_max_i2s_playback_channels = 8;
>  	lt9611->bridge.hdmi_audio_dai_port = 2;

This looks unrleated?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 05/10] drm/display: bridge-connector: hook in CEC notifier support

[Dmitry Baryshkov]

On Tue, 28 Jan 2025 at 18:15, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Sun, Jan 26, 2025 at 03:29:10PM +0200, Dmitry Baryshkov wrote:
> > Allow HDMI DRM bridges to create CEC notifier. Physical address is
> > handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
> > being called from .detect() path.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/lontium-lt9611.c        |  2 +-
> >  drivers/gpu/drm/display/Kconfig                |  1 +
> >  drivers/gpu/drm/display/drm_bridge_connector.c | 11 ++++++++++-
> >  include/drm/drm_bridge.h                       |  9 +++++++--
> >  4 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > index 0df2c68e45897f2f2eed10a56dd670582526180b..158a4966ac00657074b11e93dc9bcab41c16db18 100644
> > --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > @@ -1252,7 +1252,7 @@ static int lt9611_probe(struct i2c_client *client)
> >       lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> >       lt9611->bridge.vendor = "Lontium";
> >       lt9611->bridge.product = "LT9611";
> > -     lt9611->bridge.hdmi_audio_dev = dev;
> > +     lt9611->bridge.hdmi_dev = dev;
> >       lt9611->bridge.hdmi_audio_max_i2s_playback_channels = 8;
> >       lt9611->bridge.hdmi_audio_dai_port = 2;
>
> This looks unrleated?

No, it is related. Initially it was the 'hdmi_audio_dev' field (when
there was just audio), now it was renamed to 'hdmi_dev', as it's
common to both audio and CEC.

-- 
With best wishes
Dmitry

[PATCH v3 06/10] drm/bridge: move private data to the end of the struct

[Dmitry Baryshkov]

WHen adding HDMI fields I didn't notice the private: declaration for HPD
fields. Move private fields to the end of the struct drm_bride to have
clear distinction between private and public fields.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 include/drm/drm_bridge.h | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1f1670e3c6aac39b8b891b0d5e7e91254eb0d3a1..348778f233b06265a6ae577762c6558e69cdb396 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -872,21 +872,6 @@ struct drm_bridge {
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
 	struct i2c_adapter *ddc;
-	/** private: */
-	/**
-	 * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
-	 */
-	struct mutex hpd_mutex;
-	/**
-	 * @hpd_cb: Hot plug detection callback, registered with
-	 * drm_bridge_hpd_enable().
-	 */
-	void (*hpd_cb)(void *data, enum drm_connector_status status);
-	/**
-	 * @hpd_data: Private data passed to the Hot plug detection callback
-	 * @hpd_cb.
-	 */
-	void *hpd_data;
 
 	/**
 	 * @vendor: Vendor of the product to be used for the SPD InfoFrame
@@ -938,6 +923,22 @@ struct drm_bridge {
 	 * @hdmi_cec_notifier: use this bridge to register a CEC notifier
 	 */
 	bool hdmi_cec_notifier;
+
+	/** private: */
+	/**
+	 * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
+	 */
+	struct mutex hpd_mutex;
+	/**
+	 * @hpd_cb: Hot plug detection callback, registered with
+	 * drm_bridge_hpd_enable().
+	 */
+	void (*hpd_cb)(void *data, enum drm_connector_status status);
+	/**
+	 * @hpd_data: Private data passed to the Hot plug detection callback
+	 * @hpd_cb.
+	 */
+	void *hpd_data;
 };
 
 static inline struct drm_bridge *

-- 
2.39.5

Re: [PATCH v3 06/10] drm/bridge: move private data to the end of the struct

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Sun, Jan 26, 2025 at 03:29:11PM +0200, Dmitry Baryshkov wrote:
> WHen adding HDMI fields I didn't notice the private: declaration for HPD
> fields. Move private fields to the end of the struct drm_bride to have
> clear distinction between private and public fields.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH v3 07/10] drm/display: hdmi-cec-helper: add bridge-related functions

[Dmitry Baryshkov]

Let drm_bridge drivers handle CEC adapters in a generic way. Add a set
of helper functions to be called by DRM bridge drivers to report CEC
message-related events.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 38 +++++++++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                  | 15 +++++++++++
 include/drm/display/drm_hdmi_cec_helper.h     |  8 ++++++
 include/drm/drm_bridge.h                      | 11 ++++++++
 4 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
index a6ed5f0fc3835b013a83308f5285ea0819c5702c..8cb3bd35febd55211f7088b9f47dfc6a00c06109 100644
--- a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c
@@ -207,3 +207,41 @@ void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector)
 	mutex_unlock(&connector->cec.mutex);
 }
 EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set);
+
+void drm_bridge_cec_transmit_done(struct drm_bridge *bridge, u8 status,
+				  u8 arb_lost_cnt, u8 nack_cnt,
+				  u8 low_drive_cnt, u8 error_cnt)
+{
+	mutex_lock(&bridge->cec_mutex);
+
+	if (bridge->cec_data)
+		drm_connector_hdmi_cec_transmit_done(bridge->cec_data,
+						     status, arb_lost_cnt,
+						     nack_cnt, low_drive_cnt, error_cnt);
+
+	mutex_unlock(&bridge->cec_mutex);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_cec_transmit_done);
+
+void drm_bridge_cec_transmit_attempt_done(struct drm_bridge *bridge, u8 status)
+{
+	mutex_lock(&bridge->cec_mutex);
+
+	if (bridge->cec_data)
+		drm_connector_hdmi_cec_transmit_attempt_done(bridge->cec_data, status);
+
+	mutex_unlock(&bridge->cec_mutex);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_cec_transmit_attempt_done);
+
+void drm_bridge_cec_received_msg(struct drm_bridge *bridge,
+				 struct cec_msg *msg)
+{
+	mutex_lock(&bridge->cec_mutex);
+
+	if (bridge->cec_data)
+		drm_connector_hdmi_cec_received_msg(bridge->cec_data, msg);
+
+	mutex_unlock(&bridge->cec_mutex);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_cec_received_msg);
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c6af46dd02bfa9e15b59e4c460debdd7fd84be44..7ee791aa8f962fb13467464cebb8f53ab06991f0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -206,6 +206,7 @@ static LIST_HEAD(bridge_list);
 void drm_bridge_add(struct drm_bridge *bridge)
 {
 	mutex_init(&bridge->hpd_mutex);
+	mutex_init(&bridge->cec_mutex);
 
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
@@ -248,6 +249,7 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 	mutex_unlock(&bridge_lock);
 
 	mutex_destroy(&bridge->hpd_mutex);
+	mutex_destroy(&bridge->cec_mutex);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
@@ -1302,6 +1304,19 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
 
+void drm_bridge_cec_data_set(struct drm_bridge *bridge, void *cec_data)
+{
+	if (!(bridge->ops & DRM_BRIDGE_OP_HDMI))
+		return;
+
+	mutex_lock(&bridge->cec_mutex);
+
+	bridge->cec_data = cec_data;
+
+	mutex_unlock(&bridge->cec_mutex);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_cec_data_set);
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/display/drm_hdmi_cec_helper.h b/include/drm/display/drm_hdmi_cec_helper.h
index cd6274e4ee9b3e41a2d85289c4a420b854340e19..4d435192cba5a16255c7856c89686024521f51d2 100644
--- a/include/drm/display/drm_hdmi_cec_helper.h
+++ b/include/drm/display/drm_hdmi_cec_helper.h
@@ -7,6 +7,7 @@
 
 #include <linux/types.h>
 
+struct drm_bridge;
 struct drm_connector;
 
 struct cec_msg;
@@ -58,4 +59,11 @@ static inline void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connec
 static inline void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector) {}
 #endif
 
+void drm_bridge_cec_transmit_done(struct drm_bridge *bridge, u8 status,
+				  u8 arb_lost_cnt, u8 nack_cnt,
+				  u8 low_drive_cnt, u8 error_cnt);
+void drm_bridge_cec_transmit_attempt_done(struct drm_bridge *bridge, u8 status);
+void drm_bridge_cec_received_msg(struct drm_bridge *bridge,
+				 struct cec_msg *msg);
+
 #endif
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 348778f233b06265a6ae577762c6558e69cdb396..df1d72c7e176c75585283684acc2ef2ffb2f8bff 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -939,6 +939,15 @@ struct drm_bridge {
 	 * @hpd_cb.
 	 */
 	void *hpd_data;
+
+	/**
+	 * @cec_mutex: Protects the @cec_data field.
+	 */
+	struct mutex cec_mutex;
+	/**
+	 * @cec_data: Private data to be used by the CEC callbacks.
+	 */
+	void *cec_data;
 };
 
 static inline struct drm_bridge *
@@ -1062,6 +1071,8 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge);
 void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 			   enum drm_connector_status status);
 
+void drm_bridge_cec_data_set(struct drm_bridge *bridge, void *cec_data);
+
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 bool drm_bridge_is_panel(const struct drm_bridge *bridge);
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);

-- 
2.39.5

[PATCH v3 08/10] drm/display: bridge-connector: handle CEC adapters

[Dmitry Baryshkov]

Implement necessary glue code to let DRM bridge drivers to implement CEC
adapters support.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 93 ++++++++++++++++++++++++++
 include/drm/drm_bridge.h                       | 21 ++++++
 2 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 9f234bc647d5c0880d4c42aea130074b7fa54573..5b77fd59d79abddd419e611a7868b001857ccb37 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -9,6 +9,8 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 
+#include <media/cec.h>
+
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_bridge_connector.h>
@@ -497,6 +499,82 @@ static const struct drm_connector_hdmi_audio_funcs drm_bridge_connector_hdmi_aud
 	.mute_stream = drm_bridge_connector_audio_mute_stream,
 };
 
+static int drm_bridge_connector_hdmi_cec_enable(struct drm_connector *connector, bool enable)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+
+	return bridge->funcs->hdmi_cec_enable(bridge, enable);
+}
+
+static int drm_bridge_connector_hdmi_cec_log_addr(struct drm_connector *connector, u8 logical_addr)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+
+	return bridge->funcs->hdmi_cec_log_addr(bridge, logical_addr);
+}
+
+static int drm_bridge_connector_hdmi_cec_transmit(struct drm_connector *connector,
+						  u8 attempts,
+						  u32 signal_free_time,
+						  struct cec_msg *msg)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+
+	return bridge->funcs->hdmi_cec_transmit(bridge, attempts,
+						signal_free_time,
+						msg);
+}
+
+static int drm_bridge_connector_hdmi_cec_init(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+
+	drm_bridge_cec_data_set(bridge, connector);
+
+	if (!bridge->funcs->hdmi_cec_init)
+		return 0;
+
+	return bridge->funcs->hdmi_cec_init(connector, bridge);
+}
+
+static void drm_bridge_connector_hdmi_cec_unregister(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+
+	drm_bridge_cec_data_set(bridge, NULL);
+
+	drm_connector_hdmi_cec_unregister(connector);
+}
+
+static const struct drm_connector_hdmi_cec_adapter_ops drm_bridge_connector_hdmi_cec_ops = {
+	.base.unregister = drm_bridge_connector_hdmi_cec_unregister,
+	.init = drm_bridge_connector_hdmi_cec_init,
+	.enable = drm_bridge_connector_hdmi_cec_enable,
+	.log_addr = drm_bridge_connector_hdmi_cec_log_addr,
+	.transmit = drm_bridge_connector_hdmi_cec_transmit,
+};
+
+
 /* -----------------------------------------------------------------------------
  * Bridge Connector Initialisation
  */
@@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			if (ret)
 				return ERR_PTR(ret);
 		}
+
+		if (bridge->hdmi_cec_adapter_name) {
+			if (!bridge->funcs->hdmi_cec_enable ||
+			    !bridge->funcs->hdmi_cec_log_addr ||
+			    !bridge->funcs->hdmi_cec_transmit)
+				return ERR_PTR(-EINVAL);
+
+			ret = drm_connector_hdmi_cec_register(connector,
+							      &drm_bridge_connector_hdmi_cec_ops,
+							      bridge->hdmi_cec_adapter_name,
+							      bridge->hdmi_cec_available_las,
+							      bridge->hdmi_dev);
+			if (ret)
+				return ERR_PTR(ret);
+		}
 	} else {
 		ret = drmm_connector_init(drm, connector,
 					  &drm_bridge_connector_funcs,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index df1d72c7e176c75585283684acc2ef2ffb2f8bff..b55e80a57758e8b652eac0cd01cb245a04e221f5 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -32,6 +32,7 @@
 #include <drm/drm_mode_object.h>
 #include <drm/drm_modes.h>
 
+struct cec_msg;
 struct device_node;
 
 struct drm_bridge;
@@ -729,6 +730,16 @@ struct drm_bridge_funcs {
 				      struct drm_bridge *bridge,
 				      bool enable, int direction);
 
+	int (*hdmi_cec_init)(struct drm_connector *connector,
+			     struct drm_bridge *bridge);
+
+	int (*hdmi_cec_enable)(struct drm_bridge *bridge, bool enable);
+
+	int (*hdmi_cec_log_addr)(struct drm_bridge *bridge, u8 logical_addr);
+
+	int (*hdmi_cec_transmit)(struct drm_bridge *bridge, u8 attempts,
+				 u32 signal_free_time, struct cec_msg *msg);
+
 	/**
 	 * @debugfs_init:
 	 *
@@ -924,6 +935,16 @@ struct drm_bridge {
 	 */
 	bool hdmi_cec_notifier;
 
+	/**
+	 * @hdmi_cec_adapter_name: the name of the adapter to register
+	 */
+	const char *hdmi_cec_adapter_name;
+
+	/**
+	 * @hdmi_cec_available_las: number of logical addresses, CEC_MAX_LOG_ADDRS if unset
+	 */
+	u8 hdmi_cec_available_las;
+
 	/** private: */
 	/**
 	 * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.

-- 
2.39.5

Re: [PATCH v3 08/10] drm/display: bridge-connector: handle CEC adapters

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote:
>  /* -----------------------------------------------------------------------------
>   * Bridge Connector Initialisation
>   */
> @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  			if (ret)
>  				return ERR_PTR(ret);
>  		}
> +
> +		if (bridge->hdmi_cec_adapter_name) {
> +			if (!bridge->funcs->hdmi_cec_enable ||
> +			    !bridge->funcs->hdmi_cec_log_addr ||
> +			    !bridge->funcs->hdmi_cec_transmit)
> +				return ERR_PTR(-EINVAL);
> +
> +			ret = drm_connector_hdmi_cec_register(connector,
> +							      &drm_bridge_connector_hdmi_cec_ops,
> +							      bridge->hdmi_cec_adapter_name,
> +							      bridge->hdmi_cec_available_las,
> +							      bridge->hdmi_dev);
> +			if (ret)
> +				return ERR_PTR(ret);
> +		}

Maybe we can use a different bridge feature flag to trigger the CEC code
support instead?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 08/10] drm/display: bridge-connector: handle CEC adapters

[Dmitry Baryshkov]

On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote:
> On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote:
> >  /* -----------------------------------------------------------------------------
> >   * Bridge Connector Initialisation
> >   */
> > @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >  			if (ret)
> >  				return ERR_PTR(ret);
> >  		}
> > +
> > +		if (bridge->hdmi_cec_adapter_name) {
> > +			if (!bridge->funcs->hdmi_cec_enable ||
> > +			    !bridge->funcs->hdmi_cec_log_addr ||
> > +			    !bridge->funcs->hdmi_cec_transmit)
> > +				return ERR_PTR(-EINVAL);
> > +
> > +			ret = drm_connector_hdmi_cec_register(connector,
> > +							      &drm_bridge_connector_hdmi_cec_ops,
> > +							      bridge->hdmi_cec_adapter_name,
> > +							      bridge->hdmi_cec_available_las,
> > +							      bridge->hdmi_dev);
> > +			if (ret)
> > +				return ERR_PTR(ret);
> > +		}
> 
> Maybe we can use a different bridge feature flag to trigger the CEC code
> support instead?

it is possible, but what is the possible usecase? DP drivers should be
using DP_AUX CEC instead. And I think there are no other kinds of
bridges which implement CEC support. Meson driver does something strange
by registering CEC notifier from meson_encoder_hdmi. I think instead
this should be moved to the DW HDMI bridge itself

-- 
With best wishes
Dmitry

Re: [PATCH v3 08/10] drm/display: bridge-connector: handle CEC adapters

[Neil Armstrong]

On 29/01/2025 00:44, Dmitry Baryshkov wrote:
> On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote:
>> On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote:
>>>   /* -----------------------------------------------------------------------------
>>>    * Bridge Connector Initialisation
>>>    */
>>> @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>>>   			if (ret)
>>>   				return ERR_PTR(ret);
>>>   		}
>>> +
>>> +		if (bridge->hdmi_cec_adapter_name) {
>>> +			if (!bridge->funcs->hdmi_cec_enable ||
>>> +			    !bridge->funcs->hdmi_cec_log_addr ||
>>> +			    !bridge->funcs->hdmi_cec_transmit)
>>> +				return ERR_PTR(-EINVAL);
>>> +
>>> +			ret = drm_connector_hdmi_cec_register(connector,
>>> +							      &drm_bridge_connector_hdmi_cec_ops,
>>> +							      bridge->hdmi_cec_adapter_name,
>>> +							      bridge->hdmi_cec_available_las,
>>> +							      bridge->hdmi_dev);
>>> +			if (ret)
>>> +				return ERR_PTR(ret);
>>> +		}
>>
>> Maybe we can use a different bridge feature flag to trigger the CEC code
>> support instead?
> 
> it is possible, but what is the possible usecase? DP drivers should be
> using DP_AUX CEC instead. And I think there are no other kinds of
> bridges which implement CEC support. Meson driver does something strange
> by registering CEC notifier from meson_encoder_hdmi. I think instead
> this should be moved to the DW HDMI bridge itself
> 

It was done before bridge_connector has any support for CEC to keep the
functionnality, I'll be happy to switch to this.

Neil

Re: [PATCH v3 08/10] drm/display: bridge-connector: handle CEC adapters

[Dmitry Baryshkov]

On Wed, Jan 29, 2025 at 05:44:31PM +0100, Neil Armstrong wrote:
> On 29/01/2025 00:44, Dmitry Baryshkov wrote:
> > On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote:
> > > On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote:
> > > >   /* -----------------------------------------------------------------------------
> > > >    * Bridge Connector Initialisation
> > > >    */
> > > > @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > >   			if (ret)
> > > >   				return ERR_PTR(ret);
> > > >   		}
> > > > +
> > > > +		if (bridge->hdmi_cec_adapter_name) {
> > > > +			if (!bridge->funcs->hdmi_cec_enable ||
> > > > +			    !bridge->funcs->hdmi_cec_log_addr ||
> > > > +			    !bridge->funcs->hdmi_cec_transmit)
> > > > +				return ERR_PTR(-EINVAL);
> > > > +
> > > > +			ret = drm_connector_hdmi_cec_register(connector,
> > > > +							      &drm_bridge_connector_hdmi_cec_ops,
> > > > +							      bridge->hdmi_cec_adapter_name,
> > > > +							      bridge->hdmi_cec_available_las,
> > > > +							      bridge->hdmi_dev);
> > > > +			if (ret)
> > > > +				return ERR_PTR(ret);
> > > > +		}
> > > 
> > > Maybe we can use a different bridge feature flag to trigger the CEC code
> > > support instead?
> > 
> > it is possible, but what is the possible usecase? DP drivers should be
> > using DP_AUX CEC instead. And I think there are no other kinds of
> > bridges which implement CEC support. Meson driver does something strange
> > by registering CEC notifier from meson_encoder_hdmi. I think instead
> > this should be moved to the DW HDMI bridge itself
> 
> It was done before bridge_connector has any support for CEC to keep the
> functionnality, I'll be happy to switch to this.

Well... Converting dw-hdmi to use all HDMI API is going to be
interesting anyway:

- iMX8, iMX6 attach with flags=0, so dw-hdmi creates connector (with all
  its fanciness).
- Ingenic and RCAR-DU use simple drm_bridge_connector, so they will just
  get all new features.
- Meson uses drm_bridge_connector + additional bits on top of it (like
  CEC, max_bpc, HDR).

Either we end up with a monstruous commit that reworks dw-hdmi _and_
drop extra bits from meson driver at the same time or we need some
well-thought plan. For example, first make some of the features of Meson
optional and dependent on their generic counterparts being registered by
drm_bridge_connector, then convert dw-hdmi _and_ use
drm_bridge_connector internally, finally drop extra meson features now
consumed by the core.


-- 
With best wishes
Dmitry

[PATCH v3 09/10] drm/bridge: allow limiting I2S formats

[Dmitry Baryshkov]

By default HDMI codec registers all formats supported on the I2S bus.
Allow bridges (and connectors) to limit the list of the PCM formats
supported by the HDMI codec.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_bridge_connector.c  | 1 +
 drivers/gpu/drm/display/drm_hdmi_audio_helper.c | 2 ++
 drivers/gpu/drm/vc4/vc4_hdmi.c                  | 2 +-
 include/drm/display/drm_hdmi_audio_helper.h     | 1 +
 include/drm/drm_bridge.h                        | 5 +++++
 5 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 5b77fd59d79abddd419e611a7868b001857ccb37..28055bc2e7069d738bbe76b16c3bbde06f2d6e4e 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -698,6 +698,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 							    bridge->hdmi_dev,
 							    &drm_bridge_connector_hdmi_audio_funcs,
 							    bridge->hdmi_audio_max_i2s_playback_channels,
+							    bridge->hdmi_audio_i2s_formats,
 							    bridge->hdmi_audio_spdif_playback,
 							    bridge->hdmi_audio_dai_port);
 			if (ret)
diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
index 05afc9f0bdd6b6f00d74223a9d8875e6d16aea5f..589b0bd6c21366b83bd4d1131e89c71644ebc401 100644
--- a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
@@ -154,6 +154,7 @@ int drm_connector_hdmi_audio_init(struct drm_connector *connector,
 				  struct device *hdmi_codec_dev,
 				  const struct drm_connector_hdmi_audio_funcs *funcs,
 				  unsigned int max_i2s_playback_channels,
+				  u64 i2s_formats,
 				  bool spdif_playback,
 				  int dai_port)
 {
@@ -161,6 +162,7 @@ int drm_connector_hdmi_audio_init(struct drm_connector *connector,
 		.ops = &drm_connector_hdmi_audio_ops,
 		.max_i2s_channels = max_i2s_playback_channels,
 		.i2s = !!max_i2s_playback_channels,
+		.i2s_formats = i2s_formats,
 		.spdif = spdif_playback,
 		.no_i2s_capture = true,
 		.no_spdif_capture = true,
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1108983c44858382cb9f09b686956903645ebe0a..fcaba4a64a33b2267b22960772c2977b4109c67f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -582,7 +582,7 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
 
 	ret = drm_connector_hdmi_audio_init(connector, dev->dev,
 					    &vc4_hdmi_audio_funcs,
-					    8, false, -1);
+					    8, 0, false, -1);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/display/drm_hdmi_audio_helper.h b/include/drm/display/drm_hdmi_audio_helper.h
index c9a6faef4109f20ba79b610a9d5e8d5980efe2d1..44d910bdc72dd2fdbbe7ada65b67080d4a41e88b 100644
--- a/include/drm/display/drm_hdmi_audio_helper.h
+++ b/include/drm/display/drm_hdmi_audio_helper.h
@@ -14,6 +14,7 @@ int drm_connector_hdmi_audio_init(struct drm_connector *connector,
 				  struct device *hdmi_codec_dev,
 				  const struct drm_connector_hdmi_audio_funcs *funcs,
 				  unsigned int max_i2s_playback_channels,
+				  u64 i2s_formats,
 				  bool spdif_playback,
 				  int sound_dai_port);
 void drm_connector_hdmi_audio_plugged_notify(struct drm_connector *connector,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b55e80a57758e8b652eac0cd01cb245a04e221f5..d16af5fe90cb48f6671e798d9dee61a359c9233f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -920,6 +920,11 @@ struct drm_bridge {
 	 */
 	int hdmi_audio_max_i2s_playback_channels;
 
+	/**
+	 * @hdmi_audio_i2s_formats: supported I2S formats, optional
+	 */
+	u64 hdmi_audio_i2s_formats;
+
 	/**
 	 * @hdmi_audio_spdif_playback: set if HDMI codec has S/PDIF playback port
 	 */

-- 
2.39.5

Re: [PATCH v3 09/10] drm/bridge: allow limiting I2S formats

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Sun, Jan 26, 2025 at 03:29:14PM +0200, Dmitry Baryshkov wrote:
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b55e80a57758e8b652eac0cd01cb245a04e221f5..d16af5fe90cb48f6671e798d9dee61a359c9233f 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -920,6 +920,11 @@ struct drm_bridge {
>  	 */
>  	int hdmi_audio_max_i2s_playback_channels;
>  
> +	/**
> +	 * @hdmi_audio_i2s_formats: supported I2S formats, optional
> +	 */
> +	u64 hdmi_audio_i2s_formats;
> +

We should document what the default value is if it's optional.

Once fixed:
Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 09/10] drm/bridge: allow limiting I2S formats

[Dmitry Baryshkov]

On Tue, Jan 28, 2025 at 05:11:16PM +0100, Maxime Ripard wrote:
> On Sun, Jan 26, 2025 at 03:29:14PM +0200, Dmitry Baryshkov wrote:
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index b55e80a57758e8b652eac0cd01cb245a04e221f5..d16af5fe90cb48f6671e798d9dee61a359c9233f 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -920,6 +920,11 @@ struct drm_bridge {
> >  	 */
> >  	int hdmi_audio_max_i2s_playback_channels;
> >  
> > +	/**
> > +	 * @hdmi_audio_i2s_formats: supported I2S formats, optional
> > +	 */
> > +	u64 hdmi_audio_i2s_formats;
> > +
> 
> We should document what the default value is if it's optional.

"The default is to allow all formats supported by the corresponding
I2S bus driver." Does that sound fine from your POV?

> 
> Once fixed:
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 
> Maxime



-- 
With best wishes
Dmitry

Re: [PATCH v3 09/10] drm/bridge: allow limiting I2S formats

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Wed, Jan 29, 2025 at 01:45:56AM +0200, Dmitry Baryshkov wrote:
> On Tue, Jan 28, 2025 at 05:11:16PM +0100, Maxime Ripard wrote:
> > On Sun, Jan 26, 2025 at 03:29:14PM +0200, Dmitry Baryshkov wrote:
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index b55e80a57758e8b652eac0cd01cb245a04e221f5..d16af5fe90cb48f6671e798d9dee61a359c9233f 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -920,6 +920,11 @@ struct drm_bridge {
> > >  	 */
> > >  	int hdmi_audio_max_i2s_playback_channels;
> > >  
> > > +	/**
> > > +	 * @hdmi_audio_i2s_formats: supported I2S formats, optional
> > > +	 */
> > > +	u64 hdmi_audio_i2s_formats;
> > > +
> > 
> > We should document what the default value is if it's optional.
> 
> "The default is to allow all formats supported by the corresponding
> I2S bus driver." Does that sound fine from your POV?

Yep, sounds good.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH v3 10/10] drm/bridge: adv7511: switch to the HDMI connector helpers

[Dmitry Baryshkov]

Rewrite the ADV7511 driver to use implementation provided by the DRM
HDMI connector framework, including the Audio and CEC bits. Drop the
in-bridge connector support and use drm_bridge_connector if the host
requires the connector to be provided by the bridge.

Note: currently only AVI InfoFrames are supported. Existing driver
doesn't support programming any other InfoFrames directly and Audio
InfoFrame seems to be programmed using individual bits and pieces rather
than programming it directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/Kconfig         |   4 +
 drivers/gpu/drm/bridge/adv7511/adv7511.h       |  51 ++--
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c |  77 +-----
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c   |  56 ++--
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 342 +++++++++++--------------
 drivers/gpu/drm/bridge/adv7511/adv7533.c       |   4 -
 6 files changed, 206 insertions(+), 328 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
index f46a5e26b5dd640670afa21802f9019d5c7439fb..31df5467fd6a035d546996dc6c8b509eca0c64b1 100644
--- a/drivers/gpu/drm/bridge/adv7511/Kconfig
+++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
@@ -5,6 +5,10 @@ config DRM_I2C_ADV7511
 	select DRM_KMS_HELPER
 	select REGMAP_I2C
 	select DRM_MIPI_DSI
+	select DRM_DISPLAY_HELPER
+	select DRM_BRIDGE_CONNECTOR
+	select DRM_DISPLAY_HDMI_STATE_HELPER
+	select DRM_DISPLAY_HDMI_CEC_HELPER
 	help
 	  Support for the Analog Devices ADV7511(W)/13/33/35 HDMI encoders.
 
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index ec0b7f3d889c4eedeb1d80369fd2a160cd0e2968..530eb124b8969441939d8a1b7e9c1268eb929672 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -313,16 +313,11 @@ enum adv7511_csc_scaling {
  * @csc_enable:			Whether to enable color space conversion
  * @csc_scaling_factor:		Color space conversion scaling factor
  * @csc_coefficents:		Color space conversion coefficents
- * @hdmi_mode:			Whether to use HDMI or DVI output mode
- * @avi_infoframe:		HDMI infoframe
  */
 struct adv7511_video_config {
 	bool csc_enable;
 	enum adv7511_csc_scaling csc_scaling_factor;
 	const uint16_t *csc_coefficents;
-
-	bool hdmi_mode;
-	struct hdmi_avi_infoframe avi_infoframe;
 };
 
 enum adv7511_type {
@@ -337,6 +332,7 @@ struct adv7511_chip_info {
 	enum adv7511_type type;
 	unsigned int max_mode_clock_khz;
 	unsigned int max_lane_freq_khz;
+	const char *name;
 	const char * const *supply_names;
 	unsigned int num_supplies;
 	unsigned int reg_cec_offset;
@@ -371,7 +367,6 @@ struct adv7511 {
 	struct work_struct hpd_work;
 
 	struct drm_bridge bridge;
-	struct drm_connector connector;
 
 	bool embedded_sync;
 	enum adv7511_sync_polarity vsync_polarity;
@@ -389,9 +384,7 @@ struct adv7511 {
 	bool use_timing_gen;
 
 	const struct adv7511_chip_info *info;
-	struct platform_device *audio_pdev;
 
-	struct cec_adapter *cec_adap;
 	u8   cec_addr[ADV7511_MAX_ADDRS];
 	u8   cec_valid_addrs;
 	bool cec_enabled_adap;
@@ -399,16 +392,24 @@ struct adv7511 {
 	u32 cec_clk_freq;
 };
 
+static inline struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct adv7511, bridge);
+}
+
 #ifdef CONFIG_DRM_I2C_ADV7511_CEC
-int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
+int adv7511_cec_init(struct drm_connector *connector,
+		     struct drm_bridge *bridge);
+int adv7511_cec_enable(struct drm_bridge *bridge, bool enable);
+int adv7511_cec_log_addr(struct drm_bridge *bridge, u8 addr);
+int adv7511_cec_transmit(struct drm_bridge *bridge, u8 attempts,
+			      u32 signal_free_time, struct cec_msg *msg);
 int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
 #else
-static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
-{
-	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
-		     ADV7511_CEC_CTRL_POWER_DOWN);
-	return 0;
-}
+#define adv7511_cec_init NULL
+#define adv7511_cec_enable NULL
+#define adv7511_cec_log_addr NULL
+#define adv7511_cec_transmit NULL
 #endif
 
 void adv7533_dsi_power_on(struct adv7511 *adv);
@@ -421,16 +422,18 @@ int adv7533_attach_dsi(struct adv7511 *adv);
 int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
 
 #ifdef CONFIG_DRM_I2C_ADV7511_AUDIO
-int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511);
-void adv7511_audio_exit(struct adv7511 *adv7511);
+int adv7511_hdmi_audio_startup(struct drm_connector *connector,
+			       struct drm_bridge *bridge);
+void adv7511_hdmi_audio_shutdown(struct drm_connector *connector,
+				 struct drm_bridge *bridge);
+int adv7511_hdmi_audio_prepare(struct drm_connector *connector,
+			       struct drm_bridge *bridge,
+			       struct hdmi_codec_daifmt *fmt,
+			       struct hdmi_codec_params *hparms);
 #else /*CONFIG_DRM_I2C_ADV7511_AUDIO */
-static inline int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511)
-{
-	return 0;
-}
-static inline void adv7511_audio_exit(struct adv7511 *adv7511)
-{
-}
+#define adv7511_hdmi_audio_startup NULL
+#define adv7511_hdmi_audio_shutdown NULL
+#define adv7511_hdmi_audio_prepare NULL
 #endif /* CONFIG_DRM_I2C_ADV7511_AUDIO */
 
 #endif /* __DRM_I2C_ADV7511_H__ */
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
index 36a7b6f085f9a0fa2c60660df4f569b24b49c4be..9d0193075528186f47ae73a284c25376c41075a4 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
@@ -55,11 +55,12 @@ static int adv7511_update_cts_n(struct adv7511 *adv7511)
 	return 0;
 }
 
-static int adv7511_hdmi_hw_params(struct device *dev, void *data,
-				  struct hdmi_codec_daifmt *fmt,
-				  struct hdmi_codec_params *hparms)
+int adv7511_hdmi_audio_prepare(struct drm_connector *connector,
+			       struct drm_bridge *bridge,
+			       struct hdmi_codec_daifmt *fmt,
+			       struct hdmi_codec_params *hparms)
 {
-	struct adv7511 *adv7511 = dev_get_drvdata(dev);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 	unsigned int audio_source, i2s_format = 0;
 	unsigned int invert_clock;
 	unsigned int rate;
@@ -158,9 +159,10 @@ static int adv7511_hdmi_hw_params(struct device *dev, void *data,
 	return 0;
 }
 
-static int audio_startup(struct device *dev, void *data)
+int adv7511_hdmi_audio_startup(struct drm_connector *connector,
+			       struct drm_bridge *bridge)
 {
-	struct adv7511 *adv7511 = dev_get_drvdata(dev);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_AUDIO_CONFIG,
 				BIT(7), 0);
@@ -194,69 +196,12 @@ static int audio_startup(struct device *dev, void *data)
 	return 0;
 }
 
-static void audio_shutdown(struct device *dev, void *data)
+void adv7511_hdmi_audio_shutdown(struct drm_connector *connector,
+				 struct drm_bridge *bridge)
 {
-	struct adv7511 *adv7511 = dev_get_drvdata(dev);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 
 	if (adv7511->audio_source == ADV7511_AUDIO_SOURCE_SPDIF)
 		regmap_update_bits(adv7511->regmap, ADV7511_REG_AUDIO_CONFIG,
 				   BIT(7), 0);
 }
-
-static int adv7511_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-					struct device_node *endpoint,
-					void *data)
-{
-	struct of_endpoint of_ep;
-	int ret;
-
-	ret = of_graph_parse_endpoint(endpoint, &of_ep);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * HDMI sound should be located as reg = <2>
-	 * Then, it is sound port 0
-	 */
-	if (of_ep.port == 2)
-		return 0;
-
-	return -EINVAL;
-}
-
-static const struct hdmi_codec_ops adv7511_codec_ops = {
-	.hw_params	= adv7511_hdmi_hw_params,
-	.audio_shutdown = audio_shutdown,
-	.audio_startup	= audio_startup,
-	.get_dai_id	= adv7511_hdmi_i2s_get_dai_id,
-};
-
-static const struct hdmi_codec_pdata codec_data = {
-	.ops = &adv7511_codec_ops,
-	.i2s_formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |
-			SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_LE |
-			SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE),
-	.max_i2s_channels = 2,
-	.i2s = 1,
-	.no_i2s_capture = 1,
-	.spdif = 1,
-	.no_spdif_capture = 1,
-};
-
-int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511)
-{
-	adv7511->audio_pdev = platform_device_register_data(dev,
-					HDMI_CODEC_DRV_NAME,
-					PLATFORM_DEVID_AUTO,
-					&codec_data,
-					sizeof(codec_data));
-	return PTR_ERR_OR_ZERO(adv7511->audio_pdev);
-}
-
-void adv7511_audio_exit(struct adv7511 *adv7511)
-{
-	if (adv7511->audio_pdev) {
-		platform_device_unregister(adv7511->audio_pdev);
-		adv7511->audio_pdev = NULL;
-	}
-}
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
index 2e9c88a2b5ed44ef0cd417c553ea7873d00e4a14..bc96f46bbc4480d3439e821991dab81ae4950208 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -12,6 +12,8 @@
 
 #include <media/cec.h>
 
+#include <drm/display/drm_hdmi_cec_helper.h>
+
 #include "adv7511.h"
 
 static const u8 ADV7511_REG_CEC_RX_FRAME_HDR[] = {
@@ -44,8 +46,8 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 		return;
 
 	if (tx_raw_status & ADV7511_INT1_CEC_TX_ARBIT_LOST) {
-		cec_transmit_attempt_done(adv7511->cec_adap,
-					  CEC_TX_STATUS_ARB_LOST);
+		drm_bridge_cec_transmit_attempt_done(&adv7511->bridge,
+						     CEC_TX_STATUS_ARB_LOST);
 		return;
 	}
 	if (tx_raw_status & ADV7511_INT1_CEC_TX_RETRY_TIMEOUT) {
@@ -72,12 +74,13 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 			if (low_drive_cnt)
 				status |= CEC_TX_STATUS_LOW_DRIVE;
 		}
-		cec_transmit_done(adv7511->cec_adap, status,
-				  0, nack_cnt, low_drive_cnt, err_cnt);
+		drm_bridge_cec_transmit_done(&adv7511->bridge, status,
+					     0, nack_cnt, low_drive_cnt,
+					     err_cnt);
 		return;
 	}
 	if (tx_raw_status & ADV7511_INT1_CEC_TX_READY) {
-		cec_transmit_attempt_done(adv7511->cec_adap, CEC_TX_STATUS_OK);
+		drm_bridge_cec_transmit_attempt_done(&adv7511->bridge, CEC_TX_STATUS_OK);
 		return;
 	}
 }
@@ -116,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
 	regmap_update_bits(adv7511->regmap_cec,
 			   ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf), 0);
 
-	cec_received_msg(adv7511->cec_adap, &msg);
+	drm_bridge_cec_received_msg(&adv7511->bridge, &msg);
 }
 
 int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
@@ -179,9 +182,9 @@ int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 	return IRQ_HANDLED;
 }
 
-static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
+int adv7511_cec_enable(struct drm_bridge *bridge, bool enable)
 {
-	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 	unsigned int offset = adv7511->info->reg_cec_offset;
 
 	if (adv7511->i2c_cec == NULL)
@@ -225,9 +228,9 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	return 0;
 }
 
-static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
+int adv7511_cec_log_addr(struct drm_bridge *bridge, u8 addr)
 {
-	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 	unsigned int offset = adv7511->info->reg_cec_offset;
 	unsigned int i, free_idx = ADV7511_MAX_ADDRS;
 
@@ -293,10 +296,10 @@ static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
 	return 0;
 }
 
-static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
-				     u32 signal_free_time, struct cec_msg *msg)
+int adv7511_cec_transmit(struct drm_bridge *bridge, u8 attempts,
+			 u32 signal_free_time, struct cec_msg *msg)
 {
-	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
 	unsigned int offset = adv7511->info->reg_cec_offset;
 	u8 len = msg->len;
 	unsigned int i;
@@ -328,12 +331,6 @@ static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	return 0;
 }
 
-static const struct cec_adap_ops adv7511_cec_adap_ops = {
-	.adap_enable = adv7511_cec_adap_enable,
-	.adap_log_addr = adv7511_cec_adap_log_addr,
-	.adap_transmit = adv7511_cec_adap_transmit,
-};
-
 static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
 {
 	adv7511->cec_clk = devm_clk_get(dev, "cec");
@@ -348,21 +345,17 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
 	return 0;
 }
 
-int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
+int adv7511_cec_init(struct drm_connector *connector,
+		     struct drm_bridge *bridge)
 {
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
+	struct device *dev = &adv7511->i2c_main->dev;
 	unsigned int offset = adv7511->info->reg_cec_offset;
 	int ret = adv7511_cec_parse_dt(dev, adv7511);
 
 	if (ret)
 		goto err_cec_parse_dt;
 
-	adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
-		adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS);
-	if (IS_ERR(adv7511->cec_adap)) {
-		ret = PTR_ERR(adv7511->cec_adap);
-		goto err_cec_alloc;
-	}
-
 	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL, 0);
 	/* cec soft reset */
 	regmap_write(adv7511->regmap_cec,
@@ -378,17 +371,8 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 		     ADV7511_REG_CEC_CLK_DIV + offset,
 		     ((adv7511->cec_clk_freq / 750000) - 1) << 2);
 
-	ret = cec_register_adapter(adv7511->cec_adap, dev);
-	if (ret)
-		goto err_cec_register;
 	return 0;
 
-err_cec_register:
-	cec_delete_adapter(adv7511->cec_adap);
-	adv7511->cec_adap = NULL;
-err_cec_alloc:
-	dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n",
-		 ret);
 err_cec_parse_dt:
 	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
 		     ADV7511_CEC_CTRL_POWER_DOWN);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 1a76aea6a945ed4b23239edbf084681462fac1d9..60264bc903b58a72e8a2cd772ea07766c0ad12cf 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -12,14 +12,17 @@
 #include <linux/of.h>
 #include <linux/slab.h>
 
-#include <media/cec.h>
+#include <sound/pcm.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 
 #include "adv7511.h"
 
@@ -203,62 +206,37 @@ static const uint16_t adv7511_csc_ycbcr_to_rgb[] = {
 
 static void adv7511_set_config_csc(struct adv7511 *adv7511,
 				   struct drm_connector *connector,
-				   bool rgb, bool hdmi_mode)
+				   bool rgb)
 {
 	struct adv7511_video_config config;
 	bool output_format_422, output_format_ycbcr;
 	unsigned int mode;
-	uint8_t infoframe[17];
-
-	config.hdmi_mode = hdmi_mode;
-
-	hdmi_avi_infoframe_init(&config.avi_infoframe);
-
-	config.avi_infoframe.scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
 
 	if (rgb) {
 		config.csc_enable = false;
-		config.avi_infoframe.colorspace = HDMI_COLORSPACE_RGB;
+		output_format_422 = false;
+		output_format_ycbcr = false;
 	} else {
 		config.csc_scaling_factor = ADV7511_CSC_SCALING_4;
 		config.csc_coefficents = adv7511_csc_ycbcr_to_rgb;
 
 		if ((connector->display_info.color_formats &
 		     DRM_COLOR_FORMAT_YCBCR422) &&
-		    config.hdmi_mode) {
+		    connector->display_info.is_hdmi) {
 			config.csc_enable = false;
-			config.avi_infoframe.colorspace =
-				HDMI_COLORSPACE_YUV422;
-		} else {
-			config.csc_enable = true;
-			config.avi_infoframe.colorspace = HDMI_COLORSPACE_RGB;
-		}
-	}
-
-	if (config.hdmi_mode) {
-		mode = ADV7511_HDMI_CFG_MODE_HDMI;
-
-		switch (config.avi_infoframe.colorspace) {
-		case HDMI_COLORSPACE_YUV444:
-			output_format_422 = false;
-			output_format_ycbcr = true;
-			break;
-		case HDMI_COLORSPACE_YUV422:
 			output_format_422 = true;
 			output_format_ycbcr = true;
-			break;
-		default:
+		} else {
+			config.csc_enable = true;
 			output_format_422 = false;
 			output_format_ycbcr = false;
-			break;
 		}
-	} else {
-		mode = ADV7511_HDMI_CFG_MODE_DVI;
-		output_format_422 = false;
-		output_format_ycbcr = false;
 	}
 
-	adv7511_packet_disable(adv7511, ADV7511_PACKET_ENABLE_AVI_INFOFRAME);
+	if (connector->display_info.is_hdmi)
+		mode = ADV7511_HDMI_CFG_MODE_HDMI;
+	else
+		mode = ADV7511_HDMI_CFG_MODE_DVI;
 
 	adv7511_set_colormap(adv7511, config.csc_enable,
 			     config.csc_coefficents,
@@ -269,15 +247,6 @@ static void adv7511_set_config_csc(struct adv7511 *adv7511,
 
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_HDCP_HDMI_CFG,
 			   ADV7511_HDMI_CFG_MODE_MASK, mode);
-
-	hdmi_avi_infoframe_pack(&config.avi_infoframe, infoframe,
-				sizeof(infoframe));
-
-	/* The AVI infoframe id is not configurable */
-	regmap_bulk_write(adv7511->regmap, ADV7511_REG_AVI_INFOFRAME_VERSION,
-			  infoframe + 1, sizeof(infoframe) - 1);
-
-	adv7511_packet_enable(adv7511, ADV7511_PACKET_ENABLE_AVI_INFOFRAME);
 }
 
 static void adv7511_set_link_config(struct adv7511 *adv7511,
@@ -446,22 +415,16 @@ static void adv7511_hpd_work(struct work_struct *work)
 	 * restore its state.
 	 */
 	if (status == connector_status_connected &&
-	    adv7511->connector.status == connector_status_disconnected &&
+	    adv7511->status == connector_status_disconnected &&
 	    adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
 		adv7511_power_on(adv7511);
 	}
 
-	if (adv7511->connector.status != status) {
-		adv7511->connector.status = status;
+	if (adv7511->status != status) {
+		adv7511->status = status;
 
-		if (adv7511->connector.dev) {
-			if (status == connector_status_disconnected)
-				cec_phys_addr_invalidate(adv7511->cec_adap);
-			drm_kms_helper_hotplug_event(adv7511->connector.dev);
-		} else {
-			drm_bridge_hpd_notify(&adv7511->bridge, status);
-		}
+		drm_bridge_hpd_notify(&adv7511->bridge, status);
 	}
 }
 
@@ -636,45 +599,11 @@ static const struct drm_edid *adv7511_edid_read(struct adv7511 *adv7511,
 	if (!adv7511->powered)
 		__adv7511_power_off(adv7511);
 
-	if (drm_edid) {
-		/*
-		 * FIXME: The CEC physical address should be set using
-		 * cec_s_phys_addr(adap,
-		 * connector->display_info.source_physical_address, false) from
-		 * a path that has read the EDID and called
-		 * drm_edid_connector_update().
-		 */
-		const struct edid *edid = drm_edid_raw(drm_edid);
-
-		adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
-				       drm_detect_hdmi_monitor(edid));
-
-		cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
-	} else {
-		cec_s_phys_addr_from_edid(adv7511->cec_adap, NULL);
-	}
-
 	return drm_edid;
 }
 
-static int adv7511_get_modes(struct adv7511 *adv7511,
-			     struct drm_connector *connector)
-{
-	const struct drm_edid *drm_edid;
-	unsigned int count;
-
-	drm_edid = adv7511_edid_read(adv7511, connector);
-
-	drm_edid_connector_update(connector, drm_edid);
-	count = drm_edid_connector_add_modes(connector);
-
-	drm_edid_free(drm_edid);
-
-	return count;
-}
-
 static enum drm_connector_status
-adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
+adv7511_detect(struct adv7511 *adv7511)
 {
 	enum drm_connector_status status;
 	unsigned int val;
@@ -699,8 +628,6 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
 	if (status == connector_status_connected && hpd && adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
 		adv7511_power_on(adv7511);
-		if (connector)
-			adv7511_get_modes(adv7511, connector);
 		if (adv7511->status == connector_status_connected)
 			status = connector_status_disconnected;
 	} else {
@@ -719,17 +646,7 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
 	return status;
 }
 
-static enum drm_mode_status adv7511_mode_valid(struct adv7511 *adv7511,
-			      const struct drm_display_mode *mode)
-{
-	if (mode->clock > 165000)
-		return MODE_CLOCK_HIGH;
-
-	return MODE_OK;
-}
-
 static void adv7511_mode_set(struct adv7511 *adv7511,
-			     const struct drm_display_mode *mode,
 			     const struct drm_display_mode *adj_mode)
 {
 	unsigned int low_refresh_rate;
@@ -800,11 +717,11 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 			vsync_polarity = 1;
 	}
 
-	if (drm_mode_vrefresh(mode) <= 24)
+	if (drm_mode_vrefresh(adj_mode) <= 24)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_24HZ;
-	else if (drm_mode_vrefresh(mode) <= 25)
+	else if (drm_mode_vrefresh(adj_mode) <= 25)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_25HZ;
-	else if (drm_mode_vrefresh(mode) <= 30)
+	else if (drm_mode_vrefresh(adj_mode) <= 30)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_30HZ;
 	else
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_NONE;
@@ -826,77 +743,21 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 	 * supposed to give better results.
 	 */
 
-	adv7511->f_tmds = mode->clock;
-}
-
-/* -----------------------------------------------------------------------------
- * DRM Connector Operations
- */
-
-static struct adv7511 *connector_to_adv7511(struct drm_connector *connector)
-{
-	return container_of(connector, struct adv7511, connector);
+	adv7511->f_tmds = adj_mode->clock;
 }
 
-static int adv7511_connector_get_modes(struct drm_connector *connector)
-{
-	struct adv7511 *adv = connector_to_adv7511(connector);
-
-	return adv7511_get_modes(adv, connector);
-}
-
-static enum drm_mode_status
-adv7511_connector_mode_valid(struct drm_connector *connector,
-			     const struct drm_display_mode *mode)
-{
-	struct adv7511 *adv = connector_to_adv7511(connector);
-
-	return adv7511_mode_valid(adv, mode);
-}
-
-static struct drm_connector_helper_funcs adv7511_connector_helper_funcs = {
-	.get_modes = adv7511_connector_get_modes,
-	.mode_valid = adv7511_connector_mode_valid,
-};
-
-static enum drm_connector_status
-adv7511_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct adv7511 *adv = connector_to_adv7511(connector);
-
-	return adv7511_detect(adv, connector);
-}
-
-static const struct drm_connector_funcs adv7511_connector_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = adv7511_connector_detect,
-	.destroy = drm_connector_cleanup,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 static int adv7511_connector_init(struct adv7511 *adv)
 {
 	struct drm_bridge *bridge = &adv->bridge;
-	int ret;
-
-	if (adv->i2c_main->irq)
-		adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
-				DRM_CONNECTOR_POLL_DISCONNECT;
+	struct drm_connector *connector;
 
-	ret = drm_connector_init(bridge->dev, &adv->connector,
-				 &adv7511_connector_funcs,
-				 DRM_MODE_CONNECTOR_HDMIA);
-	if (ret < 0) {
+	connector = drm_bridge_connector_init(bridge->dev, bridge->encoder);
+	if (IS_ERR(connector)) {
 		DRM_ERROR("Failed to initialize connector with drm\n");
-		return ret;
+		return PTR_ERR(connector);
 	}
-	drm_connector_helper_add(&adv->connector,
-				 &adv7511_connector_helper_funcs);
-	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
+
+	drm_connector_attach_encoder(connector, bridge->encoder);
 
 	return 0;
 }
@@ -905,7 +766,7 @@ static int adv7511_connector_init(struct adv7511 *adv)
  * DRM Bridge Operations
  */
 
-static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
+static const struct adv7511 *bridge_to_adv7511_const(const struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct adv7511, bridge);
 }
@@ -913,9 +774,31 @@ static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
 static void adv7511_bridge_atomic_enable(struct drm_bridge *bridge,
 					 struct drm_bridge_state *bridge_state)
 {
+	struct drm_atomic_state *state = bridge_state->base.state;
 	struct adv7511 *adv = bridge_to_adv7511(bridge);
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
 
 	adv7511_power_on(adv);
+
+	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
+	adv7511_set_config_csc(adv, connector, adv->rgb);
+
+	adv7511_mode_set(adv, &crtc_state->adjusted_mode);
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
 }
 
 static void adv7511_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -926,13 +809,17 @@ static void adv7511_bridge_atomic_disable(struct drm_bridge *bridge,
 	adv7511_power_off(adv);
 }
 
-static void adv7511_bridge_mode_set(struct drm_bridge *bridge,
-				    const struct drm_display_mode *mode,
-				    const struct drm_display_mode *adj_mode)
+static enum drm_mode_status
+adv7511_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+					 const struct drm_display_mode *mode,
+					 unsigned long long tmds_rate)
 {
-	struct adv7511 *adv = bridge_to_adv7511(bridge);
+	const struct adv7511 *adv = bridge_to_adv7511_const(bridge);
 
-	adv7511_mode_set(adv, mode, adj_mode);
+	if (mode->clock > 1000ULL * adv->info->max_mode_clock_khz)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
 }
 
 static enum drm_mode_status adv7511_bridge_mode_valid(struct drm_bridge *bridge,
@@ -941,10 +828,10 @@ static enum drm_mode_status adv7511_bridge_mode_valid(struct drm_bridge *bridge,
 {
 	struct adv7511 *adv = bridge_to_adv7511(bridge);
 
-	if (adv->info->has_dsi)
-		return adv7533_mode_valid(adv, mode);
-	else
-		return adv7511_mode_valid(adv, mode);
+	if (!adv->info->has_dsi)
+		return MODE_OK;
+
+	return adv7533_mode_valid(adv, mode);
 }
 
 static int adv7511_bridge_attach(struct drm_bridge *bridge,
@@ -977,7 +864,7 @@ static enum drm_connector_status adv7511_bridge_detect(struct drm_bridge *bridge
 {
 	struct adv7511 *adv = bridge_to_adv7511(bridge);
 
-	return adv7511_detect(adv, NULL);
+	return adv7511_detect(adv);
 }
 
 static const struct drm_edid *adv7511_bridge_edid_read(struct drm_bridge *bridge,
@@ -988,28 +875,71 @@ static const struct drm_edid *adv7511_bridge_edid_read(struct drm_bridge *bridge
 	return adv7511_edid_read(adv, connector);
 }
 
-static void adv7511_bridge_hpd_notify(struct drm_bridge *bridge,
-				      enum drm_connector_status status)
+static int adv7511_bridge_hdmi_clear_infoframe(struct drm_bridge *bridge,
+					       enum hdmi_infoframe_type type)
 {
-	struct adv7511 *adv = bridge_to_adv7511(bridge);
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		adv7511_packet_disable(adv7511, ADV7511_PACKET_ENABLE_AVI_INFOFRAME);
+		break;
+	default:
+		drm_dbg_driver(adv7511->bridge.dev, "Unsupported HDMI InfoFrame %x\n", type);
+		break;
+	}
+
+	return 0;
+}
+
+static int adv7511_bridge_hdmi_write_infoframe(struct drm_bridge *bridge,
+					       enum hdmi_infoframe_type type,
+					       const u8 *buffer, size_t len)
+{
+	struct adv7511 *adv7511 = bridge_to_adv7511(bridge);
+
+	adv7511_bridge_hdmi_clear_infoframe(bridge, type);
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		/* The AVI infoframe id is not configurable */
+		regmap_bulk_write(adv7511->regmap, ADV7511_REG_AVI_INFOFRAME_VERSION,
+				  buffer + 1, len - 1);
 
-	if (status == connector_status_disconnected)
-		cec_phys_addr_invalidate(adv->cec_adap);
+		adv7511_packet_enable(adv7511, ADV7511_PACKET_ENABLE_AVI_INFOFRAME);
+		break;
+	default:
+		drm_dbg_driver(adv7511->bridge.dev, "Unsupported HDMI InfoFrame %x\n", type);
+		break;
+	}
+
+	return 0;
 }
 
 static const struct drm_bridge_funcs adv7511_bridge_funcs = {
-	.mode_set = adv7511_bridge_mode_set,
 	.mode_valid = adv7511_bridge_mode_valid,
 	.attach = adv7511_bridge_attach,
 	.detect = adv7511_bridge_detect,
 	.edid_read = adv7511_bridge_edid_read,
-	.hpd_notify = adv7511_bridge_hpd_notify,
 
 	.atomic_enable = adv7511_bridge_atomic_enable,
 	.atomic_disable = adv7511_bridge_atomic_disable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
+
+	.hdmi_tmds_char_rate_valid = adv7511_bridge_hdmi_tmds_char_rate_valid,
+	.hdmi_clear_infoframe = adv7511_bridge_hdmi_clear_infoframe,
+	.hdmi_write_infoframe = adv7511_bridge_hdmi_write_infoframe,
+
+	.hdmi_audio_startup = adv7511_hdmi_audio_startup,
+	.hdmi_audio_prepare = adv7511_hdmi_audio_prepare,
+	.hdmi_audio_shutdown = adv7511_hdmi_audio_shutdown,
+
+	.hdmi_cec_init = adv7511_cec_init,
+	.hdmi_cec_enable = adv7511_cec_enable,
+	.hdmi_cec_log_addr = adv7511_cec_log_addr,
+	.hdmi_cec_transmit = adv7511_cec_transmit,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1320,22 +1250,40 @@ static int adv7511_probe(struct i2c_client *i2c)
 	if (adv7511->info->link_config)
 		adv7511_set_link_config(adv7511, &link_config);
 
-	ret = adv7511_cec_init(dev, adv7511);
-	if (ret)
-		goto err_unregister_cec;
+	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
+		     ADV7511_CEC_CTRL_POWER_DOWN);
 
 	adv7511->bridge.funcs = &adv7511_bridge_funcs;
-	adv7511->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+	adv7511->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
+		DRM_BRIDGE_OP_HDMI;
 	if (adv7511->i2c_main->irq)
 		adv7511->bridge.ops |= DRM_BRIDGE_OP_HPD;
 
+	adv7511->bridge.vendor = "Analog";
+	adv7511->bridge.product = adv7511->info->name;
+	adv7511->bridge.hdmi_dev = dev;
+
+#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO
+	adv7511->bridge.hdmi_audio_max_i2s_playback_channels = 2;
+	adv7511->bridge.hdmi_audio_i2s_formats = (SNDRV_PCM_FMTBIT_S16_LE |
+						  SNDRV_PCM_FMTBIT_S20_3LE |
+						  SNDRV_PCM_FMTBIT_S24_3LE |
+						  SNDRV_PCM_FMTBIT_S24_LE |
+						  SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE),
+	adv7511->bridge.hdmi_audio_spdif_playback = 1;
+	adv7511->bridge.hdmi_audio_dai_port = 2;
+#endif
+
+#ifdef CONFIG_DRM_I2C_ADV7511_CEC
+	adv7511->bridge.hdmi_cec_adapter_name = dev_name(dev);
+	adv7511->bridge.hdmi_cec_available_las = ADV7511_MAX_ADDRS;
+#endif
+
 	adv7511->bridge.of_node = dev->of_node;
 	adv7511->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
 
 	drm_bridge_add(&adv7511->bridge);
 
-	adv7511_audio_init(dev, adv7511);
-
 	if (i2c->irq) {
 		init_waitqueue_head(&adv7511->wq);
 
@@ -1357,10 +1305,7 @@ static int adv7511_probe(struct i2c_client *i2c)
 	return 0;
 
 err_unregister_audio:
-	adv7511_audio_exit(adv7511);
 	drm_bridge_remove(&adv7511->bridge);
-err_unregister_cec:
-	cec_unregister_adapter(adv7511->cec_adap);
 	i2c_unregister_device(adv7511->i2c_cec);
 	clk_disable_unprepare(adv7511->cec_clk);
 err_i2c_unregister_packet:
@@ -1381,9 +1326,6 @@ static void adv7511_remove(struct i2c_client *i2c)
 
 	drm_bridge_remove(&adv7511->bridge);
 
-	adv7511_audio_exit(adv7511);
-
-	cec_unregister_adapter(adv7511->cec_adap);
 	i2c_unregister_device(adv7511->i2c_cec);
 	clk_disable_unprepare(adv7511->cec_clk);
 
@@ -1393,6 +1335,8 @@ static void adv7511_remove(struct i2c_client *i2c)
 
 static const struct adv7511_chip_info adv7511_chip_info = {
 	.type = ADV7511,
+	.name = "ADV7511",
+	.max_mode_clock_khz = 165000,
 	.supply_names = adv7511_supply_names,
 	.num_supplies = ARRAY_SIZE(adv7511_supply_names),
 	.link_config = true,
@@ -1400,6 +1344,7 @@ static const struct adv7511_chip_info adv7511_chip_info = {
 
 static const struct adv7511_chip_info adv7533_chip_info = {
 	.type = ADV7533,
+	.name = "ADV7533",
 	.max_mode_clock_khz = 80000,
 	.max_lane_freq_khz = 800000,
 	.supply_names = adv7533_supply_names,
@@ -1410,6 +1355,7 @@ static const struct adv7511_chip_info adv7533_chip_info = {
 
 static const struct adv7511_chip_info adv7535_chip_info = {
 	.type = ADV7535,
+	.name = "ADV7535",
 	.max_mode_clock_khz = 148500,
 	.max_lane_freq_khz = 891000,
 	.supply_names = adv7533_supply_names,
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
index 4481489aaf5ebf164313c86cbf3447d2d7914ab9..3ed0fa56bb97c53c2d578c242603e195a574dc14 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
@@ -106,10 +106,6 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
 	struct mipi_dsi_device *dsi = adv->dsi;
 	u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
 
-	/* Check max clock for either 7533 or 7535 */
-	if (mode->clock > adv->info->max_mode_clock_khz)
-		return MODE_CLOCK_HIGH;
-
 	/* Check max clock for each lane */
 	if (mode->clock * bpp > adv->info->max_lane_freq_khz * adv->num_dsi_lanes)
 		return MODE_CLOCK_HIGH;

-- 
2.39.5