[PATCH 0/7] drm/sun4i: dw-hdmi: Fix initialization & refactor

[PATCH 0/7] drm/sun4i: dw-hdmi: Fix initialization & refactor

[Jernej Skrabec]

Main goal of this series is to fix race condition between probing display
connector driver and sun8i dw-hdmi platform driver. Sometimes, boards have
ddc-en gpio specified in DT file. This is handled by display connector
driver since commit 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO
consumer conflict"). However, because there is no link between it and
sun8i dw-hdmi driver, probe order isn't determined. If display connector
driver if probed afterwards, then sun8i dw-hdmi driver won't be able to
read EDID and thus fall back to 1024x768. This can be easily solved by
using bridges and linking them together. Coincidentally, switching to
bridge model is also long term goal.

I found out some other issues when working on them (missing phy deinit and
memory corruption during executing fail path). Since there is now a bigger
chance of deferring probe, it's also good to skip reporting deferred probe
as error. This often confuses users when examining dmesg output, especially
if there is no error code reported.

I also throw 2 refactoring patches for a good measure.

Please take a look.

Best regards,
Jernej

Jernej Skrabec (7):
  drm/sun4i: dw-hdmi: Deinit PHY in fail path
  drm/sun4i: dw-hdmi: Remove double encoder cleanup
  drm/sun4i: dw-hdmi: Switch to bridge functions
  drm/sun4i: Don't show error for deferred probes.
  drm/sun4i: dw-hdmi: Split driver registration
  drm/sun4i: dw-hdmi: Make sun8i_hdmi_phy_get() more intuitive
  drm/sun4i: dw-hdmi: check for phy device first

 drivers/gpu/drm/sun4i/sun4i_drv.c      |   3 +-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c  | 191 ++++++++++++++++++-------
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h  |   9 +-
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c |  13 +-
 4 files changed, 151 insertions(+), 65 deletions(-)

-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/7] drm/sun4i: dw-hdmi: Deinit PHY in fail path

[Jernej Skrabec]

Commit 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform
device") removed code for PHY deinitialization in fail path.

Add it back.

Fixes: 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform device")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 4727dfaa8fb9..0b647b030b15 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -203,6 +203,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 
 cleanup_encoder:
 	drm_encoder_cleanup(encoder);
+	sun8i_hdmi_phy_deinit(hdmi->phy);
 err_disable_clk_tmds:
 	clk_disable_unprepare(hdmi->clk_tmds);
 err_assert_ctrl_reset:
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] drm/sun4i: dw-hdmi: Deinit PHY in fail path

[Maxime Ripard]

On Sun, 24 Sep 2023 21:25:58 +0200, Jernej Skrabec wrote:
> Commit 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform
> device") removed code for PHY deinitialization in fail path.
> 
> Add it back.
> 
> 
> [ ... ]

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

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/7] drm/sun4i: dw-hdmi: Remove double encoder cleanup

[Jernej Skrabec]

It turns out that comment is wrong - dw hdmi driver never does any
encoder cleanup. In fact, cleanup is done automatically, in destroy
callback of encoder. Even more, encoder memory will be freed when hdmi
device is destroyed. However, encoder will be cleaned up after that, in
drm_mode_config_cleanup(), which is called later. This will cause use
after free bug.

Remove redundant encoder cleanup, switch memory allocation to live as
long as drm object and while at it, check return code of encoder
initialization.

Fixes: b7c7436a5ff0 ("drm/sun4i: Implement A83T HDMI driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 0b647b030b15..8f8d3bdba5ce 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -8,6 +8,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_of.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -107,7 +108,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
-	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
 		return -ENOMEM;
 
@@ -180,7 +181,9 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 		goto err_disable_clk_tmds;
 
 	drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
+	ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
+	if (ret)
+		goto err_deinit_phy;
 
 	plat_data->mode_valid = hdmi->quirks->mode_valid;
 	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
@@ -189,20 +192,14 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	platform_set_drvdata(pdev, hdmi);
 
 	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
-
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
 	if (IS_ERR(hdmi->hdmi)) {
 		ret = PTR_ERR(hdmi->hdmi);
-		goto cleanup_encoder;
+		goto err_deinit_phy;
 	}
 
 	return 0;
 
-cleanup_encoder:
-	drm_encoder_cleanup(encoder);
+err_deinit_phy:
 	sun8i_hdmi_phy_deinit(hdmi->phy);
 err_disable_clk_tmds:
 	clk_disable_unprepare(hdmi->clk_tmds);
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] drm/sun4i: dw-hdmi: Remove double encoder cleanup

[Maxime Ripard]

On Sun, 24 Sep 2023 21:25:59 +0200, Jernej Skrabec wrote:
> It turns out that comment is wrong - dw hdmi driver never does any
> encoder cleanup. In fact, cleanup is done automatically, in destroy
> callback of encoder. Even more, encoder memory will be freed when hdmi
> device is destroyed. However, encoder will be cleaned up after that, in
> drm_mode_config_cleanup(), which is called later. This will cause use
> 
> [ ... ]

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

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/7] drm/sun4i: dw-hdmi: Switch to bridge functions

[Jernej Skrabec]

Since ddc-en property handling was moved from sun8i dw-hdmi driver to
display connector driver, probe order of drivers determines if EDID is
properly read at boot time or not.

In order to fix this, let's switch to bridge functions which allows us
to build proper chain and defer execution until all drivers are probed.

Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |   5 ++
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 8f8d3bdba5ce..93831cdf1917 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -8,14 +8,82 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_of.h>
 #include <drm/drm_simple_kms_helper.h>
 
