[PATCH v2 0/5] Add interconnent support for simpledrm/simplefb

[PATCH v2 0/5] Add interconnent support for simpledrm/simplefb

[Luca Weiss]

Some devices might require keeping an interconnect path alive so that
the framebuffer continues working. Add support for that by setting the
bandwidth requirements appropriately for all provided interconnect
paths.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Changes in v2:
- Sort the headers before adding the new interconnect header, in
  separate commits.
- Use the correct #if guards for new interconnect code
- Pick up tags
- Link to v1: https://lore.kernel.org/r/20250620-simple-drm-fb-icc-v1-0-d92142e8f74f@fairphone.com

---
Luca Weiss (5):
      dt-bindings: display: simple-framebuffer: Add interconnects property
      drm/sysfb: simpledrm: Sort headers correctly
      drm/sysfb: simpledrm: Add support for interconnect paths
      fbdev/simplefb: Sort headers correctly
      fbdev/simplefb: Add support for interconnect paths

 .../bindings/display/simple-framebuffer.yaml       |  3 +
 drivers/gpu/drm/sysfb/simpledrm.c                  | 85 ++++++++++++++++++++-
 drivers/video/fbdev/simplefb.c                     | 89 +++++++++++++++++++++-
 3 files changed, 173 insertions(+), 4 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250617-simple-drm-fb-icc-89461c559913

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

[PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Luca Weiss]

Document the interconnects property which is a list of interconnect
paths that is used by the framebuffer and therefore needs to be kept
alive when the framebuffer is being used.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -79,6 +79,9 @@ properties:
   power-domains:
     description: List of power domains used by the framebuffer.
 
+  interconnects:
+    description: List of interconnect paths used by the framebuffer.
+
   width:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: Width of the framebuffer in pixels

-- 
2.50.0

[PATCH v2 2/5] drm/sysfb: simpledrm: Sort headers correctly

[Luca Weiss]

Make sure the headers are sorted alphabetically to ensure consistent
code.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index a1c3119330deffc9e122b83941f3697e5b87f277..349219330314e3421a6bb26ad5cf39a679a5cb7a 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -2,9 +2,9 @@
 
 #include <linux/aperture.h>
 #include <linux/clk.h>
-#include <linux/of_clk.h>
 #include <linux/minmax.h>
 #include <linux/of_address.h>
+#include <linux/of_clk.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>

-- 
2.50.0

[PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Luca Weiss]

Some devices might require keeping an interconnect path alive so that
the framebuffer continues working. Add support for that by setting the
bandwidth requirements appropriately for all provided interconnect
paths.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -2,6 +2,7 @@
 
 #include <linux/aperture.h>
 #include <linux/clk.h>
+#include <linux/interconnect.h>
 #include <linux/minmax.h>
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
@@ -225,6 +226,10 @@ struct simpledrm_device {
 	struct device **pwr_dom_devs;
 	struct device_link **pwr_dom_links;
 #endif
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+	unsigned int icc_count;
+	struct icc_path **icc_paths;
+#endif
 
 	/* modesetting */
 	u32 formats[DRM_SYSFB_PLANE_NFORMATS(1)];
@@ -547,6 +552,81 @@ static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
 }
 #endif
 
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+/*
+ * Generic interconnect path handling code.
+ */
+static void simpledrm_device_detach_icc(void *res)
+{
+	struct simpledrm_device *sdev = res;
+	int i;
+
+	for (i = sdev->icc_count - 1; i >= 0; i--) {
+		if (!IS_ERR_OR_NULL(sdev->icc_paths[i]))
+			icc_put(sdev->icc_paths[i]);
+	}
+}
+
+static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
+{
+	struct device *dev = sdev->sysfb.dev.dev;
+	int ret, count, i;
+
+	count = of_count_phandle_with_args(dev->of_node, "interconnects",
+							 "#interconnect-cells");
+	if (count < 0)
+		return 0;
+
+	/* An interconnect path consists of two elements */
+	if (count % 2) {
+		drm_err(&sdev->sysfb.dev,
+			"invalid interconnects value\n");
+		return -EINVAL;
+	}
+	sdev->icc_count = count / 2;
+
+	sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
+					       sizeof(*sdev->icc_paths),
+					       GFP_KERNEL);
+	if (!sdev->icc_paths)
+		return -ENOMEM;
+
+	for (i = 0; i < sdev->icc_count; i++) {
+		sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
+		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
+			ret = PTR_ERR(sdev->icc_paths[i]);
+			if (ret == -EPROBE_DEFER)
+				goto err;
+			drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
+				i, ret);
+			continue;
+		}
+
+		ret = icc_set_bw(sdev->icc_paths[i], 0, UINT_MAX);
+		if (ret) {
+			drm_err(&sdev->sysfb.dev, "failed to set interconnect bandwidth %u: %d\n",
+				i, ret);
+			continue;
+		}
+	}
+
+	return devm_add_action_or_reset(dev, simpledrm_device_detach_icc, sdev);
+
+err:
+	while (i) {
+		--i;
+		if (!IS_ERR_OR_NULL(sdev->icc_paths[i]))
+			icc_put(sdev->icc_paths[i]);
+	}
+	return ret;
+}
+#else
+static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
+{
+	return 0;
+}
+#endif
+
 /*
  * Modesetting
  */
@@ -633,6 +713,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 	ret = simpledrm_device_attach_genpd(sdev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = simpledrm_device_attach_icc(sdev);
 	if (ret)
 		return ERR_PTR(ret);
 

-- 
2.50.0

[PATCH v2 4/5] fbdev/simplefb: Sort headers correctly

[Luca Weiss]

Make sure the headers are sorted alphabetically to ensure consistent
code.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/video/fbdev/simplefb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index be95fcddce4c8ca794826b805cd7dad2985bd637..db27d51046af5cc3c46a0bc81ad9d9ed9a0783cc 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -13,18 +13,18 @@
  */
 
 #include <linux/aperture.h>
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/platform_data/simplefb.h>
-#include <linux/platform_device.h>
-#include <linux/clk.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
+#include <linux/platform_data/simplefb.h>
+#include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/regulator/consumer.h>
 

-- 
2.50.0

[PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Luca Weiss]

Some devices might require keeping an interconnect path alive so that
the framebuffer continues working. Add support for that by setting the
bandwidth requirements appropriately for all provided interconnect
paths.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/video/fbdev/simplefb.c | 83 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index db27d51046af5cc3c46a0bc81ad9d9ed9a0783cc..b7e2f2374e3149866fd6f1803931e7f34dbbd75f 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -16,6 +16,7 @@
 #include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
+#include <linux/interconnect.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -89,6 +90,10 @@ struct simplefb_par {
 	u32 regulator_count;
 	struct regulator **regulators;
 #endif
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+	unsigned int icc_count;
+	struct icc_path **icc_paths;
+#endif
 };
 
 static void simplefb_clocks_destroy(struct simplefb_par *par);
@@ -525,6 +530,80 @@ static int simplefb_attach_genpds(struct simplefb_par *par,
 }
 #endif
 
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+/*
+ * Generic interconnect path handling code.
+ */
+static void simplefb_detach_icc(void *res)
+{
+	struct simplefb_par *par = res;
+	int i;
+
+	for (i = par->icc_count - 1; i >= 0; i--) {
+		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
+			icc_put(par->icc_paths[i]);
+	}
+}
+
+static int simplefb_attach_icc(struct simplefb_par *par,
+			       struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret, count, i;
+
+	count = of_count_phandle_with_args(dev->of_node, "interconnects",
+							 "#interconnect-cells");
+	if (count < 0)
+		return 0;
+
+	/* An interconnect path consists of two elements */
+	if (count % 2) {
+		dev_err(dev, "invalid interconnects value\n");
+		return -EINVAL;
+	}
+	par->icc_count = count / 2;
+
+	par->icc_paths = devm_kcalloc(dev, par->icc_count,
+				      sizeof(*par->icc_paths),
+				      GFP_KERNEL);
+	if (!par->icc_paths)
+		return -ENOMEM;
+
+	for (i = 0; i < par->icc_count; i++) {
+		par->icc_paths[i] = of_icc_get_by_index(dev, i);
+		if (IS_ERR_OR_NULL(par->icc_paths[i])) {
+			ret = PTR_ERR(par->icc_paths[i]);
+			if (ret == -EPROBE_DEFER)
+				goto err;
+			dev_err(dev, "failed to get interconnect path %u: %d\n", i, ret);
+			continue;
+		}
+
+		ret = icc_set_bw(par->icc_paths[i], 0, UINT_MAX);
+		if (ret) {
+			dev_err(dev, "failed to set interconnect bandwidth %u: %d\n", i, ret);
+			continue;
+		}
+	}
+
+	return devm_add_action_or_reset(dev, simplefb_detach_icc, par);
+
+err:
+	while (i) {
+		--i;
+		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
+			icc_put(par->icc_paths[i]);
+	}
+	return ret;
+}
+#else
+static int simplefb_attach_icc(struct simplefb_par *par,
+			       struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static int simplefb_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -615,6 +694,10 @@ static int simplefb_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_regulators;
 
+	ret = simplefb_attach_icc(par, pdev);
+	if (ret < 0)
+		goto error_regulators;
+
 	simplefb_clocks_enable(par, pdev);
 	simplefb_regulators_enable(par, pdev);
 

-- 
2.50.0

Re: [PATCH v2 0/5] Add interconnent support for simpledrm/simplefb

[Hans de Goede]

Hi,

On 23-Jun-25 08:44, Luca Weiss wrote:
> Some devices might require keeping an interconnect path alive so that
> the framebuffer continues working. Add support for that by setting the
> bandwidth requirements appropriately for all provided interconnect
> paths.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
> Changes in v2:
> - Sort the headers before adding the new interconnect header, in
>   separate commits.
> - Use the correct #if guards for new interconnect code
> - Pick up tags
> - Link to v1: https://lore.kernel.org/r/20250620-simple-drm-fb-icc-v1-0-d92142e8f74f@fairphone.com

Thanks, the entire series looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

for the series.

Regards,

Hans


> 
> ---
> Luca Weiss (5):
>       dt-bindings: display: simple-framebuffer: Add interconnects property
>       drm/sysfb: simpledrm: Sort headers correctly
>       drm/sysfb: simpledrm: Add support for interconnect paths
>       fbdev/simplefb: Sort headers correctly
>       fbdev/simplefb: Add support for interconnect paths
> 
>  .../bindings/display/simple-framebuffer.yaml       |  3 +
>  drivers/gpu/drm/sysfb/simpledrm.c                  | 85 ++++++++++++++++++++-
>  drivers/video/fbdev/simplefb.c                     | 89 +++++++++++++++++++++-
>  3 files changed, 173 insertions(+), 4 deletions(-)
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250617-simple-drm-fb-icc-89461c559913
> 
> Best regards,

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Javier Martinez Canillas]

