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.0Luca 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
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)
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
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>
"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
© 2016 - 2025 Red Hat, Inc.