+#include <media/cec-notifier.h>
+
 #include "sun8i_dw_hdmi.h"
 #include "sun8i_tcon_top.h"
 
+#define bridge_to_sun8i_dw_hdmi(x) \
+	container_of(x, struct sun8i_dw_hdmi, enc_bridge)
+
+static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge,
+				 enum drm_bridge_attach_flags flags)
+{
+	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
+
+	return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge,
+				 &hdmi->enc_bridge, flags);
+}
+
+static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge)
+{
+	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
+
+	cec_notifier_conn_unregister(hdmi->cec_notifier);
+	hdmi->cec_notifier = NULL;
+}
+
+static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge,
+				      enum drm_connector_status status)
+{
+	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
+	struct edid *edid;
+
+	if (!hdmi->cec_notifier)
+		return;
+
+	if (status == connector_status_connected) {
+		edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector);
+		if (edid)
+			cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier,
+							     edid);
+	} else {
+		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
+	}
+}
+
+static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge,
+				       struct drm_bridge_state *bridge_state,
+				       struct drm_crtc_state *crtc_state,
+				       struct drm_connector_state *conn_state)
+{
+	struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(conn_state->state,
+						   conn_state->connector);
+
+	if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state))
+		crtc_state->mode_changed = true;
+
+	return 0;
+}
+
+static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = {
+	.attach = sun8i_hdmi_enc_attach,
+	.detach = sun8i_hdmi_enc_detach,
+	.hpd_notify = sun8i_hdmi_enc_hpd_notify,
+	.atomic_check = sun8i_hdmi_enc_atomic_check,
+	.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,
+};
+
 static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 					   struct drm_display_mode *mode,
 					   struct drm_display_mode *adj_mode)
@@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_hdmi_plat_data *plat_data;
+	struct cec_connector_info conn_info;
+	struct drm_connector *connector;
 	struct drm_device *drm = data;
 	struct device_node *phy_node;
 	struct drm_encoder *encoder;
@@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 
 	plat_data->mode_valid = hdmi->quirks->mode_valid;
 	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
+	plat_data->output_port = 1;
 	sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
 
 	platform_set_drvdata(pdev, hdmi);
 
-	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
+	hdmi->hdmi = dw_hdmi_probe(pdev, plat_data);
 	if (IS_ERR(hdmi->hdmi)) {
 		ret = PTR_ERR(hdmi->hdmi);
 		goto err_deinit_phy;
 	}
 
+	hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node);
+
+	hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs;
+	hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA;
+	hdmi->enc_bridge.interlace_allowed = true;
+
+	drm_bridge_add(&hdmi->enc_bridge);
+
+	ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		goto err_remove_dw_hdmi;
+
+	connector = drm_bridge_connector_init(drm, encoder);
+	if (IS_ERR(connector)) {
+		dev_err(dev, "Unable to create HDMI bridge connector\n");
+		ret = PTR_ERR(connector);
+		goto err_remove_dw_hdmi;
+	}
+
+	hdmi->connector = connector;
+	drm_connector_attach_encoder(connector, encoder);
+
+	if (hdmi->quirks->use_drm_infoframe)
+		drm_connector_attach_hdr_output_metadata_property(connector);
+
+	cec_fill_conn_info_from_drm(&conn_info, connector);
+
+	hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL,
+							&conn_info);
+	if (!hdmi->cec_notifier) {
+		ret = -ENOMEM;
+		goto err_remove_dw_hdmi;
+	}
+
 	return 0;
 
+err_remove_dw_hdmi:
+	drm_bridge_remove(&hdmi->enc_bridge);
+	dw_hdmi_remove(hdmi->hdmi);
 err_deinit_phy:
 	sun8i_hdmi_phy_deinit(hdmi->phy);
 err_disable_clk_tmds:
@@ -216,7 +325,8 @@ static void sun8i_dw_hdmi_unbind(struct device *dev, struct device *master,
 {
 	struct sun8i_dw_hdmi *hdmi = dev_get_drvdata(dev);
 
-	dw_hdmi_unbind(hdmi->hdmi);
+	drm_bridge_remove(&hdmi->enc_bridge);
+	dw_hdmi_remove(hdmi->hdmi);
 	sun8i_hdmi_phy_deinit(hdmi->phy);
 	clk_disable_unprepare(hdmi->clk_tmds);
 	reset_control_assert(hdmi->rst_ctrl);
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index ab80d52a70bb..18ffc1b4841f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -7,6 +7,7 @@
 #define _SUN8I_DW_HDMI_H_
 
 #include <drm/bridge/dw_hdmi.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_encoder.h>
 #include <linux/clk.h>
 #include <linux/regmap.h>
@@ -178,9 +179,13 @@ struct sun8i_dw_hdmi_quirks {
 };
 
 struct sun8i_dw_hdmi {
+	struct cec_notifier		*cec_notifier;
 	struct clk			*clk_tmds;
+	struct drm_connector		*connector;
 	struct device			*dev;
+	struct drm_bridge		enc_bridge;
 	struct dw_hdmi			*hdmi;
+	struct drm_bridge		*hdmi_bridge;
 	struct drm_encoder		encoder;
 	struct sun8i_hdmi_phy		*phy;
 	struct dw_hdmi_plat_data	plat_data;
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] drm/sun4i: dw-hdmi: Switch to bridge functions

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6568 bytes --]

Hi,

On Sun, Sep 24, 2023 at 09:26:00PM +0200, Jernej Skrabec wrote:
> Since ddc-en property handling was moved from sun8i dw-hdmi driver to
> display connector driver, probe order of drivers determines if EDID is
> properly read at boot time or not.
> 
> In order to fix this, let's switch to bridge functions which allows us
> to build proper chain and defer execution until all drivers are probed.
> 
> Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++-
>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |   5 ++
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> index 8f8d3bdba5ce..93831cdf1917 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> @@ -8,14 +8,82 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> +#include <media/cec-notifier.h>
> +
>  #include "sun8i_dw_hdmi.h"
>  #include "sun8i_tcon_top.h"
>  
> +#define bridge_to_sun8i_dw_hdmi(x) \
> +	container_of(x, struct sun8i_dw_hdmi, enc_bridge)
> +
> +static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge,
> +				 enum drm_bridge_attach_flags flags)
> +{
> +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> +
> +	return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge,
> +				 &hdmi->enc_bridge, flags);
> +}
> +
> +static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge)
> +{
> +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> +
> +	cec_notifier_conn_unregister(hdmi->cec_notifier);
> +	hdmi->cec_notifier = NULL;
> +}
> +
> +static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge,
> +				      enum drm_connector_status status)
> +{
> +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> +	struct edid *edid;
> +
> +	if (!hdmi->cec_notifier)
> +		return;
> +
> +	if (status == connector_status_connected) {
> +		edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector);
> +		if (edid)
> +			cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier,
> +							     edid);
> +	} else {
> +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> +	}
> +}
> +
> +static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *bridge_state,
> +				       struct drm_crtc_state *crtc_state,
> +				       struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector_state *old_conn_state =
> +		drm_atomic_get_old_connector_state(conn_state->state,
> +						   conn_state->connector);
> +
> +	if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state))
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
> +}
> +
> +static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = {
> +	.attach = sun8i_hdmi_enc_attach,
> +	.detach = sun8i_hdmi_enc_detach,
> +	.hpd_notify = sun8i_hdmi_enc_hpd_notify,
> +	.atomic_check = sun8i_hdmi_enc_atomic_check,
> +	.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,
> +};
> +
>  static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>  					   struct drm_display_mode *mode,
>  					   struct drm_display_mode *adj_mode)
> @@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_hdmi_plat_data *plat_data;
> +	struct cec_connector_info conn_info;
> +	struct drm_connector *connector;
>  	struct drm_device *drm = data;
>  	struct device_node *phy_node;
>  	struct drm_encoder *encoder;
> @@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
>  
>  	plat_data->mode_valid = hdmi->quirks->mode_valid;
>  	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
> +	plat_data->output_port = 1;
>  	sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> -	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> +	hdmi->hdmi = dw_hdmi_probe(pdev, plat_data);
>  	if (IS_ERR(hdmi->hdmi)) {
>  		ret = PTR_ERR(hdmi->hdmi);
>  		goto err_deinit_phy;
>  	}
>  
> +	hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node);

So you're also adding child bridge support? This should be mentioned in
the commit log.

> +	hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs;
> +	hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> +	hdmi->enc_bridge.interlace_allowed = true;
> +
> +	drm_bridge_add(&hdmi->enc_bridge);
> +
> +	ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		goto err_remove_dw_hdmi;
> +
> +	connector = drm_bridge_connector_init(drm, encoder);
> +	if (IS_ERR(connector)) {
> +		dev_err(dev, "Unable to create HDMI bridge connector\n");
> +		ret = PTR_ERR(connector);
> +		goto err_remove_dw_hdmi;
> +	}
> +
> +	hdmi->connector = connector;
> +	drm_connector_attach_encoder(connector, encoder);
> +
> +	if (hdmi->quirks->use_drm_infoframe)
> +		drm_connector_attach_hdr_output_metadata_property(connector);
> +
> +	cec_fill_conn_info_from_drm(&conn_info, connector);
> +
> +	hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL,
> +							&conn_info);
> +	if (!hdmi->cec_notifier) {
> +		ret = -ENOMEM;
> +		goto err_remove_dw_hdmi;
> +	}
> +
>  	return 0;

Yeah, I'm not sure. We now have two different yet redundant set of
operations with no clear definition wrt what belongs where. I'm really
not impressed with the use of bridges for things that are clearly not
bridges.

The "nothing happens until all drivers are loaded" argument is also
supposed to be covered by the component framework, so why do we even
have to use a bridge here?

You were saying that it's an issue of probe order going wrong, could you
explain a bit more what goes wrong so we can try to figure something out
that doesn't involve a bridge?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] drm/sun4i: dw-hdmi: Switch to bridge functions

[Jernej Škrabec]