Luca Weiss <luca.weiss@fairphone.com> writes:

Hello Luca,

> Document the interconnects property which is a list of interconnect
> paths that is used by the framebuffer and therefore needs to be kept
> alive when the framebuffer is being used.
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 2/5] drm/sysfb: simpledrm: Sort headers correctly

[Javier Martinez Canillas]

Luca Weiss <luca.weiss@fairphone.com> writes:

> Make sure the headers are sorted alphabetically to ensure consistent
> code.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 2/5] drm/sysfb: simpledrm: Sort headers correctly

[Thomas Zimmermann]

Am 23.06.25 um 08:44 schrieb Luca Weiss:
> Make sure the headers are sorted alphabetically to ensure consistent
> code.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/sysfb/simpledrm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index a1c3119330deffc9e122b83941f3697e5b87f277..349219330314e3421a6bb26ad5cf39a679a5cb7a 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -2,9 +2,9 @@
>   
>   #include <linux/aperture.h>
>   #include <linux/clk.h>
> -#include <linux/of_clk.h>
>   #include <linux/minmax.h>
>   #include <linux/of_address.h>
> +#include <linux/of_clk.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_domain.h>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 4/5] fbdev/simplefb: Sort headers correctly

[Thomas Zimmermann]

Am 23.06.25 um 08:44 schrieb Luca Weiss:
> Make sure the headers are sorted alphabetically to ensure consistent
> code.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/simplefb.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index be95fcddce4c8ca794826b805cd7dad2985bd637..db27d51046af5cc3c46a0bc81ad9d9ed9a0783cc 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -13,18 +13,18 @@
>    */
>   
>   #include <linux/aperture.h>
> +#include <linux/clk.h>
>   #include <linux/errno.h>
>   #include <linux/fb.h>
>   #include <linux/io.h>
>   #include <linux/module.h>
> -#include <linux/platform_data/simplefb.h>
> -#include <linux/platform_device.h>
> -#include <linux/clk.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
>   #include <linux/of_clk.h>
>   #include <linux/of_platform.h>
>   #include <linux/parser.h>
> +#include <linux/platform_data/simplefb.h>
> +#include <linux/platform_device.h>
>   #include <linux/pm_domain.h>
>   #include <linux/regulator/consumer.h>
>   
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Javier Martinez Canillas]

Luca Weiss <luca.weiss@fairphone.com> writes:

> Some devices might require keeping an interconnect path alive so that
> the framebuffer continues working. Add support for that by setting the
> bandwidth requirements appropriately for all provided interconnect
> paths.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -2,6 +2,7 @@
>  
>  #include <linux/aperture.h>
>  #include <linux/clk.h>
> +#include <linux/interconnect.h>
>  #include <linux/minmax.h>
>  #include <linux/of_address.h>
>  #include <linux/of_clk.h>
> @@ -225,6 +226,10 @@ struct simpledrm_device {
>  	struct device **pwr_dom_devs;
>  	struct device_link **pwr_dom_links;
>  #endif

Can you add a /* interconnects */ comment here? Similarly how there is one
for clocks, regulators, power domains, etc.

> +#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
> +	unsigned int icc_count;
> +	struct icc_path **icc_paths;
> +#endif
>  

...

> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
> +{
> +	struct device *dev = sdev->sysfb.dev.dev;
> +	int ret, count, i;
> +
> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
> +							 "#interconnect-cells");
> +	if (count < 0)
> +		return 0;
> +
> +	/* An interconnect path consists of two elements */
> +	if (count % 2) {
> +		drm_err(&sdev->sysfb.dev,
> +			"invalid interconnects value\n");
> +		return -EINVAL;
> +	}
> +	sdev->icc_count = count / 2;
> +
> +	sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
> +					       sizeof(*sdev->icc_paths),
> +					       GFP_KERNEL);
> +	if (!sdev->icc_paths)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sdev->icc_count; i++) {
> +		sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
> +		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
> +			ret = PTR_ERR(sdev->icc_paths[i]);
> +			if (ret == -EPROBE_DEFER)
> +				goto err;
> +			drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
> +				i, ret);

You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
case and also will get this message in the /sys/kernel/debug/devices_deferred
debugfs entry, as the reason why the probe deferral happened.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 4/5] fbdev/simplefb: Sort headers correctly

[Javier Martinez Canillas]

Luca Weiss <luca.weiss@fairphone.com> writes:

> Make sure the headers are sorted alphabetically to ensure consistent
> code.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Javier Martinez Canillas]

Luca Weiss <luca.weiss@fairphone.com> writes:

> Some devices might require keeping an interconnect path alive so that
> the framebuffer continues working. Add support for that by setting the
> bandwidth requirements appropriately for all provided interconnect
> paths.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/video/fbdev/simplefb.c | 83 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>

[...]

> +static void simplefb_detach_icc(void *res)
> +{
> +	struct simplefb_par *par = res;
> +	int i;
> +
> +	for (i = par->icc_count - 1; i >= 0; i--) {
> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
> +			icc_put(par->icc_paths[i]);
> +	}
> +}
> +
> +static int simplefb_attach_icc(struct simplefb_par *par,
> +			       struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret, count, i;
> +
> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
> +							 "#interconnect-cells");
> +	if (count < 0)
> +		return 0;
> +
> +	/* An interconnect path consists of two elements */
> +	if (count % 2) {
> +		dev_err(dev, "invalid interconnects value\n");
> +		return -EINVAL;
> +	}
> +	par->icc_count = count / 2;
> +
> +	par->icc_paths = devm_kcalloc(dev, par->icc_count,
> +				      sizeof(*par->icc_paths),
> +				      GFP_KERNEL);
> +	if (!par->icc_paths)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < par->icc_count; i++) {
> +		par->icc_paths[i] = of_icc_get_by_index(dev, i);
> +		if (IS_ERR_OR_NULL(par->icc_paths[i])) {
> +			ret = PTR_ERR(par->icc_paths[i]);
> +			if (ret == -EPROBE_DEFER)
> +				goto err;
> +			dev_err(dev, "failed to get interconnect path %u: %d\n", i, ret);
> +			continue;
> +		}
> +
> +		ret = icc_set_bw(par->icc_paths[i], 0, UINT_MAX);
> +		if (ret) {
> +			dev_err(dev, "failed to set interconnect bandwidth %u: %d\n", i, ret);
> +			continue;
> +		}
> +	}
> +
> +	return devm_add_action_or_reset(dev, simplefb_detach_icc, par);
> +
> +err:
> +	while (i) {
> +		--i;
> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
> +			icc_put(par->icc_paths[i]);
> +	}
> +	return ret;
> +}
> +#else

These two functions contain the same logic that you are using in the
simpledrm driver. I wonder if could be made helpers so that the code
isn't duplicated in both drivers.

But in any case it could be a follow-up of your series I think.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
> Document the interconnects property which is a list of interconnect
> paths that is used by the framebuffer and therefore needs to be kept
> alive when the framebuffer is being used.
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> @@ -79,6 +79,9 @@ properties:
>    power-domains:
>      description: List of power domains used by the framebuffer.
>  
> +  interconnects:
> +    description: List of interconnect paths used by the framebuffer.
> +

maxItems: 1, or this is not a simple FB anymore. Anything which needs
some sort of resources in unknown way is not simple anymore. You need
device specific bindings.

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Luca Weiss]

Hi Krzysztof,

On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>> Document the interconnects property which is a list of interconnect
>> paths that is used by the framebuffer and therefore needs to be kept
>> alive when the framebuffer is being used.
>> 
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> @@ -79,6 +79,9 @@ properties:
>>    power-domains:
>>      description: List of power domains used by the framebuffer.
>>  
>> +  interconnects:
>> +    description: List of interconnect paths used by the framebuffer.
>> +
>
> maxItems: 1, or this is not a simple FB anymore. Anything which needs
> some sort of resources in unknown way is not simple anymore. You need
> device specific bindings.

The bindings support an arbitrary number of clocks, regulators,
power-domains. Why should I artificially limit the interconnects to only
one?

The driver code also has that support added in this series.

Regards
Luca

>
> Best regards,
> Krzysztof

Re: [PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Luca Weiss]

