Hi Javier,
On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
> Luca Weiss <luca.we...@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 <tzimmerm...@suse.de>
>> Signed-off-by: Luca Weiss <luca.we...@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 <javi...@redhat.com>