Dne ponedeljek, 25. september 2023 ob 09:57:22 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Sun, Sep 24, 2023 at 09:26:00PM +0200, Jernej Skrabec wrote:
> > Since ddc-en property handling was moved from sun8i dw-hdmi driver to
> > display connector driver, probe order of drivers determines if EDID is
> > properly read at boot time or not.
> > 
> > In order to fix this, let's switch to bridge functions which allows us
> > to build proper chain and defer execution until all drivers are probed.
> > 
> > Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++-
> >  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |   5 ++
> >  2 files changed, 117 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > index 8f8d3bdba5ce..93831cdf1917 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > @@ -8,14 +8,82 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  
> > +#include <drm/drm_atomic_state_helper.h>
> > +#include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_managed.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_simple_kms_helper.h>
> >  
> > +#include <media/cec-notifier.h>
> > +
> >  #include "sun8i_dw_hdmi.h"
> >  #include "sun8i_tcon_top.h"
> >  
> > +#define bridge_to_sun8i_dw_hdmi(x) \
> > +	container_of(x, struct sun8i_dw_hdmi, enc_bridge)
> > +
> > +static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge,
> > +				 enum drm_bridge_attach_flags flags)
> > +{
> > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > +
> > +	return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge,
> > +				 &hdmi->enc_bridge, flags);
> > +}
> > +
> > +static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge)
> > +{
> > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > +
> > +	cec_notifier_conn_unregister(hdmi->cec_notifier);
> > +	hdmi->cec_notifier = NULL;
> > +}
> > +
> > +static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge,
> > +				      enum drm_connector_status status)
> > +{
> > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > +	struct edid *edid;
> > +
> > +	if (!hdmi->cec_notifier)
> > +		return;
> > +
> > +	if (status == connector_status_connected) {
> > +		edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector);
> > +		if (edid)
> > +			cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier,
> > +							     edid);
> > +	} else {
> > +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> > +	}
> > +}
> > +
> > +static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge,
> > +				       struct drm_bridge_state *bridge_state,
> > +				       struct drm_crtc_state *crtc_state,
> > +				       struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_connector_state *old_conn_state =
> > +		drm_atomic_get_old_connector_state(conn_state->state,
> > +						   conn_state->connector);
> > +
> > +	if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state))
> > +		crtc_state->mode_changed = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = {
> > +	.attach = sun8i_hdmi_enc_attach,
> > +	.detach = sun8i_hdmi_enc_detach,
> > +	.hpd_notify = sun8i_hdmi_enc_hpd_notify,
> > +	.atomic_check = sun8i_hdmi_enc_atomic_check,
> > +	.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,
> > +};
> > +
> >  static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> >  					   struct drm_display_mode *mode,
> >  					   struct drm_display_mode *adj_mode)
> > @@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct dw_hdmi_plat_data *plat_data;
> > +	struct cec_connector_info conn_info;
> > +	struct drm_connector *connector;
> >  	struct drm_device *drm = data;
> >  	struct device_node *phy_node;
> >  	struct drm_encoder *encoder;
> > @@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> >  
> >  	plat_data->mode_valid = hdmi->quirks->mode_valid;
> >  	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
> > +	plat_data->output_port = 1;
> >  	sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> >  
> >  	platform_set_drvdata(pdev, hdmi);
> >  
> > -	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > +	hdmi->hdmi = dw_hdmi_probe(pdev, plat_data);
> >  	if (IS_ERR(hdmi->hdmi)) {
> >  		ret = PTR_ERR(hdmi->hdmi);
> >  		goto err_deinit_phy;
> >  	}
> >  
> > +	hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node);
> 
> So you're also adding child bridge support? This should be mentioned in
> the commit log.
> 
> > +	hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs;
> > +	hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > +	hdmi->enc_bridge.interlace_allowed = true;
> > +
> > +	drm_bridge_add(&hdmi->enc_bridge);
> > +
> > +	ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL,
> > +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +	if (ret)
> > +		goto err_remove_dw_hdmi;
> > +
> > +	connector = drm_bridge_connector_init(drm, encoder);
> > +	if (IS_ERR(connector)) {
> > +		dev_err(dev, "Unable to create HDMI bridge connector\n");
> > +		ret = PTR_ERR(connector);
> > +		goto err_remove_dw_hdmi;
> > +	}
> > +
> > +	hdmi->connector = connector;
> > +	drm_connector_attach_encoder(connector, encoder);
> > +
> > +	if (hdmi->quirks->use_drm_infoframe)
> > +		drm_connector_attach_hdr_output_metadata_property(connector);
> > +
> > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > +
> > +	hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL,
> > +							&conn_info);
> > +	if (!hdmi->cec_notifier) {
> > +		ret = -ENOMEM;
> > +		goto err_remove_dw_hdmi;
> > +	}
> > +
> >  	return 0;
> 
> Yeah, I'm not sure. We now have two different yet redundant set of
> operations with no clear definition wrt what belongs where. I'm really
> not impressed with the use of bridges for things that are clearly not
> bridges.

DRM bridges should be taken as more broad concept than just a physical
bridge.

> 
> The "nothing happens until all drivers are loaded" argument is also
> supposed to be covered by the component framework, so why do we even
> have to use a bridge here?

There is more than one reason:
- Display connector driver, which sets ddc-en gpio, is purely bridge driver.
- In long term, I plan to add format negotiation between display, dw-hdmi
  and DE3 (I already have WIP code). This is already implemented in bridge
  infrastructure.
- There is a plan to remove connector handling from DW HDMI handling
  and use display connector driver instead. This again involves bridges.
  sun4i-drm and rockchip-drm are the only remaining drivers, which are
  not yet converted.

> 
> You were saying that it's an issue of probe order going wrong, could you
> explain a bit more what goes wrong so we can try to figure something out
> that doesn't involve a bridge?

Not sure how to make this clearer than in cover letter and this commit
message. ddc-en pin is responsibility of display-connector driver (see
commit mentioned in fixes tag), so it must probe before sun8i dw hdmi.

Why are you so against bridges? As I explained above, format negotiation
is really implemented only there, so they are needed at some point
anyway.

Best regards,
Jernej



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] drm/sun4i: dw-hdmi: Switch to bridge functions

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 10650 bytes --]