On Fri Jun 27, 2025 at 9:56 AM CEST, Javier Martinez Canillas wrote:
> Luca Weiss <luca.weiss@fairphone.com> writes:
>
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  drivers/video/fbdev/simplefb.c | 83 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>
>
> [...]
>
>> +static void simplefb_detach_icc(void *res)
>> +{
>> +	struct simplefb_par *par = res;
>> +	int i;
>> +
>> +	for (i = par->icc_count - 1; i >= 0; i--) {
>> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
>> +			icc_put(par->icc_paths[i]);
>> +	}
>> +}
>> +
>> +static int simplefb_attach_icc(struct simplefb_par *par,
>> +			       struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret, count, i;
>> +
>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>> +							 "#interconnect-cells");
>> +	if (count < 0)
>> +		return 0;
>> +
>> +	/* An interconnect path consists of two elements */
>> +	if (count % 2) {
>> +		dev_err(dev, "invalid interconnects value\n");
>> +		return -EINVAL;
>> +	}
>> +	par->icc_count = count / 2;
>> +
>> +	par->icc_paths = devm_kcalloc(dev, par->icc_count,
>> +				      sizeof(*par->icc_paths),
>> +				      GFP_KERNEL);
>> +	if (!par->icc_paths)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < par->icc_count; i++) {
>> +		par->icc_paths[i] = of_icc_get_by_index(dev, i);
>> +		if (IS_ERR_OR_NULL(par->icc_paths[i])) {
>> +			ret = PTR_ERR(par->icc_paths[i]);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto err;
>> +			dev_err(dev, "failed to get interconnect path %u: %d\n", i, ret);
>> +			continue;
>> +		}
>> +
>> +		ret = icc_set_bw(par->icc_paths[i], 0, UINT_MAX);
>> +		if (ret) {
>> +			dev_err(dev, "failed to set interconnect bandwidth %u: %d\n", i, ret);
>> +			continue;
>> +		}
>> +	}
>> +
>> +	return devm_add_action_or_reset(dev, simplefb_detach_icc, par);
>> +
>> +err:
>> +	while (i) {
>> +		--i;
>> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
>> +			icc_put(par->icc_paths[i]);
>> +	}
>> +	return ret;
>> +}
>> +#else
>
> These two functions contain the same logic that you are using in the
> simpledrm driver. I wonder if could be made helpers so that the code
> isn't duplicated in both drivers.

I believe most resource handling code (clocks, regulators,
power-domains, plus now interconnect) should be pretty generic between
the two.

>
> But in any case it could be a follow-up of your series I think.

To be fair, I don't think I'll work on this, I've got plenty of Qualcomm
SoC-specific bits to work on :)

Regards
Luca

>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Re: [PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Javier Martinez Canillas]

"Luca Weiss" <luca.weiss@fairphone.com> writes:

> On Fri Jun 27, 2025 at 9:56 AM CEST, Javier Martinez Canillas wrote:

[...]

>> These two functions contain the same logic that you are using in the
>> simpledrm driver. I wonder if could be made helpers so that the code
>> isn't duplicated in both drivers.
>
> I believe most resource handling code (clocks, regulators,
> power-domains, plus now interconnect) should be pretty generic between
> the two.
>

Yeah.

>>
>> But in any case it could be a follow-up of your series I think.
>
> To be fair, I don't think I'll work on this, I've got plenty of Qualcomm
> SoC-specific bits to work on :)
>

That's OK :) It was just a drive by comment, but as said I don't think
that this code duplication should be a blocker for this patch series.
-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Javier Martinez Canillas]

"Luca Weiss" <luca.weiss@fairphone.com> writes:

> Hi Krzysztof,
>
> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>> Document the interconnects property which is a list of interconnect
>>> paths that is used by the framebuffer and therefore needs to be kept
>>> alive when the framebuffer is being used.
>>> 
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>  1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> @@ -79,6 +79,9 @@ properties:
>>>    power-domains:
>>>      description: List of power domains used by the framebuffer.
>>>  
>>> +  interconnects:
>>> +    description: List of interconnect paths used by the framebuffer.
>>> +
>>
>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>> some sort of resources in unknown way is not simple anymore. You need
>> device specific bindings.
>
> The bindings support an arbitrary number of clocks, regulators,
> power-domains. Why should I artificially limit the interconnects to only
> one?
>

I agree with Luca here. There are device specific bindings for the device
specific drivers. But this is about the generic drivers that are able to
scan out using a system provided framebuffer.

The display controller is setup by the firmware but it might need a set
of clocks, power domains, regulators, etc left enabled in order to work.

It's true that the "simple" is a misnomer, probably these drivers should
had been named sysfb and sysfbdrm, or something along those lines.

> The driver code also has that support added in this series.
>
> Regards
> Luca
>
>>
>> Best regards,
>> Krzysztof
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 27.06.25 um 10:08 schrieb Krzysztof Kozlowski:
> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>> Document the interconnects property which is a list of interconnect
>> paths that is used by the framebuffer and therefore needs to be kept
>> alive when the framebuffer is being used.
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>> @@ -79,6 +79,9 @@ properties:
>>     power-domains:
>>       description: List of power domains used by the framebuffer.
>>   
>> +  interconnects:
>> +    description: List of interconnect paths used by the framebuffer.
>> +
> maxItems: 1, or this is not a simple FB anymore. Anything which needs
> some sort of resources in unknown way is not simple anymore. You need
> device specific bindings.

In this context, 'simple' means that this device cannot change display 
modes or do graphics acceleration. The hardware itself is not 
necessarily simple. As Javier pointed out, it's initialized by firmware 
on the actual hardware. Think of 'VGA-for-ARM'. We need these resources 
to keep the display working.

Best regards
Thomas

>
> Best regards,
> Krzysztof
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Thomas Zimmermann]

Hi

Am 27.06.25 um 09:56 schrieb Javier Martinez Canillas:
> Luca Weiss <luca.weiss@fairphone.com> writes:
>
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   drivers/video/fbdev/simplefb.c | 83 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 83 insertions(+)
>>
> [...]
>
>> +static void simplefb_detach_icc(void *res)
>> +{
>> +	struct simplefb_par *par = res;
>> +	int i;
>> +
>> +	for (i = par->icc_count - 1; i >= 0; i--) {
>> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
>> +			icc_put(par->icc_paths[i]);
>> +	}
>> +}
>> +
>> +static int simplefb_attach_icc(struct simplefb_par *par,
>> +			       struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret, count, i;
>> +
>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>> +							 "#interconnect-cells");
>> +	if (count < 0)
>> +		return 0;
>> +
>> +	/* An interconnect path consists of two elements */
>> +	if (count % 2) {
>> +		dev_err(dev, "invalid interconnects value\n");
>> +		return -EINVAL;
>> +	}
>> +	par->icc_count = count / 2;
>> +
>> +	par->icc_paths = devm_kcalloc(dev, par->icc_count,
>> +				      sizeof(*par->icc_paths),
>> +				      GFP_KERNEL);
>> +	if (!par->icc_paths)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < par->icc_count; i++) {
>> +		par->icc_paths[i] = of_icc_get_by_index(dev, i);
>> +		if (IS_ERR_OR_NULL(par->icc_paths[i])) {
>> +			ret = PTR_ERR(par->icc_paths[i]);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto err;
>> +			dev_err(dev, "failed to get interconnect path %u: %d\n", i, ret);
>> +			continue;
>> +		}
>> +
>> +		ret = icc_set_bw(par->icc_paths[i], 0, UINT_MAX);
>> +		if (ret) {
>> +			dev_err(dev, "failed to set interconnect bandwidth %u: %d\n", i, ret);
>> +			continue;
>> +		}
>> +	}
>> +
>> +	return devm_add_action_or_reset(dev, simplefb_detach_icc, par);
>> +
>> +err:
>> +	while (i) {
>> +		--i;
>> +		if (!IS_ERR_OR_NULL(par->icc_paths[i]))
>> +			icc_put(par->icc_paths[i]);
>> +	}
>> +	return ret;
>> +}
>> +#else
> These two functions contain the same logic that you are using in the
> simpledrm driver. I wonder if could be made helpers so that the code
> isn't duplicated in both drivers.

No please not!. Any work should rather be directed towards deleting 
simplefb entirely.

Best regards
Thomas

>
> But in any case it could be a follow-up of your series I think.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 5/5] fbdev/simplefb: Add support for interconnect paths

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi

[...]

>> These two functions contain the same logic that you are using in the
>> simpledrm driver. I wonder if could be made helpers so that the code
>> isn't duplicated in both drivers.
>
> No please not!. Any work should rather be directed towards deleting 
> simplefb entirely.
>

That is a good point. You are correct that having some duplication to
make easier to get rid of the fbdev driver is a much better approach.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 27/06/2025 11:48, Luca Weiss wrote:
> Hi Krzysztof,
> 
> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>> Document the interconnects property which is a list of interconnect
>>> paths that is used by the framebuffer and therefore needs to be kept
>>> alive when the framebuffer is being used.
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> @@ -79,6 +79,9 @@ properties:
>>>    power-domains:
>>>      description: List of power domains used by the framebuffer.
>>>  
>>> +  interconnects:
>>> +    description: List of interconnect paths used by the framebuffer.
>>> +
>>
>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>> some sort of resources in unknown way is not simple anymore. You need
>> device specific bindings.
> 
> The bindings support an arbitrary number of clocks, regulators,
> power-domains. Why should I artificially limit the interconnects to only
> one?

And IMO they should not. Bindings are not supposed to be generic.

> 
> The driver code also has that support added in this series.

That's not the problem here.


Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 27/06/2025 13:34, Thomas Zimmermann wrote:
> Hi
> 
> Am 27.06.25 um 10:08 schrieb Krzysztof Kozlowski:
>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>> Document the interconnects property which is a list of interconnect
>>> paths that is used by the framebuffer and therefore needs to be kept
>>> alive when the framebuffer is being used.
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>   Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>> @@ -79,6 +79,9 @@ properties:
>>>     power-domains:
>>>       description: List of power domains used by the framebuffer.
>>>   
>>> +  interconnects:
>>> +    description: List of interconnect paths used by the framebuffer.
>>> +
>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>> some sort of resources in unknown way is not simple anymore. You need
>> device specific bindings.
> 
> In this context, 'simple' means that this device cannot change display 
> modes or do graphics acceleration. The hardware itself is not 
> necessarily simple. As Javier pointed out, it's initialized by firmware 

If hardware is not simple, then it needs specific bindings.

> on the actual hardware. Think of 'VGA-for-ARM'. We need these resources 
> to keep the display working.