On Mon, Sep 25, 2023 at 05:29:20PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 25. september 2023 ob 09:57:22 CEST je Maxime Ripard napisal(a):
> > On Sun, Sep 24, 2023 at 09:26:00PM +0200, Jernej Skrabec wrote:
> > > Since ddc-en property handling was moved from sun8i dw-hdmi driver to
> > > display connector driver, probe order of drivers determines if EDID is
> > > properly read at boot time or not.
> > > 
> > > In order to fix this, let's switch to bridge functions which allows us
> > > to build proper chain and defer execution until all drivers are probed.
> > > 
> > > Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict")
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++-
> > >  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |   5 ++
> > >  2 files changed, 117 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > index 8f8d3bdba5ce..93831cdf1917 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > @@ -8,14 +8,82 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  
> > > +#include <drm/drm_atomic_state_helper.h>
> > > +#include <drm/drm_bridge_connector.h>
> > >  #include <drm/drm_managed.h>
> > >  #include <drm/drm_modeset_helper_vtables.h>
> > >  #include <drm/drm_of.h>
> > >  #include <drm/drm_simple_kms_helper.h>
> > >  
> > > +#include <media/cec-notifier.h>
> > > +
> > >  #include "sun8i_dw_hdmi.h"
> > >  #include "sun8i_tcon_top.h"
> > >  
> > > +#define bridge_to_sun8i_dw_hdmi(x) \
> > > +	container_of(x, struct sun8i_dw_hdmi, enc_bridge)
> > > +
> > > +static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge,
> > > +				 enum drm_bridge_attach_flags flags)
> > > +{
> > > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > > +
> > > +	return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge,
> > > +				 &hdmi->enc_bridge, flags);
> > > +}
> > > +
> > > +static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge)
> > > +{
> > > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > > +
> > > +	cec_notifier_conn_unregister(hdmi->cec_notifier);
> > > +	hdmi->cec_notifier = NULL;
> > > +}
> > > +
> > > +static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge,
> > > +				      enum drm_connector_status status)
> > > +{
> > > +	struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > > +	struct edid *edid;
> > > +
> > > +	if (!hdmi->cec_notifier)
> > > +		return;
> > > +
> > > +	if (status == connector_status_connected) {
> > > +		edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector);
> > > +		if (edid)
> > > +			cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier,
> > > +							     edid);
> > > +	} else {
> > > +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> > > +	}
> > > +}
> > > +
> > > +static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge,
> > > +				       struct drm_bridge_state *bridge_state,
> > > +				       struct drm_crtc_state *crtc_state,
> > > +				       struct drm_connector_state *conn_state)
> > > +{
> > > +	struct drm_connector_state *old_conn_state =
> > > +		drm_atomic_get_old_connector_state(conn_state->state,
> > > +						   conn_state->connector);
> > > +
> > > +	if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state))
> > > +		crtc_state->mode_changed = true;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = {
> > > +	.attach = sun8i_hdmi_enc_attach,
> > > +	.detach = sun8i_hdmi_enc_detach,
> > > +	.hpd_notify = sun8i_hdmi_enc_hpd_notify,
> > > +	.atomic_check = sun8i_hdmi_enc_atomic_check,
> > > +	.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,
> > > +};
> > > +
> > >  static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> > >  					   struct drm_display_mode *mode,
> > >  					   struct drm_display_mode *adj_mode)
> > > @@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> > >  {
> > >  	struct platform_device *pdev = to_platform_device(dev);
> > >  	struct dw_hdmi_plat_data *plat_data;
> > > +	struct cec_connector_info conn_info;
> > > +	struct drm_connector *connector;
> > >  	struct drm_device *drm = data;
> > >  	struct device_node *phy_node;
> > >  	struct drm_encoder *encoder;
> > > @@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> > >  
> > >  	plat_data->mode_valid = hdmi->quirks->mode_valid;
> > >  	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
> > > +	plat_data->output_port = 1;
> > >  	sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> > >  
> > >  	platform_set_drvdata(pdev, hdmi);
> > >  
> > > -	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > > +	hdmi->hdmi = dw_hdmi_probe(pdev, plat_data);
> > >  	if (IS_ERR(hdmi->hdmi)) {
> > >  		ret = PTR_ERR(hdmi->hdmi);
> > >  		goto err_deinit_phy;
> > >  	}
> > >  
> > > +	hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node);
> > 
> > So you're also adding child bridge support? This should be mentioned in
> > the commit log.
> > 
> > > +	hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs;
> > > +	hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > > +	hdmi->enc_bridge.interlace_allowed = true;
> > > +
> > > +	drm_bridge_add(&hdmi->enc_bridge);
> > > +
> > > +	ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL,
> > > +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > > +	if (ret)
> > > +		goto err_remove_dw_hdmi;
> > > +
> > > +	connector = drm_bridge_connector_init(drm, encoder);
> > > +	if (IS_ERR(connector)) {
> > > +		dev_err(dev, "Unable to create HDMI bridge connector\n");
> > > +		ret = PTR_ERR(connector);
> > > +		goto err_remove_dw_hdmi;
> > > +	}
> > > +
> > > +	hdmi->connector = connector;
> > > +	drm_connector_attach_encoder(connector, encoder);
> > > +
> > > +	if (hdmi->quirks->use_drm_infoframe)
> > > +		drm_connector_attach_hdr_output_metadata_property(connector);
> > > +
> > > +	cec_fill_conn_info_from_drm(&conn_info, connector);
> > > +
> > > +	hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL,
> > > +							&conn_info);
> > > +	if (!hdmi->cec_notifier) {
> > > +		ret = -ENOMEM;
> > > +		goto err_remove_dw_hdmi;
> > > +	}
> > > +
> > >  	return 0;
> > 
> > Yeah, I'm not sure. We now have two different yet redundant set of
> > operations with no clear definition wrt what belongs where. I'm really
> > not impressed with the use of bridges for things that are clearly not
> > bridges.
> 
> DRM bridges should be taken as more broad concept than just a physical
> bridge.
> 
> > 
> > The "nothing happens until all drivers are loaded" argument is also
> > supposed to be covered by the component framework, so why do we even
> > have to use a bridge here?
> 
> There is more than one reason:
> - Display connector driver, which sets ddc-en gpio, is purely bridge driver.

You can still use a bridge with an encoder. Plus, there's strictly no
reason that driver should be a bridge driver only.

> - In long term, I plan to add format negotiation between display, dw-hdmi
>   and DE3 (I already have WIP code). This is already implemented in bridge
>   infrastructure.
> - There is a plan to remove connector handling from DW HDMI handling
>   and use display connector driver instead. This again involves bridges.
>   sun4i-drm and rockchip-drm are the only remaining drivers, which are
>   not yet converted.

And those are great examples of why bridges are a mess right now. They
have severe limitations, but yet we put more and more features into them
for no particular reason.

I don't know who decided that, but I was not part of that discussion.

> > You were saying that it's an issue of probe order going wrong, could you
> > explain a bit more what goes wrong so we can try to figure something out
> > that doesn't involve a bridge?
> 
> Not sure how to make this clearer than in cover letter and this commit
> message. ddc-en pin is responsibility of display-connector driver (see
> commit mentioned in fixes tag), so it must probe before sun8i dw hdmi.
> 
> Why are you so against bridges?

From a fundamental point of view, we went from a model where we had:

 Framebuffer -> Plane -> CRTC -> Encoder -> Connector -> Monitor / Panel / Bridge

 ^                                                   ^  ^                        ^
 +-------------------- Source -----------------------+  +---------- Sink --------+

To effectively merging together panels, bridges and encoders together
into bridges, and before the connector.

As this discussion shows, there's no separation anymore between a source
and a sink, and it gets hard to know what is what anymore.

From a more technical point of view, it makes it hard to share code.
Most of the bridges will no longer create their connector anymore, so if
you want to modify the connector or add attributes for some reason,
well, you can't anymore because you don't even know what structure /
driver it's going to use anymore.

Oh, and if you want to add a property, you can't either.

Bridges really are second-class citizen, but we're encouraging everyone
to move to a solution with less capabilities for some reason.

> As I explained above, format negotiation is really implemented only
> there, so they are needed at some point anyway.

Format negociation is often abused to handle HDMI format output
negociation. If that's what you meant, then it's very much implemented
in several drivers already without bridges (i915, vc4), and I have a
series to tackle that generically here:
https://lore.kernel.org/dri-devel/20230920-kms-hdmi-connector-state-v2-0-17932daddd7d@kernel.org/

The ones in the bridge don't follow the same logic though, so they end
up with a behaviour different to what most of the userspace app expect.

If you meant format negociation as in "my (actual) bridge can only take
a couple of formats and I need to make sure the encoder chooses the
right one", then yeah, that makes sense. That has nothing to do with
HDMI or dw-hdmi though.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/7] drm/sun4i: Don't show error for deferred probes.

[Jernej Skrabec]

Drivers probing in display pipeline can be deferred for many reasons.
Don't print error for such cases.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 6a8dfc022d3c..b4816a1b0be3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -88,7 +88,8 @@ static int sun4i_drv_bind(struct device *dev)
 
 	ret = component_bind_all(drm->dev, drm);
 	if (ret) {
-		dev_err(drm->dev, "Couldn't bind all pipelines components\n");
+		dev_err_probe(drm->dev, ret,
+			      "Couldn't bind all pipelines components\n");
 		goto cleanup_mode_config;
 	}
 
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] drm/sun4i: Don't show error for deferred probes.

[Maxime Ripard]

On Sun, 24 Sep 2023 21:26:01 +0200, Jernej Skrabec wrote:
> Drivers probing in display pipeline can be deferred for many reasons.
> Don't print error for such cases.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

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

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[Jernej Skrabec]

There is no reason to register two drivers in same place. Using macro
lowers amount of boilerplate code.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c  | 27 +-------------------------
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h  |  2 --
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c |  3 ++-
 3 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 93831cdf1917..d93e8ff71aae 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -378,32 +378,7 @@ static struct platform_driver sun8i_dw_hdmi_pltfm_driver = {
 		.of_match_table = sun8i_dw_hdmi_dt_ids,
 	},
 };
-
-static int __init sun8i_dw_hdmi_init(void)
-{
-	int ret;
-
-	ret = platform_driver_register(&sun8i_dw_hdmi_pltfm_driver);
-	if (ret)
-		return ret;
-
-	ret = platform_driver_register(&sun8i_hdmi_phy_driver);
-	if (ret) {
-		platform_driver_unregister(&sun8i_dw_hdmi_pltfm_driver);
-		return ret;
-	}
-
-	return ret;
-}
-
-static void __exit sun8i_dw_hdmi_exit(void)
-{
-	platform_driver_unregister(&sun8i_dw_hdmi_pltfm_driver);
-	platform_driver_unregister(&sun8i_hdmi_phy_driver);
-}
-
-module_init(sun8i_dw_hdmi_init);
-module_exit(sun8i_dw_hdmi_exit);
+module_platform_driver(sun8i_dw_hdmi_pltfm_driver);
 
 MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@siol.net>");
 MODULE_DESCRIPTION("Allwinner DW HDMI bridge");
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index 18ffc1b4841f..21e010deeb48 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -194,8 +194,6 @@ struct sun8i_dw_hdmi {
 	struct reset_control		*rst_ctrl;
 };
 
-extern struct platform_driver sun8i_hdmi_phy_driver;
-
 static inline struct sun8i_dw_hdmi *
 encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder)
 {
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
index 489ea94693ff..f917a979e4a4 100644
--- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
+++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
@@ -729,10 +729,11 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev)
 	return 0;
 }
 