I don't claim you do not need these resources. I claim device is not
simple thus does not suit rules for generic bindings. Generic bindings
are in general not allowed and we have them only for very, very simple
devices.

You say this is not simple device, so there you go - specific binding
for this complex (not-simple) device.

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede]

Hi Krzysztof,

On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
> On 27/06/2025 11:48, Luca Weiss wrote:
>> Hi Krzysztof,
>>
>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>> Document the interconnects property which is a list of interconnect
>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>> alive when the framebuffer is being used.
>>>>
>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> @@ -79,6 +79,9 @@ properties:
>>>>    power-domains:
>>>>      description: List of power domains used by the framebuffer.
>>>>  
>>>> +  interconnects:
>>>> +    description: List of interconnect paths used by the framebuffer.
>>>> +
>>>
>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>> some sort of resources in unknown way is not simple anymore. You need
>>> device specific bindings.
>>
>> The bindings support an arbitrary number of clocks, regulators,
>> power-domains. Why should I artificially limit the interconnects to only
>> one?
> 
> And IMO they should not. Bindings are not supposed to be generic.

The simplefb binding is a binding to allow keeping the firmware, e.g.
uboot setup framebuffer alive to e.g. show a boot splash until
the native display-engine drive loads. Needing display-engine
specific bindings totally contradicts the whole goal of 

It is generic by nature and I really do not see how clocks and
regulators are any different then interconnects here.

Note that we had a huge discussion about adding clock
and regulators to simplefb many years ago with pretty
much the same arguments against doing so. In the end it was
decided to add regulator and clocks support to the simplefb
bindings and non of the feared problems with e.g. ordening
of turning things on happened.

A big part of this is that the claiming of clks / regulators /
interconnects by the simplefb driver is there to keep things on,
so it happens before the kernel starts tuning off unused resources
IOW everything is already up and running and this really is about
avoiding turning things off.

Regards,

Hans

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 28.06.25 um 13:49 schrieb Krzysztof Kozlowski:
> On 27/06/2025 11:48, Luca Weiss wrote:
>> Hi Krzysztof,
>>
>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>> Document the interconnects property which is a list of interconnect
>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>> alive when the framebuffer is being used.
>>>>
>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> @@ -79,6 +79,9 @@ properties:
>>>>     power-domains:
>>>>       description: List of power domains used by the framebuffer.
>>>>   
>>>> +  interconnects:
>>>> +    description: List of interconnect paths used by the framebuffer.
>>>> +
>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>> some sort of resources in unknown way is not simple anymore. You need
>>> device specific bindings.
>> The bindings support an arbitrary number of clocks, regulators,
>> power-domains. Why should I artificially limit the interconnects to only
>> one?
> And IMO they should not. Bindings are not supposed to be generic.
>
>> The driver code also has that support added in this series.
> That's not the problem here.

Then could you please state the problem in clear terms? We're obviously 
not all on the same page here.

Best regards
Thomas

>
>
> Best regards,
> Krzysztof
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 28.06.25 um 13:50 schrieb Krzysztof Kozlowski:
> On 27/06/2025 13:34, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 27.06.25 um 10:08 schrieb Krzysztof Kozlowski:
>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>> Document the interconnects property which is a list of interconnect
>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>> alive when the framebuffer is being used.
>>>>
>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>> @@ -79,6 +79,9 @@ properties:
>>>>      power-domains:
>>>>        description: List of power domains used by the framebuffer.
>>>>    
>>>> +  interconnects:
>>>> +    description: List of interconnect paths used by the framebuffer.
>>>> +
>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>> some sort of resources in unknown way is not simple anymore. You need
>>> device specific bindings.
>> In this context, 'simple' means that this device cannot change display
>> modes or do graphics acceleration. The hardware itself is not
>> necessarily simple. As Javier pointed out, it's initialized by firmware
> If hardware is not simple, then it needs specific bindings.
>
>> on the actual hardware. Think of 'VGA-for-ARM'. We need these resources
>> to keep the display working.
> I don't claim you do not need these resources. I claim device is not
> simple thus does not suit rules for generic bindings. Generic bindings
> are in general not allowed and we have them only for very, very simple
> devices.
>
> You say this is not simple device, so there you go - specific binding
> for this complex (not-simple) device.

No, I didn't. I said that the device is simple. I did not say that the 
device's hardware is simple. Sounds nonsensical, but makes sense here. 
The simple-framebuffer is just the range of display memory that the 
firmware configured for printing boot-up messages. We use it for the 
kernel's output as well.  Being generic and simple is the exact raison 
d'etre for simple-framebuffer.  (The display property points to the 
actual hardware, but we don't need it.)

Best regards
Thomas

>
> Best regards,
> Krzysztof

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede]

Hi,