-struct platform_driver sun8i_hdmi_phy_driver = {
+static struct platform_driver sun8i_hdmi_phy_driver = {
 	.probe  = sun8i_hdmi_phy_probe,
 	.driver = {
 		.name = "sun8i-hdmi-phy",
 		.of_match_table = sun8i_hdmi_phy_of_table,
 	},
 };
+module_platform_driver(sun8i_hdmi_phy_driver);
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[kernel test robot]

Hi Jernej,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc3 next-20230921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jernej-Skrabec/drm-sun4i-dw-hdmi-Deinit-PHY-in-fail-path/20230925-032818
base:   linus/master
patch link:    https://lore.kernel.org/r/20230924192604.3262187-6-jernej.skrabec%40gmail.com
patch subject: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration
config: parisc-randconfig-002-20230925 (https://download.01.org/0day-ci/archive/20230925/202309251030.rZTXXyFE-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230925/202309251030.rZTXXyFE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309251030.rZTXXyFE-lkp@intel.com/

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o: in function `sun8i_hdmi_phy_driver_init':
>> (.init.text+0x0): multiple definition of `init_module'; drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o:(.init.text+0x0): first defined here
   hppa-linux-ld: drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o: in function `sun8i_hdmi_phy_driver_exit':
>> (.exit.text+0x0): multiple definition of `cleanup_module'; drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o:(.exit.text+0x0): first defined here

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 319 bytes --]

On Sun, Sep 24, 2023 at 09:26:02PM +0200, Jernej Skrabec wrote:
> There is no reason to register two drivers in same place. Using macro
> lowers amount of boilerplate code.

There's one actually: you can't have several module_init functions in
the some module, and both files are compiled into the same module.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[Jernej Škrabec]

Dne ponedeljek, 25. september 2023 ob 09:47:15 CEST je Maxime Ripard napisal(a):
> On Sun, Sep 24, 2023 at 09:26:02PM +0200, Jernej Skrabec wrote:
> > There is no reason to register two drivers in same place. Using macro
> > lowers amount of boilerplate code.
> 
> There's one actually: you can't have several module_init functions in
> the some module, and both files are compiled into the same module.

Yeah, I figured as much. However, I think code clean up is good enough reason
to add hidden option in Kconfig and extra entry in Makefile (in v2).

Do you agree?

Best regards,
Jernej 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 918 bytes --]

On Mon, Sep 25, 2023 at 05:07:45PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 25. september 2023 ob 09:47:15 CEST je Maxime Ripard napisal(a):
> > On Sun, Sep 24, 2023 at 09:26:02PM +0200, Jernej Skrabec wrote:
> > > There is no reason to register two drivers in same place. Using macro
> > > lowers amount of boilerplate code.
> > 
> > There's one actually: you can't have several module_init functions in
> > the some module, and both files are compiled into the same module.
> 
> Yeah, I figured as much. However, I think code clean up is good enough reason
> to add hidden option in Kconfig and extra entry in Makefile (in v2).
> 
> Do you agree?

Yeah, I don't know. Adding more modules makes it more difficult to
handle (especially autoloading) without a clear argument why.
Module_init is simple enough as it is currently, maybe we should just
add a comment to make it clearer?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[Jernej Škrabec]

Dne četrtek, 05. oktober 2023 ob 10:43:14 CEST je Maxime Ripard napisal(a):
> On Mon, Sep 25, 2023 at 05:07:45PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 25. september 2023 ob 09:47:15 CEST je Maxime Ripard napisal(a):
> > > On Sun, Sep 24, 2023 at 09:26:02PM +0200, Jernej Skrabec wrote:
> > > > There is no reason to register two drivers in same place. Using macro
> > > > lowers amount of boilerplate code.
> > > 
> > > There's one actually: you can't have several module_init functions in
> > > the some module, and both files are compiled into the same module.
> > 
> > Yeah, I figured as much. However, I think code clean up is good enough reason
> > to add hidden option in Kconfig and extra entry in Makefile (in v2).
> > 
> > Do you agree?
> 
> Yeah, I don't know. Adding more modules makes it more difficult to
> handle (especially autoloading) without a clear argument why.
> Module_init is simple enough as it is currently, maybe we should just
> add a comment to make it clearer?

I'll just drop this patch then. While I think autoloading works pretty good
these days and cleaner code is nice, it can certainly cause some issues while
packaging.

Best regards,
Jernej




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration

[kernel test robot]

Hi Jernej,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc3 next-20230925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jernej-Skrabec/drm-sun4i-dw-hdmi-Deinit-PHY-in-fail-path/20230925-032818
base:   linus/master
patch link:    https://lore.kernel.org/r/20230924192604.3262187-6-jernej.skrabec%40gmail.com
patch subject: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20230926/202309260027.aNIjQRBI-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230926/202309260027.aNIjQRBI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309260027.aNIjQRBI-lkp@intel.com/

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o: in function `sun8i_hdmi_phy_driver_init':
>> sun8i_hdmi_phy.c:(.init.text+0x0): multiple definition of `init_module'; drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o:sun8i_dw_hdmi.c:(.init.text+0x0): first defined here
   s390-linux-ld: drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o: in function `sun8i_hdmi_phy_driver_exit':
>> sun8i_hdmi_phy.c:(.exit.text+0x0): multiple definition of `cleanup_module'; drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o:sun8i_dw_hdmi.c:(.exit.text+0x0): first defined here

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/7] drm/sun4i: dw-hdmi: Make sun8i_hdmi_phy_get() more intuitive

[Jernej Skrabec]