On 30-Jun-25 8:34 AM, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.06.25 um 13:50 schrieb Krzysztof Kozlowski:
>> On 27/06/2025 13:34, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 27.06.25 um 10:08 schrieb Krzysztof Kozlowski:
>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>> Document the interconnects property which is a list of interconnect
>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>> alive when the framebuffer is being used.
>>>>>
>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>      power-domains:
>>>>>        description: List of power domains used by the framebuffer.
>>>>>    +  interconnects:
>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>> +
>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>> some sort of resources in unknown way is not simple anymore. You need
>>>> device specific bindings.
>>> In this context, 'simple' means that this device cannot change display
>>> modes or do graphics acceleration. The hardware itself is not
>>> necessarily simple. As Javier pointed out, it's initialized by firmware
>> If hardware is not simple, then it needs specific bindings.
>>
>>> on the actual hardware. Think of 'VGA-for-ARM'. We need these resources
>>> to keep the display working.
>> I don't claim you do not need these resources. I claim device is not
>> simple thus does not suit rules for generic bindings. Generic bindings
>> are in general not allowed and we have them only for very, very simple
>> devices.
>>
>> You say this is not simple device, so there you go - specific binding
>> for this complex (not-simple) device.
> 
> No, I didn't. I said that the device is simple. I did not say that the device's hardware is simple. Sounds nonsensical, but makes sense here. The simple-framebuffer is just the range of display memory that the firmware configured for printing boot-up messages. We use it for the kernel's output as well.  Being generic and simple is the exact raison d'etre for simple-framebuffer.  (The display property points to the actual hardware, but we don't need it.)

I believe part of the problem here is the simple part of the simplefb
name in hindsight that is a mistake and we should have called the thing
firmware-framebuffer since its goal is to pass along a firmware setup
framebuffer to the OS for displaying stuff.

As for the argument for having a firmware-framebuffer not being allowed
because framebuffers are to complex to have a generic binding, that
ship has long sailed since we already have the simplefb binding.

And since we already have the binding I do not find this not being
simple a valid technical argument. That is an argument to allow
having a generic binding at all or to not have it at all, but here
we already have the binding and this is just about evolving the binding
with changing hw needs.

And again this reminds me very much of the whole clocks / regulators
addition to simplefb discussion we had over a decade ago. Back then
we had a huge thread, almost a flamefest with in my memory over
a 100 emails and back then the only argument against adding them
was also "it is not simple", which IMHO really is a non argument for
an already existing binding. Certainly it is not a good technical
argument.

During the last decade, after clocks and regulators were added to
the binding. simplefb has been used successfully on millions (billions?)
handover the firmware framebuffer to the OS for bootsplash use,
replacing various vendor hacks for this. Disallowing the addition of
interconnect support to the simplefb binding will only result in
various vendor hacks appearing in vendor kernels for this, which
I believe is something which we should try to avoid.

So as the maintainer of the simplefb kernel driver for over a decade
I strongly advice the DT maintainers to accept this bindings patch
and from my my side this still is:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 30.06.25 um 09:26 schrieb Hans de Goede:
> Hi,
>
> On 30-Jun-25 8:34 AM, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 28.06.25 um 13:50 schrieb Krzysztof Kozlowski:
>>> On 27/06/2025 13:34, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 27.06.25 um 10:08 schrieb Krzysztof Kozlowski:
>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>>> Document the interconnects property which is a list of interconnect
>>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>>> alive when the framebuffer is being used.
>>>>>>
>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>     Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>>       power-domains:
>>>>>>         description: List of power domains used by the framebuffer.
>>>>>>     +  interconnects:
>>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>>> +
>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>>> some sort of resources in unknown way is not simple anymore. You need
>>>>> device specific bindings.
>>>> In this context, 'simple' means that this device cannot change display
>>>> modes or do graphics acceleration. The hardware itself is not
>>>> necessarily simple. As Javier pointed out, it's initialized by firmware
>>> If hardware is not simple, then it needs specific bindings.
>>>
>>>> on the actual hardware. Think of 'VGA-for-ARM'. We need these resources
>>>> to keep the display working.
>>> I don't claim you do not need these resources. I claim device is not
>>> simple thus does not suit rules for generic bindings. Generic bindings
>>> are in general not allowed and we have them only for very, very simple
>>> devices.
>>>
>>> You say this is not simple device, so there you go - specific binding
>>> for this complex (not-simple) device.
>> No, I didn't. I said that the device is simple. I did not say that the device's hardware is simple. Sounds nonsensical, but makes sense here. The simple-framebuffer is just the range of display memory that the firmware configured for printing boot-up messages. We use it for the kernel's output as well.  Being generic and simple is the exact raison d'etre for simple-framebuffer.  (The display property points to the actual hardware, but we don't need it.)
> I believe part of the problem here is the simple part of the simplefb
> name in hindsight that is a mistake and we should have called the thing
> firmware-framebuffer since its goal is to pass along a firmware setup
> framebuffer to the OS for displaying stuff.

I totally feel you. In DRM land, we've also been upset about the naming. 
But well...


>
> As for the argument for having a firmware-framebuffer not being allowed
> because framebuffers are to complex to have a generic binding, that
> ship has long sailed since we already have the simplefb binding.
>
> And since we already have the binding I do not find this not being
> simple a valid technical argument. That is an argument to allow
> having a generic binding at all or to not have it at all, but here
> we already have the binding and this is just about evolving the binding
> with changing hw needs.

Exactly my point.

>
> And again this reminds me very much of the whole clocks / regulators
> addition to simplefb discussion we had over a decade ago. Back then
> we had a huge thread, almost a flamefest with in my memory over
> a 100 emails and back then the only argument against adding them
> was also "it is not simple", which IMHO really is a non argument for
> an already existing binding. Certainly it is not a good technical
> argument.
>
> During the last decade, after clocks and regulators were added to
> the binding. simplefb has been used successfully on millions (billions?)
> handover the firmware framebuffer to the OS for bootsplash use,
> replacing various vendor hacks for this. Disallowing the addition of
> interconnect support to the simplefb binding will only result in
> various vendor hacks appearing in vendor kernels for this, which
> I believe is something which we should try to avoid.

Exactly. And I'd also add that the current way of handling the situation 
is the only feasible one. Simple-framebuffer needs to be generic and 
compatible with existing and future hardware at minimal cost. The way of 
doing so, is to have a few properties, such as clocks, regulators and 
now interconnects, that the firmware clearly tells us about.  If we go 
with per-hardware/per-vendor nodes, simple-framebuffer loses its usefulness.


>
> So as the maintainer of the simplefb kernel driver for over a decade
> I strongly advice the DT maintainers to accept this bindings patch

As the maintainer of the simpledrm driver, I second this.

Best regards
Thomas

> and from my my side this still is:
>
> Reviewed-by: Hans de Goede <hansg@kernel.org>
>
> Regards,
>
> Hans
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 29/06/2025 14:07, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
>> On 27/06/2025 11:48, Luca Weiss wrote:
>>> Hi Krzysztof,
>>>
>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>> Document the interconnects property which is a list of interconnect
>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>> alive when the framebuffer is being used.
>>>>>
>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>    power-domains:
>>>>>      description: List of power domains used by the framebuffer.
>>>>>  
>>>>> +  interconnects:
>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>> +
>>>>
>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>> some sort of resources in unknown way is not simple anymore. You need
>>>> device specific bindings.
>>>
>>> The bindings support an arbitrary number of clocks, regulators,
>>> power-domains. Why should I artificially limit the interconnects to only
>>> one?
>>
>> And IMO they should not. Bindings are not supposed to be generic.
> 
> The simplefb binding is a binding to allow keeping the firmware, e.g.
> uboot setup framebuffer alive to e.g. show a boot splash until
> the native display-engine drive loads. Needing display-engine
> specific bindings totally contradicts the whole goal of 

No, it does not. DT is well designed for that through expressing
compatibility. I did not say you cannot have generic fallback for simple
use case.

But this (and previous patchset) grows this into generic binding ONLY
and that is not correct.


> 
> It is generic by nature and I really do not see how clocks and
> regulators are any different then interconnects here.

Yeah, they are also wrong. I already commented on this.

> 
> Note that we had a huge discussion about adding clock
> and regulators to simplefb many years ago with pretty
> much the same arguments against doing so. In the end it was
> decided to add regulator and clocks support to the simplefb
> bindings and non of the feared problems with e.g. ordening
> of turning things on happened.
> 
> A big part of this is that the claiming of clks / regulators /
> interconnects by the simplefb driver is there to keep things on,
> so it happens before the kernel starts tuning off unused resources
> IOW everything is already up and running and this really is about
> avoiding turning things off.

No one asks to drop them from the driver. I only want specific front
compatible which will list and constrain the properties. It is not
contradictory to your statements, U-boot support, driver support. I
really do not see ANY argument why this cannot follow standard DT rules.

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Maxime Ripard]

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

On Mon, Jun 30, 2025 at 10:24:06AM +0200, Krzysztof Kozlowski wrote:
> On 29/06/2025 14:07, Hans de Goede wrote:
> > Hi Krzysztof,
> > 
> > On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
> >> On 27/06/2025 11:48, Luca Weiss wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
> >>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
> >>>>> Document the interconnects property which is a list of interconnect
> >>>>> paths that is used by the framebuffer and therefore needs to be kept
> >>>>> alive when the framebuffer is being used.
> >>>>>
> >>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
> >>>>>  1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
> >>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>> @@ -79,6 +79,9 @@ properties:
> >>>>>    power-domains:
> >>>>>      description: List of power domains used by the framebuffer.
> >>>>>  
> >>>>> +  interconnects:
> >>>>> +    description: List of interconnect paths used by the framebuffer.
> >>>>> +
> >>>>
> >>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
> >>>> some sort of resources in unknown way is not simple anymore. You need
> >>>> device specific bindings.
> >>>
> >>> The bindings support an arbitrary number of clocks, regulators,
> >>> power-domains. Why should I artificially limit the interconnects to only
> >>> one?
> >>
> >> And IMO they should not. Bindings are not supposed to be generic.
> > 
> > The simplefb binding is a binding to allow keeping the firmware, e.g.
> > uboot setup framebuffer alive to e.g. show a boot splash until
> > the native display-engine drive loads. Needing display-engine
> > specific bindings totally contradicts the whole goal of 
> 
> No, it does not. DT is well designed for that through expressing
> compatibility. I did not say you cannot have generic fallback for simple
> use case.
> 
> But this (and previous patchset) grows this into generic binding ONLY
> and that is not correct.

Can we have a proper definition of what a correct device tree binding is
then?

It's a bit surprising to have *that* discussion over a binding that is
now well older than a decade now, and while there is definitely some
generic bindings in ePAPR/DT spec, like the CPU ones.

If you don't consider that spec to be correct DT bindings, please
provide a definition of what that is, and / or reasonable alternatives.

Also, no, a device specific binding isn't reasonable here, because we
*don't* have a device. From a technical standpoint, the firmware creates
the framebuffer, Linux just uses it. Just like you don't have a
device/platform specific compatible for PSCI, SCPI, et al.

And from a process standpoint, that driver is typically used years
before we even get to writing a driver for the actual display driver.
And since bindings are far from standard and actually pretty
opionionated, even if we submitted a binding to use a proper binding
without having a clear idea of what the hardware is, or what a driver
would want, we would end up with either a broken binding, or a broken
driver.

Maxime

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

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede]

Hi,

On 30-Jun-25 10:24 AM, Krzysztof Kozlowski wrote:
> On 29/06/2025 14:07, Hans de Goede wrote:
>> Hi Krzysztof,
>>
>> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
>>> On 27/06/2025 11:48, Luca Weiss wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>>> Document the interconnects property which is a list of interconnect
>>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>>> alive when the framebuffer is being used.
>>>>>>
>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>>    power-domains:
>>>>>>      description: List of power domains used by the framebuffer.
>>>>>>  
>>>>>> +  interconnects:
>>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>>> +
>>>>>
>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>>> some sort of resources in unknown way is not simple anymore. You need
>>>>> device specific bindings.
>>>>
>>>> The bindings support an arbitrary number of clocks, regulators,
>>>> power-domains. Why should I artificially limit the interconnects to only
>>>> one?
>>>
>>> And IMO they should not. Bindings are not supposed to be generic.
>>
>> The simplefb binding is a binding to allow keeping the firmware, e.g.
>> uboot setup framebuffer alive to e.g. show a boot splash until
>> the native display-engine drive loads. Needing display-engine
>> specific bindings totally contradicts the whole goal of 
> 
> No, it does not. DT is well designed for that through expressing
> compatibility. I did not say you cannot have generic fallback for simple
> use case.
> 
> But this (and previous patchset) grows this into generic binding ONLY
> and that is not correct.

I think that it is important here to notice that this is not
a generic fallback binding, this is not and will never be
intended to replace have a proper binding for
the display-engine.

This is just a way to give the kernel access to the firmware
setup framebuffer to e.g. show a bootsplash but also fatal
kernel errors until the real display-engine driver loads.

Note sometimes the whole framebuffer memory is not touched
at all and the sole reason for having a driver attach to
the simplefb node early on is just to keep the resources
needed to keep the panel lit up around (on) until the real
display-engine driver comes along to claim those resources.

This avoids the display going black if the display-engine
driver only binds after the kernel starts turning off
unused resources, this typically happens when the display-engine
driver is a module.

>> It is generic by nature and I really do not see how clocks and
>> regulators are any different then interconnects here.
> 
> Yeah, they are also wrong. I already commented on this.
> 
>>
>> Note that we had a huge discussion about adding clock
>> and regulators to simplefb many years ago with pretty
>> much the same arguments against doing so. In the end it was
>> decided to add regulator and clocks support to the simplefb
>> bindings and non of the feared problems with e.g. ordening
>> of turning things on happened.
>>
>> A big part of this is that the claiming of clks / regulators /
>> interconnects by the simplefb driver is there to keep things on,
>> so it happens before the kernel starts tuning off unused resources
>> IOW everything is already up and running and this really is about
>> avoiding turning things off.
> 
> No one asks to drop them from the driver. I only want specific front
> compatible which will list and constrain the properties. It is not
> contradictory to your statements, U-boot support, driver support. I
> really do not see ANY argument why this cannot follow standard DT rules.

So what you are saying is that you want something like:

framebuffer0: framebuffer@1d385000 {
	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
}

and that the binding for qcom.simple-framebuffer-sm8650-mdss
can then list interconnects ?

Regards,

Hans

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 30/06/2025 10:38, Maxime Ripard wrote:
> On Mon, Jun 30, 2025 at 10:24:06AM +0200, Krzysztof Kozlowski wrote:
>> On 29/06/2025 14:07, Hans de Goede wrote:
>>> Hi Krzysztof,
>>>
>>> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
>>>> On 27/06/2025 11:48, Luca Weiss wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>>>> Document the interconnects property which is a list of interconnect
>>>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>>>> alive when the framebuffer is being used.
>>>>>>>
>>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>>>    power-domains:
>>>>>>>      description: List of power domains used by the framebuffer.
>>>>>>>  
>>>>>>> +  interconnects:
>>>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>>>> +
>>>>>>
>>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>>>> some sort of resources in unknown way is not simple anymore. You need
>>>>>> device specific bindings.
>>>>>
>>>>> The bindings support an arbitrary number of clocks, regulators,
>>>>> power-domains. Why should I artificially limit the interconnects to only
>>>>> one?
>>>>
>>>> And IMO they should not. Bindings are not supposed to be generic.
>>>
>>> The simplefb binding is a binding to allow keeping the firmware, e.g.
>>> uboot setup framebuffer alive to e.g. show a boot splash until
>>> the native display-engine drive loads. Needing display-engine
>>> specific bindings totally contradicts the whole goal of 
>>
>> No, it does not. DT is well designed for that through expressing
>> compatibility. I did not say you cannot have generic fallback for simple
>> use case.
>>
>> But this (and previous patchset) grows this into generic binding ONLY
>> and that is not correct.
> 
> Can we have a proper definition of what a correct device tree binding is
> then?
> 
> It's a bit surprising to have *that* discussion over a binding that is
> now well older than a decade now, and while there is definitely some
> generic bindings in ePAPR/DT spec, like the CPU ones.

Hm? In ARM world at least they are specific, e.g. they have specific
compatibles.

> 
> If you don't consider that spec to be correct DT bindings, please
> provide a definition of what that is, and / or reasonable alternatives.
> 
> Also, no, a device specific binding isn't reasonable here, because we
> *don't* have a device. From a technical standpoint, the firmware creates

You touch internal parts of the SoC and you list very specific SoC
parts. Interconnect is internal part of the SoC and only specific
devices are using it.

You define here generic SW construct for something which is opposite of
generic: the interconnect connecting two specific, unique components of
one, given SoC.

> the framebuffer, Linux just uses it. Just like you don't have a
> device/platform specific compatible for PSCI, SCPI, et al.

They follow some sort of spec and still they do not reference chosen
SoC-design-specific properties.



Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede]

Hi Krzysztof,

On 30-Jun-25 11:36 AM, Krzysztof Kozlowski wrote:
> On 30/06/2025 10:38, Maxime Ripard wrote:
>> On Mon, Jun 30, 2025 at 10:24:06AM +0200, Krzysztof Kozlowski wrote:
>>> On 29/06/2025 14:07, Hans de Goede wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/06/2025 11:48, Luca Weiss wrote:
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>>>>> Document the interconnects property which is a list of interconnect
>>>>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>>>>> alive when the framebuffer is being used.
>>>>>>>>
>>>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>>>>    power-domains:
>>>>>>>>      description: List of power domains used by the framebuffer.
>>>>>>>>  
>>>>>>>> +  interconnects:
>>>>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>>>>> +
>>>>>>>
>>>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>>>>> some sort of resources in unknown way is not simple anymore. You need
>>>>>>> device specific bindings.
>>>>>>
>>>>>> The bindings support an arbitrary number of clocks, regulators,
>>>>>> power-domains. Why should I artificially limit the interconnects to only
>>>>>> one?
>>>>>
>>>>> And IMO they should not. Bindings are not supposed to be generic.
>>>>
>>>> The simplefb binding is a binding to allow keeping the firmware, e.g.
>>>> uboot setup framebuffer alive to e.g. show a boot splash until
>>>> the native display-engine drive loads. Needing display-engine
>>>> specific bindings totally contradicts the whole goal of 
>>>
>>> No, it does not. DT is well designed for that through expressing
>>> compatibility. I did not say you cannot have generic fallback for simple
>>> use case.
>>>
>>> But this (and previous patchset) grows this into generic binding ONLY
>>> and that is not correct.
>>
>> Can we have a proper definition of what a correct device tree binding is
>> then?
>>
>> It's a bit surprising to have *that* discussion over a binding that is
>> now well older than a decade now, and while there is definitely some
>> generic bindings in ePAPR/DT spec, like the CPU ones.
> 
> Hm? In ARM world at least they are specific, e.g. they have specific
> compatibles.
> 
>>
>> If you don't consider that spec to be correct DT bindings, please
>> provide a definition of what that is, and / or reasonable alternatives.
>>
>> Also, no, a device specific binding isn't reasonable here, because we
>> *don't* have a device. From a technical standpoint, the firmware creates
> 
> You touch internal parts of the SoC and you list very specific SoC
> parts. Interconnect is internal part of the SoC and only specific
> devices are using it.
> 
> You define here generic SW construct for something which is opposite of
> generic: the interconnect connecting two specific, unique components of
> one, given SoC.
> 
>> the framebuffer, Linux just uses it. Just like you don't have a
>> device/platform specific compatible for PSCI, SCPI, et al.
> 
> They follow some sort of spec and still they do not reference chosen
> SoC-design-specific properties.

It does not look like this discussion is going anywhere,
despite 2 drm subsystem maintainers and the simplefb
maintainer telling you that this is what is necessary
and also that we believe this is the right thing todo.

IOW despite 3 domain experts telling you we want this,
you keep coming up with vague, not really technical
argument about this not being generic / simple enough.

Looking at this from a driver pov interconnects are just
another resource we need to avoid from turning off.

And this is simple and generic, the actual display-engine
drivers are very complex and when powering things up
this needs to be done in a very specific order with
specific delays. That is hw-specific. The simplefb/simpledrm
code does not need any of this knowledge everything is
already setup. The simple* drivers just needs to claim all
listed resources in an arbitrary order and without any delays
as someone who has written many many drivers this is
about as simple and generic as it can get.

But as mentioned it looks like this discussion is going
anywhere. Is there some sort of arbitration / appeal
process which we can use when DT-maintainers block
a binding which has been acked and is seen as necessary
by the subsystem maintainers of the subsystem for which
the bindings are ?

Regards,

Hans

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Maxime Ripard]

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

On Mon, Jun 30, 2025 at 11:36:51AM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2025 10:38, Maxime Ripard wrote:
> > On Mon, Jun 30, 2025 at 10:24:06AM +0200, Krzysztof Kozlowski wrote:
> >> On 29/06/2025 14:07, Hans de Goede wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
> >>>> On 27/06/2025 11:48, Luca Weiss wrote:
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
> >>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
> >>>>>>> Document the interconnects property which is a list of interconnect
> >>>>>>> paths that is used by the framebuffer and therefore needs to be kept
> >>>>>>> alive when the framebuffer is being used.
> >>>>>>>
> >>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>>>>>> ---
> >>>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
> >>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> >>>>>>> @@ -79,6 +79,9 @@ properties:
> >>>>>>>    power-domains:
> >>>>>>>      description: List of power domains used by the framebuffer.
> >>>>>>>  
> >>>>>>> +  interconnects:
> >>>>>>> +    description: List of interconnect paths used by the framebuffer.
> >>>>>>> +
> >>>>>>
> >>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
> >>>>>> some sort of resources in unknown way is not simple anymore. You need
> >>>>>> device specific bindings.
> >>>>>
> >>>>> The bindings support an arbitrary number of clocks, regulators,
> >>>>> power-domains. Why should I artificially limit the interconnects to only
> >>>>> one?
> >>>>
> >>>> And IMO they should not. Bindings are not supposed to be generic.
> >>>
> >>> The simplefb binding is a binding to allow keeping the firmware, e.g.
> >>> uboot setup framebuffer alive to e.g. show a boot splash until
> >>> the native display-engine drive loads. Needing display-engine
> >>> specific bindings totally contradicts the whole goal of 
> >>
> >> No, it does not. DT is well designed for that through expressing
> >> compatibility. I did not say you cannot have generic fallback for simple
> >> use case.
> >>
> >> But this (and previous patchset) grows this into generic binding ONLY
> >> and that is not correct.
> > 
> > Can we have a proper definition of what a correct device tree binding is
> > then?
> > 
> > It's a bit surprising to have *that* discussion over a binding that is
> > now well older than a decade now, and while there is definitely some
> > generic bindings in ePAPR/DT spec, like the CPU ones.
> 
> Hm? In ARM world at least they are specific, e.g. they have specific
> compatibles.
> 
> > 
> > If you don't consider that spec to be correct DT bindings, please
> > provide a definition of what that is, and / or reasonable alternatives.
> > 
> > Also, no, a device specific binding isn't reasonable here, because we
> > *don't* have a device. From a technical standpoint, the firmware creates
> 
> You touch internal parts of the SoC and you list very specific SoC
> parts. Interconnect is internal part of the SoC and only specific
> devices are using it.
> 
> You define here generic SW construct for something which is opposite of
> generic: the interconnect connecting two specific, unique components of
> one, given SoC.
> 
> > the framebuffer, Linux just uses it. Just like you don't have a
> > device/platform specific compatible for PSCI, SCPI, et al.
> 
> They follow some sort of spec and still they do not reference chosen
> SoC-design-specific properties.

ish.

I mean, on theory, you're absolutely correct. In practice,
assigned-clock-parents, assigned-clock-rates, or protected-clocks for
example exist and are *only* about SoC-design specific behaviours.

Maxime

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

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 30/06/2025 10:40, Hans de Goede wrote:
>>
>> No one asks to drop them from the driver. I only want specific front
>> compatible which will list and constrain the properties. It is not
>> contradictory to your statements, U-boot support, driver support. I
>> really do not see ANY argument why this cannot follow standard DT rules.
> 
> So what you are saying is that you want something like:
> 
> framebuffer0: framebuffer@1d385000 {
> 	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
> }
> 
> and that the binding for qcom.simple-framebuffer-sm8650-mdss
> can then list interconnects ?
IMO yes (after adjusting above to coding style), but as mentioned in
other response you can just get an ack or opinion from Rob or Conor.

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 02.07.25 um 22:43 schrieb Krzysztof Kozlowski:
> On 30/06/2025 10:40, Hans de Goede wrote:
>>> No one asks to drop them from the driver. I only want specific front
>>> compatible which will list and constrain the properties. It is not
>>> contradictory to your statements, U-boot support, driver support. I
>>> really do not see ANY argument why this cannot follow standard DT rules.
>> So what you are saying is that you want something like:
>>
>> framebuffer0: framebuffer@1d385000 {
>> 	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
>> }
>>
>> and that the binding for qcom.simple-framebuffer-sm8650-mdss
>> can then list interconnects ?
> IMO yes (after adjusting above to coding style), but as mentioned in
> other response you can just get an ack or opinion from Rob or Conor.

But does that work with *any* device that requires interconnects? The 
next such simple-framebuffer device should work out of the box *without* 
the kernel knowing anything about it. That's one of the key features of 
the simple-framebuffer.  If we have to maintainer per-device feature 
sets, it breaks that assumption.

Best regards
Thomas

>
> Best regards,
> Krzysztof

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede]

Hi Thomas,

On 3-Jul-25 8:47 AM, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.07.25 um 22:43 schrieb Krzysztof Kozlowski:
>> On 30/06/2025 10:40, Hans de Goede wrote:
>>>> No one asks to drop them from the driver. I only want specific front
>>>> compatible which will list and constrain the properties. It is not
>>>> contradictory to your statements, U-boot support, driver support. I
>>>> really do not see ANY argument why this cannot follow standard DT rules.
>>> So what you are saying is that you want something like:
>>>
>>> framebuffer0: framebuffer@1d385000 {
>>>     compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
>>> }
>>>
>>> and that the binding for qcom.simple-framebuffer-sm8650-mdss
>>> can then list interconnects ?
>> IMO yes (after adjusting above to coding style), but as mentioned in
>> other response you can just get an ack or opinion from Rob or Conor.
> 
> But does that work with *any* device that requires interconnects? The next such simple-framebuffer device should work out of the box *without* the kernel knowing anything about it. That's one of the key features of the simple-framebuffer.  If we have to maintainer per-device feature sets, it breaks that assumption.

The driver code for this can still be generic and since the driver
will bind to the fallback plain "simple-framebuffer" compatible
this should also work for new platforms.

The e.g. "qcom.simple-framebuffer-sm8650-mdss" compatible would
purely be something in the dt-bindings to document which simplefb
implementations will have interconnects and which ones will not.

The driver does not necessarily need to check these more
precise compatibles, it can still just check for the generic
presence of interconnects.

Regards,

Hans

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Thomas Zimmermann]

Hi

Am 03.07.25 um 10:34 schrieb Hans de Goede:
> Hi Thomas,
>
> On 3-Jul-25 8:47 AM, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 02.07.25 um 22:43 schrieb Krzysztof Kozlowski:
>>> On 30/06/2025 10:40, Hans de Goede wrote:
>>>>> No one asks to drop them from the driver. I only want specific front
>>>>> compatible which will list and constrain the properties. It is not
>>>>> contradictory to your statements, U-boot support, driver support. I
>>>>> really do not see ANY argument why this cannot follow standard DT rules.
>>>> So what you are saying is that you want something like:
>>>>
>>>> framebuffer0: framebuffer@1d385000 {
>>>>      compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
>>>> }
>>>>
>>>> and that the binding for qcom.simple-framebuffer-sm8650-mdss
>>>> can then list interconnects ?
>>> IMO yes (after adjusting above to coding style), but as mentioned in
>>> other response you can just get an ack or opinion from Rob or Conor.
>> But does that work with *any* device that requires interconnects? The next such simple-framebuffer device should work out of the box *without* the kernel knowing anything about it. That's one of the key features of the simple-framebuffer.  If we have to maintainer per-device feature sets, it breaks that assumption.
> The driver code for this can still be generic and since the driver
> will bind to the fallback plain "simple-framebuffer" compatible
> this should also work for new platforms.
>
> The e.g. "qcom.simple-framebuffer-sm8650-mdss" compatible would
> purely be something in the dt-bindings to document which simplefb
> implementations will have interconnects and which ones will not.
>
> The driver does not necessarily need to check these more
> precise compatibles, it can still just check for the generic
> presence of interconnects.

Thanks, that's good to hear.

Best regards
Thomas

>
> Regards,
>
> Hans
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Maxime Ripard]

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

On Thu, Jul 03, 2025 at 10:34:23AM +0200, Hans de Goede wrote:
> Hi Thomas,
> 
> On 3-Jul-25 8:47 AM, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 02.07.25 um 22:43 schrieb Krzysztof Kozlowski:
> >> On 30/06/2025 10:40, Hans de Goede wrote:
> >>>> No one asks to drop them from the driver. I only want specific front
> >>>> compatible which will list and constrain the properties. It is not
> >>>> contradictory to your statements, U-boot support, driver support. I
> >>>> really do not see ANY argument why this cannot follow standard DT rules.
> >>> So what you are saying is that you want something like:
> >>>
> >>> framebuffer0: framebuffer@1d385000 {
> >>>     compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
> >>> }
> >>>
> >>> and that the binding for qcom.simple-framebuffer-sm8650-mdss
> >>> can then list interconnects ?
> >> IMO yes (after adjusting above to coding style), but as mentioned in
> >> other response you can just get an ack or opinion from Rob or Conor.
> > 
> > But does that work with *any* device that requires interconnects? The next such simple-framebuffer device should work out of the box *without* the kernel knowing anything about it. That's one of the key features of the simple-framebuffer.  If we have to maintainer per-device feature sets, it breaks that assumption.
> 
> The driver code for this can still be generic and since the driver
> will bind to the fallback plain "simple-framebuffer" compatible
> this should also work for new platforms.
> 
> The e.g. "qcom.simple-framebuffer-sm8650-mdss" compatible would
> purely be something in the dt-bindings to document which simplefb
> implementations will have interconnects and which ones will not.
> 
> The driver does not necessarily need to check these more
> precise compatibles, it can still just check for the generic
> presence of interconnects.

This ship has kind of sailed though. This binding has been used by
plenty of firmwares and bootloaders over the years, and has been
deployed on plenty of devices already.

Good luck fixing it in all of them, and then updating every device.

Maxime

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

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 03/07/2025 11:41, Maxime Ripard wrote:
>>> But does that work with *any* device that requires interconnects? The next such simple-framebuffer device should work out of the box *without* the kernel knowing anything about it. That's one of the key features of the simple-framebuffer.  If we have to maintainer per-device feature sets, it breaks that assumption.
>>
>> The driver code for this can still be generic and since the driver
>> will bind to the fallback plain "simple-framebuffer" compatible
>> this should also work for new platforms.
>>
>> The e.g. "qcom.simple-framebuffer-sm8650-mdss" compatible would
>> purely be something in the dt-bindings to document which simplefb
>> implementations will have interconnects and which ones will not.
>>
>> The driver does not necessarily need to check these more
>> precise compatibles, it can still just check for the generic
>> presence of interconnects.
> 
> This ship has kind of sailed though. This binding has been used by
> plenty of firmwares and bootloaders over the years, and has been
> deployed on plenty of devices already.
> 
> Good luck fixing it in all of them, and then updating every device.
No one suggested that... We speak about new devices, although maybe this
one SM7635 new device runs plenty of firmwares and bootloaders?

Best regards,
Krzysztof

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Dmitry Baryshkov]

On Mon, Jun 23, 2025 at 08:44:47AM +0200, Luca Weiss wrote:
> Some devices might require keeping an interconnect path alive so that
> the framebuffer continues working. Add support for that by setting the
> bandwidth requirements appropriately for all provided interconnect
> paths.
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)

If simpledrm is being replaced by a proper DRM driver (thus removing
those extra votes), will it prevent ICC device from reaching the sync
state?

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Dmitry Baryshkov]

On Wed, Jul 02, 2025 at 10:43:27PM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2025 10:40, Hans de Goede wrote:
> >>
> >> No one asks to drop them from the driver. I only want specific front
> >> compatible which will list and constrain the properties. It is not
> >> contradictory to your statements, U-boot support, driver support. I
> >> really do not see ANY argument why this cannot follow standard DT rules.
> > 
> > So what you are saying is that you want something like:
> > 
> > framebuffer0: framebuffer@1d385000 {
> > 	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
> > }
> > 
> > and that the binding for qcom.simple-framebuffer-sm8650-mdss
> > can then list interconnects ?
> IMO yes (after adjusting above to coding style), but as mentioned in
> other response you can just get an ack or opinion from Rob or Conor.

But, this way we end up describing MDSS hardware block twice: once with
the proper device structure and once more in the simple-framebuffer
definition. I think this is a bit strange.

I understand your point of having a device-specific compatible string
and also having a verifiable schema, but I think it's an overkill here.

Consider regulator supplies of this simple-framebuffer. Obviously some
of them supply the panel and not the SoC parts. Should we also include
the panel into the respective compat string? What about describing the
device with two different DSI panels?

I think this explodes too quickly to be useful. I'd cast my (small) vote
into continuing using the simple-framebuffer as is, without additional
compatible strings and extend the bindings allowing unbound number of
interconnects.

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Luca Weiss]

Hi Dmitry,

On Sun Jul 6, 2025 at 1:14 PM CEST, Dmitry Baryshkov wrote:
> On Mon, Jun 23, 2025 at 08:44:47AM +0200, Luca Weiss wrote:
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>> 
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>
> If simpledrm is being replaced by a proper DRM driver (thus removing
> those extra votes), will it prevent ICC device from reaching the sync
> state?

How can I try this out? On Milos I don't have MDSS yet but I can add an
interconnect path on another device to try it.

Are there some prints I can enable, or check sysfs/debugfs to see if it
reached sync state?

I only recall seeing sync_state pending warnings in some instances, but
never that it's synced.

Regards
Luca

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Luca Weiss]

Hi Javier,

On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
> Luca Weiss <luca.weiss@fairphone.com> writes:
>
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
>> index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
>> --- a/drivers/gpu/drm/sysfb/simpledrm.c
>> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
>> @@ -2,6 +2,7 @@
>>  
>>  #include <linux/aperture.h>
>>  #include <linux/clk.h>
>> +#include <linux/interconnect.h>
>>  #include <linux/minmax.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_clk.h>
>> @@ -225,6 +226,10 @@ struct simpledrm_device {
>>  	struct device **pwr_dom_devs;
>>  	struct device_link **pwr_dom_links;
>>  #endif
>
> Can you add a /* interconnects */ comment here? Similarly how there is one
> for clocks, regulators, power domains, etc.

Sure!

>
>> +#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
>> +	unsigned int icc_count;
>> +	struct icc_path **icc_paths;
>> +#endif
>>  
>
> ...
>
>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>> +{
>> +	struct device *dev = sdev->sysfb.dev.dev;
>> +	int ret, count, i;
>> +
>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>> +							 "#interconnect-cells");
>> +	if (count < 0)
>> +		return 0;
>> +
>> +	/* An interconnect path consists of two elements */
>> +	if (count % 2) {
>> +		drm_err(&sdev->sysfb.dev,
>> +			"invalid interconnects value\n");
>> +		return -EINVAL;
>> +	}
>> +	sdev->icc_count = count / 2;
>> +
>> +	sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
>> +					       sizeof(*sdev->icc_paths),
>> +					       GFP_KERNEL);
>> +	if (!sdev->icc_paths)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < sdev->icc_count; i++) {
>> +		sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
>> +		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
>> +			ret = PTR_ERR(sdev->icc_paths[i]);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto err;
>> +			drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
>> +				i, ret);
>
> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
> case and also will get this message in the /sys/kernel/debug/devices_deferred
> debugfs entry, as the reason why the probe deferral happened.

Not quite sure how to implement dev_err_probe, but I think this should
be quite okay?

		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
			ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
				      "failed to get interconnect path %u\n", i);
			if (ret == -EPROBE_DEFER)
				goto err;
			continue;
		}

That would still keep the current behavior for defer vs permanent error
while printing when necessary and having it for devices_deferred for the
defer case.

Not sure what the difference between drm_err and dev_err are, but I
trust you on that.

Let me know.

Regards
Luca

>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Luca Weiss]

Hi Krzysztof,

On Sun Jul 6, 2025 at 1:24 PM CEST, Dmitry Baryshkov wrote:
> On Wed, Jul 02, 2025 at 10:43:27PM +0200, Krzysztof Kozlowski wrote:
>> On 30/06/2025 10:40, Hans de Goede wrote:
>> >>
>> >> No one asks to drop them from the driver. I only want specific front
>> >> compatible which will list and constrain the properties. It is not
>> >> contradictory to your statements, U-boot support, driver support. I
>> >> really do not see ANY argument why this cannot follow standard DT rules.
>> > 
>> > So what you are saying is that you want something like:
>> > 
>> > framebuffer0: framebuffer@1d385000 {
>> > 	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
>> > }
>> > 
>> > and that the binding for qcom.simple-framebuffer-sm8650-mdss
>> > can then list interconnects ?
>> IMO yes (after adjusting above to coding style), but as mentioned in
>> other response you can just get an ack or opinion from Rob or Conor.
>
> But, this way we end up describing MDSS hardware block twice: once with
> the proper device structure and once more in the simple-framebuffer
> definition. I think this is a bit strange.
>
> I understand your point of having a device-specific compatible string
> and also having a verifiable schema, but I think it's an overkill here.
>
> Consider regulator supplies of this simple-framebuffer. Obviously some
> of them supply the panel and not the SoC parts. Should we also include
> the panel into the respective compat string? What about describing the
> device with two different DSI panels?
>
> I think this explodes too quickly to be useful. I'd cast my (small) vote
> into continuing using the simple-framebuffer as is, without additional
> compatible strings and extend the bindings allowing unbound number of
> interconnects.

How do we continue on this?

If the current solution is not acceptable, can you suggest one that is?

I'd like to keep this moving to not block the dts upstreaming
unnecessarily - or otherwise I need to drop simple-framebuffer from the
dts patch and keep this out-of-tree along with a patch like this.

Regards
Luca

Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Krzysztof Kozlowski]

On 11/07/2025 09:49, Luca Weiss wrote:
>> I think this explodes too quickly to be useful. I'd cast my (small) vote
>> into continuing using the simple-framebuffer as is, without additional
>> compatible strings and extend the bindings allowing unbound number of
>> interconnects.
> 
> How do we continue on this?
> 
> If the current solution is not acceptable, can you suggest one that is?
> 
> I'd like to keep this moving to not block the dts upstreaming
> unnecessarily - or otherwise I need to drop simple-framebuffer from the
> dts patch and keep this out-of-tree along with a patch like this.


I gave another alternative already (in this thread!) - get an ack or
opinion from @Rob or @Conor. For the cases I am not sure or I got
something wrong, I always defer to @Rob.

Best regards,
Krzysztof

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Javier Martinez Canillas]

"Luca Weiss" <luca.weiss@fairphone.com> writes:

Hello Luca,

> Hi Javier,
>
> On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:

[...]

>>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>>> +{
>>> +	struct device *dev = sdev->sysfb.dev.dev;
>>> +	int ret, count, i;
>>> +
>>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>>> +							 "#interconnect-cells");
>>> +	if (count < 0)
>>> +		return 0;
>>> +

You are already checking here the number of interconnects phandlers. IIUC
this should return -ENOENT if there's no "interconects" property and your
logic returns success in that case.

[...]

>>
>> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
>> case and also will get this message in the /sys/kernel/debug/devices_deferred
>> debugfs entry, as the reason why the probe deferral happened.
>
> Not quite sure how to implement dev_err_probe, but I think this should
> be quite okay?
>

And of_icc_get_by_index() should only return NULL if CONFIG_INTERCONNECT
is disabled but you have ifdef guards already for this so it should not
happen.

> 		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {

Then here you could just do a IS_ERR() check and not care about being NULL.

> 			ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
> 				      "failed to get interconnect path %u\n", i);
> 			if (ret == -EPROBE_DEFER)
> 				goto err;

Why you only want to put the icc_paths get for the probe deferral case? I
think that you want to do it for any error?

> 			continue;

I'm not sure why you need this?

> 		}
>
> That would still keep the current behavior for defer vs permanent error
> while printing when necessary and having it for devices_deferred for the
> defer case.
>

As mentioned I still don't understand why you want the error path to only
be called for probe deferral. I would had thought that any failure to get
an interconnect would led to an error and cleanup.

> Not sure what the difference between drm_err and dev_err are, but I
> trust you on that.
>

The drm_err() adds DRM specific info but IMO the dev_err_probe() is better
to avoid printing errors in case of probe deferral and also to have it in
the devices_deferred debugfs entry.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Luca Weiss]

Hi Javier,

On Fri Jul 11, 2025 at 11:21 AM CEST, Javier Martinez Canillas wrote:
> "Luca Weiss" <luca.weiss@fairphone.com> writes:
>
> Hello Luca,
>
>> Hi Javier,
>>
>> On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
>
> [...]
>
>>>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>>>> +{
>>>> +	struct device *dev = sdev->sysfb.dev.dev;
>>>> +	int ret, count, i;
>>>> +
>>>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>>>> +							 "#interconnect-cells");
>>>> +	if (count < 0)
>>>> +		return 0;
>>>> +
>
> You are already checking here the number of interconnects phandlers. IIUC
> this should return -ENOENT if there's no "interconects" property and your
> logic returns success in that case.

We shouldn't error out in case there's no interconnects defined for this
simple-framebuffer though? That'd break all other usages of it?

>
> [...]
>
>>>
>>> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
>>> case and also will get this message in the /sys/kernel/debug/devices_deferred
>>> debugfs entry, as the reason why the probe deferral happened.
>>
>> Not quite sure how to implement dev_err_probe, but I think this should
>> be quite okay?
>>
>
> And of_icc_get_by_index() should only return NULL if CONFIG_INTERCONNECT
> is disabled but you have ifdef guards already for this so it should not
> happen.
>
>> 		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
>
> Then here you could just do a IS_ERR() check and not care about being NULL.

But checking also for NULL shouldn't hurt either, in case the compile
guards get removed in the future or something?

Quote:
> * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
> * when the API is disabled or the "interconnects" DT property is missing.



>
>> 			ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
>> 				      "failed to get interconnect path %u\n", i);
>> 			if (ret == -EPROBE_DEFER)
>> 				goto err;
>
> Why you only want to put the icc_paths get for the probe deferral case? I
> think that you want to do it for any error?

This is the same logic as e.g. for the regulator code in simpledrm. The
idea seems to be that in case some regulator (or here interconnect)
doesn't probe correctly, we still try anyways. Just for EPROBE_DEFER we
defer and wait until the supplier is available.

So defer -> defer simpledrm probe
So error -> ignore error and continue probe

>
>> 			continue;
>
> I'm not sure why you need this?

For the above behavior.

I guess there were some original design decisions behind handling it
this way, so I don't see a reason to handle it differently for
interconnects.

>
>> 		}
>>
>> That would still keep the current behavior for defer vs permanent error
>> while printing when necessary and having it for devices_deferred for the
>> defer case.
>>
>
> As mentioned I still don't understand why you want the error path to only
> be called for probe deferral. I would had thought that any failure to get
> an interconnect would led to an error and cleanup.

See above.

Regards
Luca

>
>> Not sure what the difference between drm_err and dev_err are, but I
>> trust you on that.
>>
>
> The drm_err() adds DRM specific info but IMO the dev_err_probe() is better
> to avoid printing errors in case of probe deferral and also to have it in
> the devices_deferred debugfs entry.