Let's make sun8i_hdmi_phy_get() to behave more like other kernel
functions and return phy pointer instead of setting field in struct.
This also makes function more universal.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c  |  5 +++--
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h  |  2 +-
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 10 ++++------
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index d93e8ff71aae..41f815a1faec 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -239,10 +239,11 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 		goto err_disable_clk_tmds;
 	}
 
-	ret = sun8i_hdmi_phy_get(hdmi, phy_node);
+	hdmi->phy = sun8i_hdmi_phy_get(phy_node);
 	of_node_put(phy_node);
-	if (ret) {
+	if (IS_ERR(hdmi->phy)) {
 		dev_err(dev, "Couldn't get the HDMI PHY\n");
+		ret = PTR_ERR(hdmi->phy);
 		goto err_disable_clk_tmds;
 	}
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index 21e010deeb48..748b6a4d9cdd 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -200,7 +200,7 @@ encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder)
 	return container_of(encoder, struct sun8i_dw_hdmi, encoder);
 }
 
-int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node);
+struct sun8i_hdmi_phy *sun8i_hdmi_phy_get(struct device_node *node);
 
 int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy);
 void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy);
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
index f917a979e4a4..1c9bdefed35e 100644
--- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
+++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
@@ -650,25 +650,23 @@ static const struct of_device_id sun8i_hdmi_phy_of_table[] = {
 	{ /* sentinel */ }
 };
 
-int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
+struct sun8i_hdmi_phy *sun8i_hdmi_phy_get(struct device_node *node)
 {
 	struct platform_device *pdev = of_find_device_by_node(node);
 	struct sun8i_hdmi_phy *phy;
 
 	if (!pdev)
-		return -EPROBE_DEFER;
+		return ERR_PTR(-EPROBE_DEFER);
 
 	phy = platform_get_drvdata(pdev);
 	if (!phy) {
 		put_device(&pdev->dev);
-		return -EPROBE_DEFER;
+		return ERR_PTR(-EPROBE_DEFER);
 	}
 
-	hdmi->phy = phy;
-
 	put_device(&pdev->dev);
 
-	return 0;
+	return phy;
 }
 
 static int sun8i_hdmi_phy_probe(struct platform_device *pdev)
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/7] drm/sun4i: dw-hdmi: check for phy device first

[Jernej Skrabec]

Let's check for phy device first. Since it uses much of the same clocks
and resets it also lowers amount of possible deferred probes.

While at it, don't report error for deferred phy probe.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 35 +++++++++++++--------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 41f815a1faec..c1becd964326 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -173,11 +173,24 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	struct device_node *phy_node;
 	struct drm_encoder *encoder;
 	struct sun8i_dw_hdmi *hdmi;
+	struct sun8i_hdmi_phy *phy;
 	int ret;
 
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
+	phy_node = of_parse_phandle(dev->of_node, "phys", 0);
+	if (!phy_node) {
+		dev_err(dev, "Can't find PHY phandle\n");
+		return -EINVAL;
+	}
+
+	phy = sun8i_hdmi_phy_get(phy_node);
+	of_node_put(phy_node);
+	if (IS_ERR(phy))
+		return dev_err_probe(dev, PTR_ERR(phy),
+				     "Couldn't get the HDMI PHY\n");
+
 	hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
 		return -ENOMEM;
@@ -185,6 +198,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	plat_data = &hdmi->plat_data;
 	hdmi->dev = &pdev->dev;
 	encoder = &hdmi->encoder;
+	hdmi->phy = phy;
 
 	hdmi->quirks = of_device_get_match_data(dev);
 
@@ -232,22 +246,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 		goto err_assert_ctrl_reset;
 	}
 
-	phy_node = of_parse_phandle(dev->of_node, "phys", 0);
-	if (!phy_node) {
-		dev_err(dev, "Can't found PHY phandle\n");
-		ret = -EINVAL;
-		goto err_disable_clk_tmds;
-	}
-
-	hdmi->phy = sun8i_hdmi_phy_get(phy_node);
-	of_node_put(phy_node);
-	if (IS_ERR(hdmi->phy)) {
-		dev_err(dev, "Couldn't get the HDMI PHY\n");
-		ret = PTR_ERR(hdmi->phy);
-		goto err_disable_clk_tmds;
-	}
-
-	ret = sun8i_hdmi_phy_init(hdmi->phy);
+	ret = sun8i_hdmi_phy_init(phy);
 	if (ret)
 		goto err_disable_clk_tmds;
 
@@ -259,7 +258,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	plat_data->mode_valid = hdmi->quirks->mode_valid;
 	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
 	plat_data->output_port = 1;
-	sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
+	sun8i_hdmi_phy_set_ops(phy, plat_data);
 
 	platform_set_drvdata(pdev, hdmi);
 
@@ -310,7 +309,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 	drm_bridge_remove(&hdmi->enc_bridge);
 	dw_hdmi_remove(hdmi->hdmi);
 err_deinit_phy:
-	sun8i_hdmi_phy_deinit(hdmi->phy);
+	sun8i_hdmi_phy_deinit(phy);
 err_disable_clk_tmds:
 	clk_disable_unprepare(hdmi->clk_tmds);
 err_assert_ctrl_reset:
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] drm/sun4i: dw-hdmi: check for phy device first

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 361 bytes --]

On Sun, Sep 24, 2023 at 09:26:04PM +0200, Jernej Skrabec wrote:
> Let's check for phy device first. Since it uses much of the same clocks
> and resets it also lowers amount of possible deferred probes.

Much of the same clocks and resets than what? The HDMI controller?

If so, with the commit log modified

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

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel